7 ms·

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship (<<< keyword) is maintainable, tested, and documented as far as any gotchas or hacks go. Ideally we want to improve the code base as well, especially when dealing with legacy code. I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leave any comments or get any on my PRs for them since we are all on the same wavelength and for the most part know what we are doing. A junior engineer is a… junior engineer. They are going to have a lot of comments telling them how to write better code and a good suggestion will include a code suggestion and links to read. It’s how they will get better, and for bigger tickets or super juniors, I sometimes pair with them to go through their ticket together. At a certain level you just know how to identify patterns and understand how to build good software. You read books like Clean Architecture or Designing Data Intensive Applications to level up. Juniors tend to just write yolo shit code or add more shit to a legacy shitcode repo instead of trying to improve it. They’ll add to the mess instead of writing a new clean layer on top to which we can easily maintain and modify. But that’s why you have to show them. The only thing that helps people get better is time, being humble, and being genuinely interested themselves in becoming better at their craft. I remember my junior engineer days, and seeing my PRs filled with comments. But if I didn’t have those I don’t think I’d have leveled up so fast.

doutunlined 4y ago
Your comment implies juniors are only juniors for a temporary amount of time. I've worked with many SWEs (some with years of FAANG experience) who consistently code like juniors. They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. Consistently doing this over months erodes the quality of a codebase.
- deleted 4y ago
  [deleted]
- simplotek 4y ago
  > They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. That says nothing without context. If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit someone's architectural tastes. It makes no sense to criticize someone for focusing where it matters the most and actually deliver critical features without delay if the tradeoff is some legacy debt, and that by no mean implies they're junior. One important skill of a senior is being able to keep their eyes on the prize and be smart about tradeoffs. Prioritizing subjective opinions over software architecture over delivering value is more in line with a junior way of thinking about software than not, and to me doesn't sound like a position to criticize others.
  - bcrosby95 4y ago
    I'm gonna call bullshit on all of this. The time tax on not leaking layers in an application that doesn't already leak layers is nearly zero. The problem is people don't want to expend the very minor extra amount of effort. So you do it once. Then the next person does it because hey we already do it there so the code is not even subjectively worse, it's objectively the same, cuz you already crossed that line. After a while you have a giant festering pile of shit that you just keep making bigger and bigger and now people are talking about rewriting it. In my experience you start actively losing velocity in this situation very quickly. If you're doing this when you still your have 3+ months on a project, you're likely making poor decisions. I'm not saying you need to constantly gold plate your architecture. But it should be palatable. The subjective part is what is palatable. But I don't know anyone who thinks steaming turdpiles are palatable.
    - turdprincess 4y ago
      In my experience, codebases never maintain their level of quality. Unless you are taking deliberate action to improve the codebase with every commit, it's actively getting worse like you described above. The interesting thing is that this isn't a technical problem, it's a human problem. Most folks want to clock in, do their job and clock out, and going the extra mile to improve things isn't worth the effort. I think its the same human tendency that leaves so much pee on the toilet floor - some humans will clean up their own, but almost none will clean up someone else's.
Zagill 4y ago
I've always had a pretty positive attitude towards code reviews (still a junior dev), because I know there's probably something I missed or a technique I'm not familiar with or some language quirk I didn't know about. If I submit a PR with a bunch of new functionality, I'm going to be way more concerned if I don't get a handful of comments on it than if I do.
- alwaysbeconsing 4y ago
  Frankly I think everyone should have this attitude, seniors as well. (I try to) I am experienced but I know I write bugs and code that may be not as clear as it could be. We all do: we're only human. Also depending on the codebase (and language to some degree), if someone senior writes a lot of overcomplex or abstract code that the rest of the team can't understand/maintain, that's just as much a problem as anything else.
  - deleted 4y ago
    [deleted]
  - jvans 4y ago
    > if someone senior writes a lot of overcomplex or abstract code For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works. Any tips for giving feedback to senior people here? A lot of the complexity from abstraction is very hard to quantify and experienced people can have arguments that sound reasonable.
    - moomoo11 4y ago
      Is the abstraction so that they can easily swap out pieces of the system without a single headache, or write effective tests, or to make it easy to operate in a soup of services? Because then it makes complete sense. Like dependency injection is something that a lot of juniors struggle to understand. Or in larger applications DDD and all the crap that goes along with it.
