Hacker News Reader

I can't say I enjoyed code reviewing but I never felt like I was being mean to my teammates. I was really happy when a teammate found a bug in code review of my code rather than that bug getting checked in. I was also really happy when they shared knowledge and I learned something. Maybe they shared a pattern I was unfamiliar with or maybe they pointed out an existing function I could use I didn't know was in the code.

I assumed the same was true for them. I learned from another team member to try to phrase comments as questions. "Did you mean to do X here? It seems like it might have issue Y" etc...

Bugs are easy.... you just point out the bug and move on... design flaws are hard... it's hard to bring up the fact that while yes, your module solves this one immediate problem, it was written so rigidly that two sprints from now the whole thing will need to be re-written scratch. Or maybe you took a particular approach that seemed easy to implement, but it won't do async stuff properly and so it needs to be re-worked from the ground up. And I get you solved your immediate problem for your immediate task, but whoever works on this next is going to have to completely re-think the approach and that's not theoretical, that next week when we add more behavior.

You ever worked on a team a where someone would go write code in their own crazy way that wouldn't follow any sort of existing pattern or take advantage of existing tooling? So they spend like a week on a simple task because they re-wrote the strings.c because they didn't want to include it? Yeah, they get fired eventually, but they are the worst to write code reviews for, because you go into it just thinking WTH, why are they doing it this way, this entire approach is convoluted and error-prone and rigid and fragile and complicated.

If a super engineer has advice for me on how to deal with this stuff, I'm all ears. I usually just ignore it unless it directly impacts me and then go back and re-write it when we have to add on to it/make it interop with some more code/release it as an available API.

I had a coworker who had a 2,000 line PR that took me 3 days to review, it was like code breaking. Custom abstractions upon custom abstractions, abstractions in custom imported utilities for a one-liner that was only used once. Basically, instead of writing the code, he made code that generated the code that was needed, once the program was running. Dear Lord. Like somehow this monstrosity was good because instead of creating the feature, he built tooling to make many identical features whenever we want! Except nothing can be different, if it's different we need to rewrite the whole thing.

Every 4 hours or so I would ask my boss if I really had to do it. He told me to just keep going, I think he was building a case for letting him go. I still only made like 10 comments, needless to say they weren't taken well. What a nightmare.

Tangential to this whole thread, but guys like that, every day he worked took two days for other devs to fix/undo his work. Took my company a few years to catch on, he was a senior dev and also good at talking himself up. I would have paid him the same salary to go sit on a beach somewhere and enjoy himself instead of touching the code, we'd all have been happier. In fact the world would be a better place, because now he is presumably working somewhere else doing the same things.

I hated code reviews when I first had to do them because it was a toxic place. I hate them less upon focusing on what you describe. I'll add to it that I've taken a more conversational tone. Sometimes I'll just add comments to talk about how this code fits into the whole system and weird legacy stuff that is affecting what they are writing.

> After further thought, I think a fundamental difference is I really want to see my coworkers succeed, and don't enjoy their failure. Also I dislike all forms of toil.

I'm in both of these camps too. I think I'd dislike code reviews if I didn't remind myself:

* Code my coworkers write is code I don't have to write, so it's saving me toil.

* Issues I catch in review are issues that won't lead to late night debugging sessions catching heisenbugs on the eve of shipping our product, so it's saving me some of the worst kinds of toil.

A little work now to save me a lot of work later. And I've been burned by suddenly getting stuck maintaining & bugfixing code that I got lax about reviewing. Each issue I catch, I think "thank goodness I caught that now - that'd be a pain in the ass to debug".

It also makes me genuinely appreciate review feedback catching all the dumb shit I might do, which can really help when your quick change turns into a slog of missed edge cases, "can you fix X while you're in there", etc.

Turn it around - make sure each person has the chance to review one anothers' code, and make sure the senior members of the team lead the way in using those reviews as opportunities to both highlight great things in the code and to propose changes /in a way that teaches/ how you're analyzing it and what made you propose that change. If they challenge it, great! They're thinking about your reasoning on their own, and you can use that to come to a new consensus on the team's approach to development.

There's great health to be gained from code reviews. But they really do need to practiced, reconfigured, and practiced again so your team gets what they need from it.

While I have similar feelings, I enjoy code reviews on high quality code. I like reading high quality code. But yes, the enjoyment of a code review is inversely proportional to the amount of feedback required. Some people enjoy mentorship more than others, and that's ok.

> "If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel"

This quote made me laugh from the exaggerated "insane pleasure" in other's mistakes. But I feel this way sometimes. I'm completely sure it comes from my own insecurities, so that when I see another guy missed something I wouldn't, I get a feeling of relief - actually I'm not the worst programmer in the world.

So for me this feeling comes from impostor syndrome - I thought I was bad, I saw another guy who was worse, I felt good. Insecurity is really a self-fulfilling prophecy. I'm trying to work on it by forcing myself to ask questions I think are dumb, and not fearing judgment from others. This in turn will also influence my peers to feel less afraid of their doubts, turning the spiral around.

A study from Google [1] agrees with this: they found that the best predictor of team quality was the willingness to ask questions and express opinions without fear. Quoting Google:

"In a team with high psychological safety, teammates feel safe to take risks around their team members. They feel confident that no one on the team will embarrass or punish anyone else for admitting a mistake, asking a question, or offering a new idea."

[1] https://www.inc.com/justin-bariso/google-spent-years-studyin...

I find using automated tools as part of the pipeline, like codefactor, are a decent first step to sort out some of the glaring issues. As they're automated and act as quality gates before an actual human comes to do a review then they're not as bad as they could be. Sure there scope can be limited, depending on what language/framework you're using, but common ones catch quite a lot of obvious issues.

I do tons of code review and enjoy it. I approach it with both learner/teacher attitude (not critic) and treat it as project learning and communication exercise.

Best way of doing code review is by asking questions.

Recently coworker recommended this talk which suggest good attitude IMO - you can find it under "strong code review" from rails conf

> And I won't comment unless I am convinced it is important.

I tend to lean this way as well. Im of the mindset that most things that are "caught" in a code review don't add any real value. Or maybe I just have bad coding standards.