Live data from Hacker News

Code reviews do find bugs

two-wrongs.com

161–169 of 169 posts

Re: Code reviews do find bugs

#161
post #110

The value of code reviews really depends on the code and the person working on the code. For a team who have spent years working on the same repo, code reviews may not hold much value. But if you have a new guy on the team, or a junior, you'll definitely want to review their code. Code reviews can also do more than just find bugs. You can point out a better way of doing things. Maybe this SQL could be more efficient.…

Code reviews also keep the team up to date with what is changing in the code

Commonly stated, except I think that statement is wrong. A PR is the thing that has the effect of sending emails to the team, it is the PR that leaves behind a webpage & summary of what has changed. It is the PR that is doing the work there, not the CR. Which implies, you can open and then immediately merge PRs without CR to get that same benefit.

CR does give others a chance to study code, and become familiar with it - but that is different from "keeping up to date."

Re: Code reviews do find bugs

#162

Earlier quoted context omitted.

I'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...): CR slows things down & often unnecessarily. Overly time consuming Time lost with needless explanations. Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is prob…

This is a bunch of downsides, but I guess I'm asking, does anyone intelligent think that given the alternatives, on balance, it's better to not do them? You seem quite against them - what would you propose as a concrete alternative and what would the result likely be?

[deleted]

Re: Code reviews do find bugs

#163

My beef with code reviews is that often they lead to tremendous amounts of wasted time, that's many thousands spent in a single week sometimes for simple pull requests. Working from 6 years, not much, and not in as many places like others, I have built the opinion that code reviews are like tests, they should be used as a tool when they are necessary, they shouldn't be the default for every change. In best case scena…

There’s probably more to this story. It’s somewhat a skill and maybe art to create MRs so that they are easy to review and get approved.

It is also true for the feedback on MRs. Providing compressed and succinct feedback makes it faster to address.

Almost like “if the change is difficult, refactor and make the change easy”. There are many ways to do one thing, some are better, some are not.

Companies and teams that have good review culture are successful in using the reviews as a tool.

Re: Code reviews do find bugs

#164

Earlier quoted context omitted.

"Tech debt and code smell are real" I think what I struggle most with is that often times there's a valid business reason to "just ship it ASAP", but the missing piece is the accountability around the conditions attached to that. Like, okay, if we don't want to fix this now because it needs to be in the next release then we can merge it as-is, but you can't document this externally, it can't because part of an API, a…

"There is no later." is my new mantra.

I have to favorite mantras related to this:

There is nothing more permanent than temporary.

... and ...

We don't have time to do it right, but we do have time to do it twice.

Re: Code reviews do find bugs

#165

Earlier quoted context omitted.

As far as dodging projectiles goes: yes the office is supposed to be a safe space, and if someone threw a chair at me, one of us would not work there the next day. (Add normal caveats for "maybe they threw the chair to save you from the ninja creeping up behind you".)

The "chair" in question was 3 coat hangers and 2 frisbees... I have no idea how it held a human up. The "throw" was more of a shove and the thing went flying in my general direction. The only thing that chair was going to hurt was my feelings. It was far less scary than the office where I sat on an ammo shipment.

I'm shocked, shocked, that you actually were just lying about your experiences to make whatever point you wanted. The one time I worked with a guy who lied about throwing chairs at people it turned out he was a wanted serial killer, and the company almost went bankrupt. Of course none of that is true, but if it was, it would be a colorful way to back up my refusal to work with people like that!

Re: Code reviews do find bugs

#166
post #3

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issu…

"Tech debt and code smell are real" I think what I struggle most with is that often times there's a valid business reason to "just ship it ASAP", but the missing piece is the accountability around the conditions attached to that. Like, okay, if we don't want to fix this now because it needs to be in the next release then we can merge it as-is, but you can't document this externally, it can't because part of an API, a…

> Everyone is happy to smile and nod and the appropriate tickets are filed with deadlines attached

Once I did a final review of a backlog of a project that got decommissioned after a decade of development and use. Most of the "fix tech debt" tickets were still there in the backlog. And how could it be otherwise if the tech debt tickets always got assigned priority 2, and everything the management wanted done got assigned priority 1, and there were always more priority 1 tasks than we could fit into a spring?

Next time, I will have no confusion about what "priority 2" actually means. It's in the backlog to make you feel happy, but you are not supposed to actually ever do it. (If by some miracle all current "priority 1" tickets get done at some moment, some of your team members will be assigned to a different project.)

Re: Code reviews do find bugs

#167
post #3

Agreed. I mainly manage and review code at this point in my career. I find many bugs, every once in a while finding something that would have caused an outage or notable problem for users. What I find more though is code that isn't thought through. Tech debt and code smell are real, and they affect the performance of a team. Nipping that in the bud takes quality PR reviews and time to meet with submitters around issu…