rerdavies 4y ago
I most recently worked on a 100% code-review team. Fabulous! In my personal experience, the hump from wide-eyed novice junior to productive contributing member really doesn't take that long when every line that everyone writes is code-reviewed. The first couple of code reviews might take longer than usual. But it definitely takes much less time than it used to take to bring a junior up to speed. No net productivity in the first year used to be a rule, when bringing juniors onboard in the dark ages of computing. With 100% code reviews, that process seems to take no more than a couple of months -- maybe even as little as one month. The difference is dramatic. And it's a fabulous way to transmit not only good culture, but also love of our craft.
- mplanchard 4y ago
  What do you mean by 100% code review?
  - bdangubic 4y ago
    Every line of code must be code reviewed before it goes to stage/prod
    - xxpor 4y ago
      … how else do people operate? Complete yolo?
      - Aeolun 4y ago
        Maybe rubber stamp PR? We can say we review everything 100%, but practically this sometimes just means clicking the approve button.
      - rerdavies 4y ago
        lol. I've been doing this for a long time. It wasn't always done that way.
      - ornornor 4y ago
        You’d be surprised how many places operate that way. Or better, commit to master via the web ui because “it’s a quick small fix”
nonethewiser 4y ago
This sounds like you and your peers don't really read and take the time to understand what changes are in the PR.
cramjabsyn 4y ago
> I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback. I’m looking for basics. Will it work? is the code clear? is it commented/documented? is the approach simple and maintainable? Some submitters really need coaching on how to receive feedback, and how to plan their timelines to incorporate revisions.
- Aeolun 4y ago
  To be fair, some people are just pedantic. Just because you are the reviewer doesn’t mean things have to be done exactly the way you say. If I never accept anything from more junior people because it isn’t perfect, how are they ever going to stay motivated? I aim for one or at most two rounds of review, because anything else is just depressing.
  - cramjabsyn 4y ago
    Ehhh far from perfection, more like “please use the native feature that handles this in 2 LOC instead of shelling out to an external 100 line script”
    - Aeolun 4y ago
      There’s limits and degrees to anything. I guess I never get PR’s that are quite that boneheaded.
  - interactivecode 4y ago
    100% agree, code review should be there to check if the code doesn’t contain any obvious security risks or mistakes. Designing features, architecture and learning should happen before someone even gets to the point of submitting a PR. Plus if it can be improved on in follow PRs then do that instead of blocking juniors with pedantics
- pjmorris 4y ago
rendaw 4y ago
Unless the submitter is stuck or asks for it, I avoid giving recommended changes on code reviews and instead just explain what (bad) consequences pieces of code have. Providing recommendations takes away autonomy from the person who wants to contribute, and it's easy to end up glossing over the real consequences and ending up with "well, I would have done it this way" nit picking reviews.
deleted 4y ago
[deleted]

doutunlined 4y ago

Your comment implies juniors are only juniors for a temporary amount of time. I've worked with many SWEs (some with years of FAANG experience) who consistently code like juniors. They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. Consistently doing this over months erodes the quality of a codebase.

deleted 4y ago
[deleted]
simplotek 4y ago
> They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. That says nothing without context. If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit someone's architectural tastes. It makes no sense to criticize someone for focusing where it matters the most and actually deliver critical features without delay if the tradeoff is some legacy debt, and that by no mean implies they're junior. One important skill of a senior is being able to keep their eyes on the prize and be smart about tradeoffs. Prioritizing subjective opinions over software architecture over delivering value is more in line with a junior way of thinking about software than not, and to me doesn't sound like a position to criticize others.
- bcrosby95 4y ago
  I'm gonna call bullshit on all of this. The time tax on not leaking layers in an application that doesn't already leak layers is nearly zero. The problem is people don't want to expend the very minor extra amount of effort. So you do it once. Then the next person does it because hey we already do it there so the code is not even subjectively worse, it's objectively the same, cuz you already crossed that line. After a while you have a giant festering pile of shit that you just keep making bigger and bigger and now people are talking about rewriting it. In my experience you start actively losing velocity in this situation very quickly. If you're doing this when you still your have 3+ months on a project, you're likely making poor decisions. I'm not saying you need to constantly gold plate your architecture. But it should be palatable. The subjective part is what is palatable. But I don't know anyone who thinks steaming turdpiles are palatable.
  - turdprincess 4y ago
    In my experience, codebases never maintain their level of quality. Unless you are taking deliberate action to improve the codebase with every commit, it's actively getting worse like you described above. The interesting thing is that this isn't a technical problem, it's a human problem. Most folks want to clock in, do their job and clock out, and going the extra mile to improve things isn't worth the effort. I think its the same human tendency that leaves so much pee on the toilet floor - some humans will clean up their own, but almost none will clean up someone else's.

