4 ms·

While I agree with all the points listed in the article, it highlights for me a major problem of a lot of code reviews. Most code reviews seem to focus on: 1.

While I agree with all the points listed in the article, it highlights for me a major problem of a lot of code reviews. Most code reviews seem to focus on: 1. Examining what the change does 2. Finding ways to make the change in a nicer way. E.g. Refactoring etc. This leaves out the key step 0 - what is actually trying to be achieved, does it need to be done and is there a better (maybe completely different) way to do it. This leads to a focus on relatively trivial matters such as naming conventions and method lengths. I think that the underlying reason for this is laziness. Talking about clever refactoring is an easier/faster process than understanding the 'why'.

yoran 11y ago
I agree with this. But I don't think code reviews are suited for this step 0 as you say. Just the way a pull request is formatted, it's very hard for the reviewer to deduce from the changeset what the high-level design of the code is. That's why we discuss these high-level design and architectural decisions before-hand so that they are known to the reviewer at the time of the review. We're a small team so it works well. But I'm not sure how this scales up as the team gets bigger. I would like to know how bigger teams approach this problem!
jeremiep 11y ago
Same here, I've seen too many code reviews where people complain about a badly named variables and nobody saw the design was faulty leading to costly bugs to fix in production. One thing we did on the current project is pre-commit reviews in pairs. This ensures at least two people in the team knows about the changes, let us talk about the why and how of the changes, and possibly teach a coworker new things in the process. What it ended up doing is that every programmer now self-reviews their own changes prior to the actual review knowing they'll soon share it all face to face with a coworker. Turns out the talks are now about the design of the code, not how it looks.
MichaelGG 11y ago
I'm guilty of getting stuck up on trivial formatting issues. When someone pushes a commit that has random whitespace (trailing or arbitrary newlines all over, or just inconsistent spacing), it feels sloppy. Same for many other simple things. If the code is unnecessarily superficially ugly, it sets up a block in my mind that makes it harder to focus on the real issues. Is it wrong to kick this stuff back and tell devs to make it pretty first?
- dkubb 11y ago
  I believe that ugly code is usually buggy code. If someone didn't take the time to deal with trivial issues like formatting then it makes me wonder if they spent time thinking hard about the actual problem they were trying to solve. I usually explicitly do code reviews in 2 passes. I look for low level problems first, and also try to identify ways that they can be tested using tools so that next time they are caught before the code is sent for review. My second pass looks at higher level problems with the overall design and checks the functionality to make sure it matches the objectives.
dsuth 11y ago
You should not really be arguing about design and architecture in a code review - that should be done in design review.

yoran 11y ago

I agree with this. But I don't think code reviews are suited for this step 0 as you say. Just the way a pull request is formatted, it's very hard for the reviewer to deduce from the changeset what the high-level design of the code is. That's why we discuss these high-level design and architectural decisions before-hand so that they are known to the reviewer at the time of the review. We're a small team so it works well. But I'm not sure how this scales up as the team gets bigger. I would like to know how bigger teams approach this problem!

jeremiep 11y ago

Same here, I've seen too many code reviews where people complain about a badly named variables and nobody saw the design was faulty leading to costly bugs to fix in production. One thing we did on the current project is pre-commit reviews in pairs. This ensures at least two people in the team knows about the changes, let us talk about the why and how of the changes, and possibly teach a coworker new things in the process. What it ended up doing is that every programmer now self-reviews their own changes prior to the actual review knowing they'll soon share it all face to face with a coworker. Turns out the talks are now about the design of the code, not how it looks.

MichaelGG 11y ago

I'm guilty of getting stuck up on trivial formatting issues. When someone pushes a commit that has random whitespace (trailing or arbitrary newlines all over, or just inconsistent spacing), it feels sloppy. Same for many other simple things. If the code is unnecessarily superficially ugly, it sets up a block in my mind that makes it harder to focus on the real issues. Is it wrong to kick this stuff back and tell devs to make it pretty first?

dkubb 11y ago
I believe that ugly code is usually buggy code. If someone didn't take the time to deal with trivial issues like formatting then it makes me wonder if they spent time thinking hard about the actual problem they were trying to solve. I usually explicitly do code reviews in 2 passes. I look for low level problems first, and also try to identify ways that they can be tested using tools so that next time they are caught before the code is sent for review. My second pass looks at higher level problems with the overall design and checks the functionality to make sure it matches the objectives.

dsuth 11y ago

You should not really be arguing about design and architecture in a code review - that should be done in design review.