5 ms·

Absolutely not. This is a recipe for disaster and a terrible example to set for any upcoming engineers that haven’t had much industry experience yet. Code revi

by geuis 5y ago

Absolutely not. This is a recipe for disaster and a terrible example to set for any upcoming engineers that haven’t had much industry experience yet. Code reviews help me be more confident in my own work. Even if 90% of the time it’s fine, there’s always that 10% where a second pair of eyes catches a mistake or offers a suggestion that makes the code even better.

mdaniel 5y ago
> offers a suggestion that makes the code even better plus, software engineering is a team sport, so if something works but is opaque, that's a fine thing to point out in a review, otherwise one runs the risk of being The "Foo Component" Owner™ and that's usually no fun and is for sure not healthy for any reasonably sized org I am also a monster fan of the "Apply Suggestion" button in GitLab, which allows me to meet the reviewer half-way by even doing the legwork for the suggested change. If they agree, push button and we're back on track. It's suboptimal for any process that requires/encourages signed commits, but damn handy otherwise
klabb3 5y ago
Meh. If 90% of the time it's a mindless ritual there is probably a better way to achieve the same goal. Code reviews are a huge time sink, especially for unimportant style- and naming nits. Code review as mentoring can be great, if it's a directed 1:1 effort. Usually, it is not.
- watwut 5y ago
  Hours long discussion about whether to call it "thing" or "thingId". True story.
- LaserDiscMan 5y ago
  As well as a time sink, code review can be detrimental if it's used as a KPI gaming mechanism or for passive aggressive one-upmanship.
- marcinzm 5y ago
  >Code reviews are a huge time sink, especially for unimportant style- and naming nits. This isn't a problem with code reviews, this is a problem with your team's processes that code review has highlighted. These problems will show up in others ways if not during code reviews since clearly there's strong disagreement on coding styles and conventions. So go and fix or implement the style guides, linters and so on. This is like saying that the way to fix fevers is to get rid of thermometers.
  - tuyiown 5y ago
    > So go and fix or implement the style guides How do you make this converge across dev without endless debate or resentment ? > linters and so on. What happens when a rule has to change ? update the entire codebase impacting everyone with conflicts ? tolerate divergence existing code ? Talking about thermometers, are you actually tracking fevers, or merely minor bad breath ?
    - abstract_put 5y ago
      I think there's a lot of grey area for what "style" is, but e.g. formatting and what not is pretty easy and not-contentious in my experience. Most people want consistency more than they want their way. How I did it as part of a ~35 person dev team was adopt one of the popular ones from the community (e.g. Google's) - treat it as a benevolent dictator. Apply it to the entire code base in one giant shot (disrupting any pull requests that were in flight but fixing them proactively), then enforce it with tooling. Leave the door open to anyone who disagrees with the defaults. It's on them to propose the change with rationale then put it to a quick majority vote with a subset of the team. I think 2 things have actually changed since the very beginning (line length from 80 -> 120 and something else that I don't remember right now). When something changes, making a sweeping change is part of accepting it - if it's too risky/disruptive then it has to be very valuable to do.
  - etchalon 5y ago
    There's a large number of things in any decently complicated system that a "style guide" can't cover and that there are dozens of potential ways to solve.
watwut 5y ago
> there’s always that 10% where a second pair of eyes catches a mistake or offers a suggestion that makes the code even better. Testing is pretty awesome process to find bugs.
- Clubber 5y ago
  QA departments are expensive. Execs in their infinite wisdom opted to dump that task on developers who earn 3x as much as QA people. Synergies!
  - marcosdumay 5y ago
    > who earn 3x as much as QA people And never learned how to properly test software... Or earn 10x as much and consider it not a priority.
  - seattle_spring 5y ago
    As a dev, I'd much rather do my own testing then have to explain extremely basic shit to the average SDET I've worked with. No, Chuck, the 404 for the favicon on page A did not cause the padding to be off on page B.
    - Clubber 5y ago
      You've had some bad experiences with QA people. The people I've worked with are methodical and work like the terminator to find bugs. One guy was so good he could even write queries against QA to show me where the data was inserting / updating improperly. A good QA person is worth their weight in gold. In my experience, it's hard to have a dev as a good tester. I'm ok at it but not as good as a pro QA person. Good devs look for shortcuts and efficiencies, which is the opposite mentality for good testing.

