5 ms·

Code review is a flawed process, especially for junior engineers. You spend time and effort, maybe days of work, getting this to work, and then some know-it-all

Code review is a flawed process, especially for junior engineers. You spend time and effort, maybe days of work, getting this to work, and then some know-it-all leaves a bunch of nit-picky comments or tells you you're doing it wrong. I tell most of the juniors I work with to work defensively against this using a few strategies: - smaller PRs. Break the work up any way you can. Make small tickets or submit your PRs with Part 1, part 2, etc. You can't release half-baked work, but you can usually find opportunities to split things up. Nobody should be submitting 20 file PRs and not expecting a lot of comments. - Validate your strategy before building it. If there's a senior engineer in your team who nitpicks your code, get their buy-in before writing it. - often bad code is the result of not really knowing how to approach a problem. Don't just write the code and slap together a PR. You might need to figure out a working solution, then go back and tweak it and refactor it before submitting it. - review your own PRs and use a critical eye. You'll catch the low hanging fruit (like forgetting console.log calls). Every time you have the urge to leave a comment justifying a choice, question whether it's the right choice at all.

danparsonson 3y ago
Alternative view - if you approach a code review (or indeed any similar process) as a competition to be won or some kind of personal validation process, then you're likely to be disappointed and are potentially wasting an opportunity to learn something. Especially as a junior!
SilverBirch 3y ago
Totally agree it's flawed, but very often the reason it's flawed is because something has gone wrong somewhere else. Yes, it's very common for someone to do a PR and for someone else to turn around and go "Oh god, why the hell did you do it like this?!" - Fundamentally challenging the entire purpose of the code. What's happening there is the code review is just highlighting the fact you didn't have a design review. You also have to have a shared understanding of what's a reasonable request in a review, I've been in teams that relentlessly nitpick and teams that just "LGTM"s, eithers fine as long as you're agreed and consistent on what your team wants to do.
- datavirtue 3y ago
  [flagged]
  - digging 3y ago
    Honestly, you sound like an asshole here. Do you realize that Adderall is a medication? Do you realize how fucked up it is to call someone mentally defective as if your brain meet some standard of perfect functioning? (it doesn't.)
    - datavirtue 3y ago
      No one cares after they tumble over the edge and make everyone miserable. It ain't helping anymore.
  - thegrimmest 3y ago
    I dunno mate, I think adults should be able to handle being chortled at. You're responsible for your own emotional state. If you've done something laughable (or even if you haven't), then don't be surprised when people laugh. Taking it personally is optional and not recommended.
    - atq2119 3y ago
      I agree that people shouldn't be too thin-skinned, it's still important to stay civil and supportive. Being able to clearly and unambiguously convey that a change sucks while not hurting the other person's feeling is a useful skill to practice. (It's okay to fail at it, too, as long as you're aware that that's almost always a weakness.)
      - thegrimmest 3y ago
        The issue is the broad, ambiguous, definitions of things like "civil". In many cultures (eg. French, Russian) a direct, no-nonsense communication style is the default. You can just say things like "Your idea is hilariously stupid" and no one really gets bent about it. In the world of multicultural, international business, this seems like the most viable default, simply because it's the easiest to codify and communicate. Trying to teach very culturally-specific minutia of civility to a diverse staff just gets in the way. I'd rather work somewhere the burden of not being offended is placed on the listener, not the speaker. Somewhere we can just say what we think.
verall 3y ago
> smaller PRs I think the opposite works better - submit thousands of lines of changes, you will get like 10 comments, address all of them, and then you're gold. The reviewer has no time to slog through thousands of lines of changes unless the code you're touching is their baby.
kiitos 3y ago
You've probably heard this before, but it's worth repeating: code review doesn't have to be a "flawed process" that is "nit-picky" -- if it is, there's a problem in your organization that needs to be -- and absolutely can be -- fixed!
Shrezzing 3y ago
> Validate your strategy before building it. If there's a senior engineer in your team who nitpicks your code, get their buy-in before writing it. I think this hits the nail on the head. Code reviews have been adopted by most of the orgs I've worked at as a way to reduce any technical barrier to starting a project. Technical planning is moved from the start of a project to the end (or more accurately, to what has now become the middle). I'm certain there are code review processes that are done well, but they don't appear to be common.
ar_lan 3y ago
Everything you listed are exactly what the "know-it-all" senior developer is expecting already, though. If you're doing these things before you submit your review for eyes to look at, you'll come to a point where your reviews should only go through 0-2 revisions before submission is ready. Personally, if I see a review that is >10 files that isn't explicitly a "Refactor" review, or I've been prepped ahead of time, it's probably going to take a long time to get the review out, because there are so many things to iterate on. I also have to block out a lot of time to even do the singular review, because it is so long and there is so much cognitive load to carry with it. Smaller reviews are almost always better. If a review really can't be "completed" in a single PR, then I've also suggested 1/x reviews where bones are placed but gated under an FSS or something similar. This prevents code that shouldn't be run from being run until the whole feature (even if small) is complete, and lets the reviews focus on independent parts. -- Essentially, I'm saying I disagree with you placing the "error" on the senior developers in this scenario. Burdening someone with insanely large reviews is the err of the submitter.
- BlargMcLarg 3y ago
  Counterpoint, most places do not teach individuals how to check in often and make their stuff smaller. Some of them don't even realize merging side branche into side branch is a valid strategy to avoid merging incomplete features into the main branch(es). The seniors are not getting out of this scot-free when they barely make an effort to educate themselves, let alone others, or strategize ways to make this dummy-proof.
  - ar_lan 3y ago
    I agree the onus is to establish a clear culture, guidelines, and mentorship to newbies and junior developers in this regard. Almost everybody who started their career probably had their first code review torn to shreds. I'm not saying that's the right way to do it. But I will say, just like when a junior developer comes in thinking "well it works, so it's right", there are almost always other considerations than just the happy path. A many-file, massive review might be 100% technically correct and flawless, and it's still going to take reviewers a lot longer to review than if they split it into 2-3. -- > Merging side branch into side branch is a valid strategy Completely agree! This is a fantastic strategy. You're not affecting the main branch, and generally people reviewing that specific branch can have the context of what your change is doing.
sanderjd 3y ago
If the code review process is (through your supportive wisdom) pushing the people you work with to follow those practices, then I would argue it is actually a pretty good process. Those are all very good things to do, that a good code review culture should definitely be pushing people toward. For instance, let's imagine this without the code review process and your subsequent advice on how to make it go better: People would just be merging in the 20 file PRs, sight unseen, without validating a strategy beforehand, without really knowing how to approach the problem, with the code just written and slapped together without tweaking and refactoring, and without any self-review using a critical eye to catch low-hanging fruit like leaving in console.logs calls. I think the "flawed" code review process seems like a much better outcome!
- schwartzworld 3y ago
  The problem is that reviewing is itself a skill, and many people are bad at it.
withinboredom 3y ago
I always tell juniors to remember that it is their code until it gets merged in. Feel free to tell people “no” and why (“addressing” a comment doesn’t necessarily mean the code has to change). If the reviewer doesn’t like it, they can do it themselves after it gets merged in or create a PR to the PR. There are only a few cases where they can actually get blocked: leaking PII, security issues, and egregiously bad code (like writing something 5x times instead of using a loop). Anyway, if I’m reviewing and I see some improvements, I usually open a PR to the PR with my suggestions. I sometimes get halfway through it and realize why it is the way it is and never even leave a comment (and will defend the PR if someone else does a low-effort suggestion).
fasterik 3y ago
This is backwards and why I think mandatory code review is a waste of everyone's time. Nobody should be writing code with the goal of passing a code review. They should be writing code that solves a problem and adds value to the end product. If you're writing a system that requires 5000 lines of code, it's a waste of your time to figure out how to break it up into 5 or 10 PRs (which might actually make it harder to review, since chunking it up would remove context). I understand that code review has a place in enforcing code quality and training junior programmers. Here's what I would suggest. When you hire someone, make sure somebody is assigned to reviewing their code for the first few months. Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up. In addition, foster a culture where people are open to criticism and proactive about fixing things. If someone sees a problem with someone else's code, they can either let them know informally, or just go in and fix it themselves. I think this approach would lead to much less red tape and much happier and more productive programmers.
- schwartzworld 3y ago
  > Nobody should be writing code with the goal of passing a code review. It's not backwards. Code needs to be readable, well thought out, bug free. Code review (done well) helps establish these points. > If you're writing a system that requires 5000 lines of code, it's a waste of your time to figure out how to break it up into 5 or 10 PRs I guess if you think those 5000 lines are perfect that makes sense. Often big PRs in my experience have a lot of issues that the authors and other reviewers will have trouble catching. > Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up. Ha! I don't even trust myself not to screw things up.
- deleted 3y ago
  [deleted]
The_Colonel 3y ago
I disagree about smaller PRs being always better. I often find them more difficult to review if they contain only small part of the feature - I see something and ask - why is this like this? Oh, it's used like that in a subsequent PR... which then forces me to look up that second PR and review them together anyway. It also brings significant additional overhead - generally every merge to master has to be tested individually, since there's no guarantee all PRs will be merged before release goes out.

danparsonson 3y ago

Alternative view - if you approach a code review (or indeed any similar process) as a competition to be won or some kind of personal validation process, then you're likely to be disappointed and are potentially wasting an opportunity to learn something. Especially as a junior!

SilverBirch 3y ago

Totally agree it's flawed, but very often the reason it's flawed is because something has gone wrong somewhere else. Yes, it's very common for someone to do a PR and for someone else to turn around and go "Oh god, why the hell did you do it like this?!" - Fundamentally challenging the entire purpose of the code. What's happening there is the code review is just highlighting the fact you didn't have a design review. You also have to have a shared understanding of what's a reasonable request in a review, I've been in teams that relentlessly nitpick and teams that just "LGTM"s, eithers fine as long as you're agreed and consistent on what your team wants to do.

datavirtue 3y ago
[flagged]
- digging 3y ago
  Honestly, you sound like an asshole here. Do you realize that Adderall is a medication? Do you realize how fucked up it is to call someone mentally defective as if your brain meet some standard of perfect functioning? (it doesn't.)
  - datavirtue 3y ago
    No one cares after they tumble over the edge and make everyone miserable. It ain't helping anymore.
- thegrimmest 3y ago
  I dunno mate, I think adults should be able to handle being chortled at. You're responsible for your own emotional state. If you've done something laughable (or even if you haven't), then don't be surprised when people laugh. Taking it personally is optional and not recommended.
  - atq2119 3y ago
    I agree that people shouldn't be too thin-skinned, it's still important to stay civil and supportive. Being able to clearly and unambiguously convey that a change sucks while not hurting the other person's feeling is a useful skill to practice. (It's okay to fail at it, too, as long as you're aware that that's almost always a weakness.)
    - thegrimmest 3y ago
      The issue is the broad, ambiguous, definitions of things like "civil". In many cultures (eg. French, Russian) a direct, no-nonsense communication style is the default. You can just say things like "Your idea is hilariously stupid" and no one really gets bent about it. In the world of multicultural, international business, this seems like the most viable default, simply because it's the easiest to codify and communicate. Trying to teach very culturally-specific minutia of civility to a diverse staff just gets in the way. I'd rather work somewhere the burden of not being offended is placed on the listener, not the speaker. Somewhere we can just say what we think.

verall 3y ago

> smaller PRs I think the opposite works better - submit thousands of lines of changes, you will get like 10 comments, address all of them, and then you're gold. The reviewer has no time to slog through thousands of lines of changes unless the code you're touching is their baby.

kiitos 3y ago

You've probably heard this before, but it's worth repeating: code review doesn't have to be a "flawed process" that is "nit-picky" -- if it is, there's a problem in your organization that needs to be -- and absolutely can be -- fixed!

Shrezzing 3y ago

> Validate your strategy before building it. If there's a senior engineer in your team who nitpicks your code, get their buy-in before writing it. I think this hits the nail on the head. Code reviews have been adopted by most of the orgs I've worked at as a way to reduce any technical barrier to starting a project. Technical planning is moved from the start of a project to the end (or more accurately, to what has now become the middle). I'm certain there are code review processes that are done well, but they don't appear to be common.

ar_lan 3y ago

Everything you listed are exactly what the "know-it-all" senior developer is expecting already, though. If you're doing these things before you submit your review for eyes to look at, you'll come to a point where your reviews should only go through 0-2 revisions before submission is ready. Personally, if I see a review that is >10 files that isn't explicitly a "Refactor" review, or I've been prepped ahead of time, it's probably going to take a long time to get the review out, because there are so many things to iterate on. I also have to block out a lot of time to even do the singular review, because it is so long and there is so much cognitive load to carry with it. Smaller reviews are almost always better. If a review really can't be "completed" in a single PR, then I've also suggested 1/x reviews where bones are placed but gated under an FSS or something similar. This prevents code that shouldn't be run from being run until the whole feature (even if small) is complete, and lets the reviews focus on independent parts. -- Essentially, I'm saying I disagree with you placing the "error" on the senior developers in this scenario. Burdening someone with insanely large reviews is the err of the submitter.

BlargMcLarg 3y ago
Counterpoint, most places do not teach individuals how to check in often and make their stuff smaller. Some of them don't even realize merging side branche into side branch is a valid strategy to avoid merging incomplete features into the main branch(es). The seniors are not getting out of this scot-free when they barely make an effort to educate themselves, let alone others, or strategize ways to make this dummy-proof.
- ar_lan 3y ago
  I agree the onus is to establish a clear culture, guidelines, and mentorship to newbies and junior developers in this regard. Almost everybody who started their career probably had their first code review torn to shreds. I'm not saying that's the right way to do it. But I will say, just like when a junior developer comes in thinking "well it works, so it's right", there are almost always other considerations than just the happy path. A many-file, massive review might be 100% technically correct and flawless, and it's still going to take reviewers a lot longer to review than if they split it into 2-3. -- > Merging side branch into side branch is a valid strategy Completely agree! This is a fantastic strategy. You're not affecting the main branch, and generally people reviewing that specific branch can have the context of what your change is doing.

sanderjd 3y ago

If the code review process is (through your supportive wisdom) pushing the people you work with to follow those practices, then I would argue it is actually a pretty good process. Those are all very good things to do, that a good code review culture should definitely be pushing people toward. For instance, let's imagine this without the code review process and your subsequent advice on how to make it go better: People would just be merging in the 20 file PRs, sight unseen, without validating a strategy beforehand, without really knowing how to approach the problem, with the code just written and slapped together without tweaking and refactoring, and without any self-review using a critical eye to catch low-hanging fruit like leaving in console.logs calls. I think the "flawed" code review process seems like a much better outcome!

schwartzworld 3y ago
The problem is that reviewing is itself a skill, and many people are bad at it.

withinboredom 3y ago

I always tell juniors to remember that it is their code until it gets merged in. Feel free to tell people “no” and why (“addressing” a comment doesn’t necessarily mean the code has to change). If the reviewer doesn’t like it, they can do it themselves after it gets merged in or create a PR to the PR. There are only a few cases where they can actually get blocked: leaking PII, security issues, and egregiously bad code (like writing something 5x times instead of using a loop). Anyway, if I’m reviewing and I see some improvements, I usually open a PR to the PR with my suggestions. I sometimes get halfway through it and realize why it is the way it is and never even leave a comment (and will defend the PR if someone else does a low-effort suggestion).

fasterik 3y ago

This is backwards and why I think mandatory code review is a waste of everyone's time. Nobody should be writing code with the goal of passing a code review. They should be writing code that solves a problem and adds value to the end product. If you're writing a system that requires 5000 lines of code, it's a waste of your time to figure out how to break it up into 5 or 10 PRs (which might actually make it harder to review, since chunking it up would remove context). I understand that code review has a place in enforcing code quality and training junior programmers. Here's what I would suggest. When you hire someone, make sure somebody is assigned to reviewing their code for the first few months. Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up. In addition, foster a culture where people are open to criticism and proactive about fixing things. If someone sees a problem with someone else's code, they can either let them know informally, or just go in and fix it themselves. I think this approach would lead to much less red tape and much happier and more productive programmers.

schwartzworld 3y ago
> Nobody should be writing code with the goal of passing a code review. It's not backwards. Code needs to be readable, well thought out, bug free. Code review (done well) helps establish these points. > If you're writing a system that requires 5000 lines of code, it's a waste of your time to figure out how to break it up into 5 or 10 PRs I guess if you think those 5000 lines are perfect that makes sense. Often big PRs in my experience have a lot of issues that the authors and other reviewers will have trouble catching. > Once they've demonstrated their ability to write good code, from that point forward you trust them not to screw things up. Ha! I don't even trust myself not to screw things up.
deleted 3y ago
[deleted]

The_Colonel 3y ago

I disagree about smaller PRs being always better. I often find them more difficult to review if they contain only small part of the feature - I see something and ask - why is this like this? Oh, it's used like that in a subsequent PR... which then forces me to look up that second PR and review them together anyway. It also brings significant additional overhead - generally every merge to master has to be tested individually, since there's no guarantee all PRs will be merged before release goes out.