Zagill 4y ago

I've always had a pretty positive attitude towards code reviews (still a junior dev), because I know there's probably something I missed or a technique I'm not familiar with or some language quirk I didn't know about. If I submit a PR with a bunch of new functionality, I'm going to be way more concerned if I don't get a handful of comments on it than if I do.

alwaysbeconsing 4y ago
Frankly I think everyone should have this attitude, seniors as well. (I try to) I am experienced but I know I write bugs and code that may be not as clear as it could be. We all do: we're only human. Also depending on the codebase (and language to some degree), if someone senior writes a lot of overcomplex or abstract code that the rest of the team can't understand/maintain, that's just as much a problem as anything else.
- deleted 4y ago
  [deleted]
- jvans 4y ago
  > if someone senior writes a lot of overcomplex or abstract code For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works. Any tips for giving feedback to senior people here? A lot of the complexity from abstraction is very hard to quantify and experienced people can have arguments that sound reasonable.
  - moomoo11 4y ago
    Is the abstraction so that they can easily swap out pieces of the system without a single headache, or write effective tests, or to make it easy to operate in a soup of services? Because then it makes complete sense. Like dependency injection is something that a lot of juniors struggle to understand. Or in larger applications DDD and all the crap that goes along with it.

rerdavies 4y ago

I most recently worked on a 100% code-review team. Fabulous! In my personal experience, the hump from wide-eyed novice junior to productive contributing member really doesn't take that long when every line that everyone writes is code-reviewed. The first couple of code reviews might take longer than usual. But it definitely takes much less time than it used to take to bring a junior up to speed. No net productivity in the first year used to be a rule, when bringing juniors onboard in the dark ages of computing. With 100% code reviews, that process seems to take no more than a couple of months -- maybe even as little as one month. The difference is dramatic. And it's a fabulous way to transmit not only good culture, but also love of our craft.

mplanchard 4y ago
What do you mean by 100% code review?
- bdangubic 4y ago
  Every line of code must be code reviewed before it goes to stage/prod
  - xxpor 4y ago
    … how else do people operate? Complete yolo?
    - Aeolun 4y ago
      Maybe rubber stamp PR? We can say we review everything 100%, but practically this sometimes just means clicking the approve button.
    - rerdavies 4y ago
      lol. I've been doing this for a long time. It wasn't always done that way.
    - ornornor 4y ago
      You’d be surprised how many places operate that way. Or better, commit to master via the web ui because “it’s a quick small fix”

nonethewiser 4y ago

This sounds like you and your peers don't really read and take the time to understand what changes are in the PR.

cramjabsyn 4y ago

> I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback. I’m looking for basics. Will it work? is the code clear? is it commented/documented? is the approach simple and maintainable? Some submitters really need coaching on how to receive feedback, and how to plan their timelines to incorporate revisions.

Aeolun 4y ago
To be fair, some people are just pedantic. Just because you are the reviewer doesn’t mean things have to be done exactly the way you say. If I never accept anything from more junior people because it isn’t perfect, how are they ever going to stay motivated? I aim for one or at most two rounds of review, because anything else is just depressing.
- cramjabsyn 4y ago
  Ehhh far from perfection, more like “please use the native feature that handles this in 2 LOC instead of shelling out to an external 100 line script”
  - Aeolun 4y ago
    There’s limits and degrees to anything. I guess I never get PR’s that are quite that boneheaded.
- interactivecode 4y ago
  100% agree, code review should be there to check if the code doesn’t contain any obvious security risks or mistakes. Designing features, architecture and learning should happen before someone even gets to the point of submitting a PR. Plus if it can be improved on in follow PRs then do that instead of blocking juniors with pedantics
pjmorris 4y ago

rendaw 4y ago

Unless the submitter is stuck or asks for it, I avoid giving recommended changes on code reviews and instead just explain what (bad) consequences pieces of code have. Providing recommendations takes away autonomy from the person who wants to contribute, and it's easy to end up glossing over the real consequences and ending up with "well, I would have done it this way" nit picking reviews.

deleted 4y ago

[deleted]