by geuis 5y ago

mdaniel 5y ago

> offers a suggestion that makes the code even better plus, software engineering is a team sport, so if something works but is opaque, that's a fine thing to point out in a review, otherwise one runs the risk of being The "Foo Component" Owner™ and that's usually no fun and is for sure not healthy for any reasonably sized org I am also a monster fan of the "Apply Suggestion" button in GitLab, which allows me to meet the reviewer half-way by even doing the legwork for the suggested change. If they agree, push button and we're back on track. It's suboptimal for any process that requires/encourages signed commits, but damn handy otherwise

klabb3 5y ago

Meh. If 90% of the time it's a mindless ritual there is probably a better way to achieve the same goal. Code reviews are a huge time sink, especially for unimportant style- and naming nits. Code review as mentoring can be great, if it's a directed 1:1 effort. Usually, it is not.

watwut 5y ago
Hours long discussion about whether to call it "thing" or "thingId". True story.
LaserDiscMan 5y ago
As well as a time sink, code review can be detrimental if it's used as a KPI gaming mechanism or for passive aggressive one-upmanship.
marcinzm 5y ago
>Code reviews are a huge time sink, especially for unimportant style- and naming nits. This isn't a problem with code reviews, this is a problem with your team's processes that code review has highlighted. These problems will show up in others ways if not during code reviews since clearly there's strong disagreement on coding styles and conventions. So go and fix or implement the style guides, linters and so on. This is like saying that the way to fix fevers is to get rid of thermometers.
- tuyiown 5y ago
  > So go and fix or implement the style guides How do you make this converge across dev without endless debate or resentment ? > linters and so on. What happens when a rule has to change ? update the entire codebase impacting everyone with conflicts ? tolerate divergence existing code ? Talking about thermometers, are you actually tracking fevers, or merely minor bad breath ?
  - abstract_put 5y ago
    I think there's a lot of grey area for what "style" is, but e.g. formatting and what not is pretty easy and not-contentious in my experience. Most people want consistency more than they want their way. How I did it as part of a ~35 person dev team was adopt one of the popular ones from the community (e.g. Google's) - treat it as a benevolent dictator. Apply it to the entire code base in one giant shot (disrupting any pull requests that were in flight but fixing them proactively), then enforce it with tooling. Leave the door open to anyone who disagrees with the defaults. It's on them to propose the change with rationale then put it to a quick majority vote with a subset of the team. I think 2 things have actually changed since the very beginning (line length from 80 -> 120 and something else that I don't remember right now). When something changes, making a sweeping change is part of accepting it - if it's too risky/disruptive then it has to be very valuable to do.
- etchalon 5y ago
  There's a large number of things in any decently complicated system that a "style guide" can't cover and that there are dozens of potential ways to solve.

watwut 5y ago

> there’s always that 10% where a second pair of eyes catches a mistake or offers a suggestion that makes the code even better. Testing is pretty awesome process to find bugs.

Clubber 5y ago
QA departments are expensive. Execs in their infinite wisdom opted to dump that task on developers who earn 3x as much as QA people. Synergies!
- marcosdumay 5y ago
  > who earn 3x as much as QA people And never learned how to properly test software... Or earn 10x as much and consider it not a priority.
- seattle_spring 5y ago
  As a dev, I'd much rather do my own testing then have to explain extremely basic shit to the average SDET I've worked with. No, Chuck, the 404 for the favicon on page A did not cause the padding to be off on page B.
  - Clubber 5y ago
    You've had some bad experiences with QA people. The people I've worked with are methodical and work like the terminator to find bugs. One guy was so good he could even write queries against QA to show me where the data was inserting / updating improperly. A good QA person is worth their weight in gold. In my experience, it's hard to have a dev as a good tester. I'm ok at it but not as good as a pro QA person. Good devs look for shortcuts and efficiencies, which is the opposite mentality for good testing.