4 ms·

Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time. When colleagues go to review the code,

Engineers work to deliver business value and commit the code that flawlessly delivers that value the very first time. When colleagues go to review the code, they are edified in its reading and quickly approve the merge request. There is no need for feedback because the code was perfect. I strongly disagree with the premise of this article. I look at code review the exact same way as I would look at the first draft of an email or a blog post. If you look up any piece of writing advice, one of the biggest things people say is "Don't worry about getting everything perfect the first time". Instead, it's really important to focus on getting something working first, and then you can spend your "second draft" refactoring that base of working code so it's more maintainable & readable. From that perspective, find code reviews really valuable as a contributor because it lets me get an outside perspective on the "first draft" of my code, and then it'll make it way way easier to write the code "correctly" the second time. So I really really want to push back on this idea that the "ideal" code review process involves a rubber stamp. That's just entirely foreign to the way I think about code reviews.

wffurr 5y ago
I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change. When it comes time to actually get approval to check in, then I try very hard to get it right. I put on my code reviewer's hat and review on my own code. Ensure that I've run the formatter, linter, pre-submit tests, and code coverage tools. And only then do I actually mail it for review. My ideal review at that step is a rubberstamp. I have a few colleagues who do the same; it's always a joy to review their changes because they're focused and well-written with tests, coverage, etc. all in place.
- bcrosby95 5y ago
  > I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. Yeah, we do a database & api design review before anything is actually coded. If you don't get that right you might have to basically start all over.
- nightpool 5y ago
  I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change. That makes sense for some types of changes, but it's too high-level to address the actual well-factored nature of the code, which is one of the main things I'm thinking about in the "second draft" of my code. Nobody's design doc is going to address "what's the right set of helper functions to extract here?" or "what's the right way to structure this logic to make it the most readable?". Design docs are great and they're super important for bigger changes, but they're often completely orthogonal to "is the actual code that implements this design doc readable and maintainable", which is the biggest question I'm seeking to answer in code reviews. This is also completely orthogonal to "do I have tests for this", "have I run the linter", etc. Certainly there are types of readability problems that running the linter or writing tests for your code can catch. Having focused changes with test coverage is only one part of making sure your code is long-term maintainable by the rest of the team.
ketzo 5y ago
On the flip side, though, I think it's important for the code that you send to other engineers for review to be at least kind of polished, because otherwise you risk just wasting other people's time. If you get feedback on your code that you already knew about, you wasted someone's time, and I think that's inefficient and a little bit disrespectful. But I definitely agree that code review is very much a drafting process. It's just that you need to walk a line.
- nightpool 5y ago
  I get where you're coming from, but if a friend asks me to look over an important email that they're planning on sending to their boss, I don't think of it as "wasting my time" if the email has a bunch of grammatical errors in it. Instead, I'm happy that I'm able to help them by applying a fresh pair of eyes to the problem, since they've probably spent a really long time working on the actual content of the email and haven't had the opportunity to step back and consider . It's just an efficient division of labor. In the same way, code review is a great opportunity to let someone who wasn't involved in the actual writing of the code approach the problem & solution space with fresh eyes, now that you've done the hard work of translating the solution into something that actually works. And code reviews are even more collaborative then that. When I'm looking over my friend's email, it's still ultimately their email and there are types of feedback that it would be inappropriate to give. But when you're contributing to a shared codebase, it's more like a novel that's been co-written by multiple different authors—obviously you want everybody on the team to be able to have input and feedback into what the "collective style" of the project is going to be.
  - shkkmo 5y ago
    > they've probably spent a really long time working on the actual content of the email and haven't had the opportunity to step back and consider Before you ask someone to proof read anything, you should generally take the effort to step back and try to proof read it yourself. The same goes with code reviews. I think there is absolutely value in soliciting feedback throughout the process, but would consider those earlier feedback stagea to be design and/or implementation reviews (which depending on feature size should happen with varying degrees of formality.) It is useful to be explicit about the types of review because the types feedback that are useful vary throughout the process. If you are looking for design feedback and someone complains about unimplemented tests, that generally isn't very helpful. If you are looking for a final code review for polished code and you get feedback about the underlying design, then you really needed to get that feedback earlier in the process.

wffurr 5y ago

I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change. When it comes time to actually get approval to check in, then I try very hard to get it right. I put on my code reviewer's hat and review on my own code. Ensure that I've run the formatter, linter, pre-submit tests, and code coverage tools. And only then do I actually mail it for review. My ideal review at that step is a rubberstamp. I have a few colleagues who do the same; it's always a joy to review their changes because they're focused and well-written with tests, coverage, etc. all in place.

bcrosby95 5y ago
> I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. Yeah, we do a database & api design review before anything is actually coded. If you don't get that right you might have to basically start all over.
nightpool 5y ago
I'll often do the "first draft" as a design doc with an API sketch or a quick prototype that I'll post in chat or email to the team but not actually "mail for review" per the tool. That way I get early feedback that I'm going in the right direction for a code change. That makes sense for some types of changes, but it's too high-level to address the actual well-factored nature of the code, which is one of the main things I'm thinking about in the "second draft" of my code. Nobody's design doc is going to address "what's the right set of helper functions to extract here?" or "what's the right way to structure this logic to make it the most readable?". Design docs are great and they're super important for bigger changes, but they're often completely orthogonal to "is the actual code that implements this design doc readable and maintainable", which is the biggest question I'm seeking to answer in code reviews. This is also completely orthogonal to "do I have tests for this", "have I run the linter", etc. Certainly there are types of readability problems that running the linter or writing tests for your code can catch. Having focused changes with test coverage is only one part of making sure your code is long-term maintainable by the rest of the team.

ketzo 5y ago

On the flip side, though, I think it's important for the code that you send to other engineers for review to be at least kind of polished, because otherwise you risk just wasting other people's time. If you get feedback on your code that you already knew about, you wasted someone's time, and I think that's inefficient and a little bit disrespectful. But I definitely agree that code review is very much a drafting process. It's just that you need to walk a line.

nightpool 5y ago
I get where you're coming from, but if a friend asks me to look over an important email that they're planning on sending to their boss, I don't think of it as "wasting my time" if the email has a bunch of grammatical errors in it. Instead, I'm happy that I'm able to help them by applying a fresh pair of eyes to the problem, since they've probably spent a really long time working on the actual content of the email and haven't had the opportunity to step back and consider . It's just an efficient division of labor. In the same way, code review is a great opportunity to let someone who wasn't involved in the actual writing of the code approach the problem & solution space with fresh eyes, now that you've done the hard work of translating the solution into something that actually works. And code reviews are even more collaborative then that. When I'm looking over my friend's email, it's still ultimately their email and there are types of feedback that it would be inappropriate to give. But when you're contributing to a shared codebase, it's more like a novel that's been co-written by multiple different authors—obviously you want everybody on the team to be able to have input and feedback into what the "collective style" of the project is going to be.
- shkkmo 5y ago
  > they've probably spent a really long time working on the actual content of the email and haven't had the opportunity to step back and consider Before you ask someone to proof read anything, you should generally take the effort to step back and try to proof read it yourself. The same goes with code reviews. I think there is absolutely value in soliciting feedback throughout the process, but would consider those earlier feedback stagea to be design and/or implementation reviews (which depending on feature size should happen with varying degrees of formality.) It is useful to be explicit about the types of review because the types feedback that are useful vary throughout the process. If you are looking for design feedback and someone complains about unimplemented tests, that generally isn't very helpful. If you are looking for a final code review for polished code and you get feedback about the underlying design, then you really needed to get that feedback earlier in the process.