I am a new manager and I am struggling to get my team to understand the value in code reviews. I have been through so many rewrites and re-re-writes of spaghetti code, I am much more critical now reviewing code, and I am trying to promote this culture on my team. Do you have any suggestions? - The same people leave detailed comments on others' merge requests, but get discouraged when nobody else puts in the same amou…

I don't know the culture of your company, so I am just guessing here. In my experience, these are the most frequent obstacles to code reviews:

- The team is understaffed, there is not enough time to do things properly. I can write the code in a day, but it would take two days to make it really good (to do the code review carefully, to address the comments, to review the rewrites), and at the end I would get scolded because my measured productivity got too low. In other words, in theory the company wants code reviews, but in practice it rewards those who skip them, and punishes those who actually do them. Also, the more busy people are, the longer I probably need to wait until someone finally reviews my code.

- Big ego. Some people take a suggestion to improve their code as a personal offense; as if the other person was telling them "I am a better programmer and indeed a better person than you". (Often the same people are quite happy to give a lot of comments to their colleagues, some of them genuine improvements, many of them merely "I would have done it differently" except they insist that their way is always better.) If you have such people on your team, you are going to have problems. In my experience, about 10-20% of men are like that. (No idea about women; I would expect the fraction to be smaller, but I didn't have enough female colleagues to verify this.) I prefer teams where no one is like that, because one person like that can ruin the entire team's dynamic.

- Lack of experience. A junior developer reviewing another person's code sometimes simply doesn't know what is good and what is bad. "If the code compiles, it's probably good? Wow, there are even unit tests, and those pass too; what else is there to complain about?" But I think that even having the code reviewed by a junior is a good thing; at least this gives them an opportunity to ask why a particular approach was chosen.

- If you only have one person on a project, if someone working on a different project is supposed to review their code, they probably feel like "I don't know much about this project, I don't know what their constraints are, how the rest of the project looks like... I guess, unless I see an obvious horrible error, I will just approve it".

So basically it's either the ego or external pressure (or both). Depending on your position in the company, you may be unable to do anything about that.

Some people have an ego that you can't fix. You can avoid hiring them, but if they already are on the team and they otherwise do their job well... You just can give more attention to this in the future. For example, if you have a group of people who cooperate well, do not just randomly redistribute them to other projects once their project is over; instead maybe try giving a new project to the same group. When interviewing new team members, always ask for feedback from the existing team members.

Some companies create toxic environment by comparing developers against each other, sometimes with the explicit goal to fire the least performing 20% each year. In such environment, obviously the code reviews also become adversarial, and people will try to avoid having their code reviewed, and some will use the code review as an opportunity to harm their competitors. In a cooperative environment, code reviews work much better; it's people working together to achieve a common goal. To create a cooperative environment, you need to treat the team as a whole. (For example, never measure individual velocity, because people will obviously soon notice that helping their team members may hurt their personal metric. You don't want to fire a person just because they spend too much time helping their colleagues with their tasks.)

EDIT:

If you use some automatic system to highlight problems with code, it will probably hurt the ego less than a similar comment made by another human. Just make sure to configure the tool properly, for example to write comments but never actually block the commit, otherwise developers will get really angry about the false positives.

Re: Code reviews do find bugs

#168

Earlier quoted context omitted.

Never realized this was a debated topic. Are there smart people who believe in not having code reviews? What's the best argument against code reviews?

I'd guess I've done somewhere in the ballpark of 1000 to 2000 code reviews. After that time, my opinion of them has drastically changed. I can give something of a long laundry list (my apologies for it being kinda ranty...): CR slows things down & often unnecessarily. Overly time consuming Time lost with needless explanations. Somewhat impossible when reviewer pool is one or two people. A bigger reviewer pool is prob…

A number of these critiques seem like problems with the work culture rather than code reviews. Yes, nitpicking, cargo culting, gatekeeping, and favoritism are problems, but are they problems with _code reviews_ specifically?

Some of the others are actually desirable features of code reviews in my experience. Yes, we don't want needless explanations or the code reviews only going to a small number of people, but my experience is that code reviews are a good mechanism for encouraging authors to write self-explanatory code and for sharing expertise around the team.

This list just doesn't jibe with my experiences.

Re: Code reviews do find bugs

#169
post #52

Earlier quoted context omitted.

Please continue to be diligent in your PRs! Garbage code belongs in the garbage.

Plenty of garbage code is making billions for all of the big tech companies that exist. It's mostly garbage code at Amazon, Facebook, etc.. Let alone the smaller companies that were "bootstrapped", had founder level code be built and then are in that constant "fixing and scaling" phase. More though, what kind of culture develops when someone works on something - and then it's called garbage? On the other hand, when s…

I hear you but I have had to waste countless hours and endure increased stress because someone pushed code that is way more LOC than needed, difficult to read, no docs, against best practices, on and on. I have also dealt with chunks of garbage code that have worked for years and no need to modify it and I'm fine with that. Getting an MVP out the door or a Hack Day project is one thing. Literally not being able to write good code when the dev has plenty of time to due so because they lack the skillset and/or discipline to do so is not cool.
Post reply on HN