5 ms·

So many missed opportunities for a small business. A code review should never be done in person; instead all communication should happen asynchronously through

So many missed opportunities for a small business. A code review should never be done in person; instead all communication should happen asynchronously through passive aggressive messages left in GitHub. If you're on bi-weekly sprints, the review should be left to age for a minimum of 5 business days before any feedback is provided. Never write resolvable comments, but rather leave ambiguous musings of how a block of code could be cleaner. Assign the review to multiple engineers but never make it a formal part of their responsibilities. On special occasions, unpredictably merge a review with no feedback, or do so simply based on your mood. Loosely, and unpredictably, enforce documentation requirements. Most important: never update your product managers on merged features, so that they can experience the joy of discovery just like your customers!

rabbits_2002 3y ago
Do you work at the same place I do?
adhesive_wombat 3y ago
Bonus if the vague comments relate to some refactoring that you'd like to do, the details of which exist only in your head. Meanwhile, yes, you know the existing code is like X, and has been like that for years, but obviously I shouldn't have followed it and it's all wrong. For details on how I'd like it to be, please read your mind. Add an ellipsis to really drive home that only will any more explanations not be forthcoming but it should be evident that this is my problem...
- asu_thomas 3y ago
  > Bonus if the vague comments relate to some refactoring that you'd like to do I am extremely ashamed to say I have left a comment like this before. At the time, I was frustrated about never being able to discuss anything over a call. There is no excuse, though.
  - bornfreddy 3y ago
    To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
    - bluefirebrand 3y ago
      I think "fyi" comments are valuable in PRs. Often all I want is for my team to approach similar problems differently in the future, not necessarily to refactor the immediate code. Or at the very least consider alternative approaches
      - desi_ninja 3y ago
        Also nit:
        bluefirebrand 3y ago
        Yes, for sure. Although I much prefer to add automatic code formatting and linting and stuff to codebases to dramatically reduce the occurrence of 'nit:'
      - WorldMaker 3y ago
        Also such "fyi" comments can be a great way to get the refactoring discussion outside of just your own head. It can be a good start to boiling down the reasons for your refactor and getting external buy-in/validation that you are thinking down a useful path to the entire team and not just your own ego.
    - nielsole 3y ago
      I often add multiple comments and then approve the pr. My main question is always: does this make the codebase overall better or worse for some definition of good? If that bar is met, iterative improvements should not block deployment/merge. This way the value of the pr process is captured: preventing obviously bad changes from reaching prod and two-way knowledge sharing. Setting a higher bar for reviews more often than not blocks people for days without good reason.
fuzzy2 3y ago
What, those are rookie numbers! I opened a pull request 13 business days ago. I even rebased it multiple times when conflicting code was otherwise merged. There’s been no feedback whatsoever. Everyone even remotely involved is now on vacation. (It’s not like I care either, the project sucks anyway.)
- kxrm 3y ago
  The best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.
  - Maxion 3y ago
    The best is when you are the technical lead with admin rights, but there's no one on the team senior enough to actually want to review your code. You leave itfor two weeks when the person who should be PO, but is never in any meetings, doesn't look at the tickets or the pull requests, and only reachable by email calls you asking where the feature is.
  - hobs 3y ago
    That's lucky to have a manager who even logs into GitHub. Many jobs of mine I have been my own approver.
    - kxrm 3y ago
      I think it is a huge assumption to believe my Engineering Manager even has time to review anything within the next 2 weeks. Every time I have hit this at my company I have had a minimum wait of almost 3 weeks on average. However, yea, against just approving your own code, this is better.
pak9rabid 3y ago
And in the case of merge conflicts, steamroll your changes over everyone else's, as you know what's best and everybody else is wrong.
- kiitos 3y ago
  Merge conflicts to the main branch? I've never even heard of an org where it's OK to force-push to main. That's wild! And a huge red flag, that ought to be fixed at priority zero. Wow.
  - fuzzy2 3y ago
    Ah, but it wouldn’t be something as inelegant as a force push. No no. You do merge. It’s just that the outcome is slightly… biased.
    - kiitos 3y ago
      OK, so we're talking about allowing PR merges without a +1 from a reviewer? That's less-bad, but still a red flag :grimacing:
agentultra 3y ago
Minimum 5 days hurt. Too real. If there’s one way to destroy productivity and morale it’s to leave a vague comment on a review and not come back to it for a few days.
- cjfd 3y ago
  5 days is very quick. I have an open pull request that has been open for 6 months or thereabouts.... :-( No, I am not suggesting this as a good practice.... Actually, I would say that a pull request should be reviewed in 24 hours. Pull requests made on Friday may wait until Monday.
  - withinboredom 3y ago
    I have an open PR from my first week. Not a single review. I’ve merged entire features since then…
    - polishdude20 3y ago
      Do you specifically ping people to review? At my company we've got a habit of not only assigning reviewers in GitHub but also pinging them in slack.
      - withinboredom 3y ago
        It’s a dev environment improvement so I pinged the whole team. Nothing like a chilly start at a new company eh?
duxup 3y ago
Code reviews seem like a hellscape of "could do X". Like man it's code, there's an infinite amount of "could do" with an infinite amount of contexts. I found google's advice to be pretty good https://google.github.io/eng-practices/review/ https://google.github.io/eng-practices/review/ while they give a lot of good advice / suggestions, they also make a point that there aren't a lot of hard stops and generally if the code works and isn't horrendous you let it go.
- s__s 3y ago
  If the code works let it go? That’s an incredibly low bar to set. Unless you’re working on an all-star team, that approach will quickly get your codebase in a really bad place with incredible amounts of technical debt.
  - duxup 3y ago
    I think you have to read their process to be clear. They don't want to hold up things over style quibbles and etc. It is something they're ok with talking about, but not every aspect that could be improved is a hard stop.
- polishdude20 3y ago
  I feel like if you're looking for design feedback during the code review that's already way too late. There should be much more communication going on before the coding even happens.
  - JKCalhoun 3y ago
    Usually no one is looking for design feedback, but plenty of offers....
  - duxup 3y ago
    Yeah. I think a nice little "Hey I saw this that doesn't match this design doc here, but this all looks good regardless." type reminder would be ok. Obviously being diplomatic / soft skills is huge and the google doc addresses that a great deal. But yeah especially if this is a non-trival change / lotta work ... not time to re-invent the wheel.
JKCalhoun 3y ago
I never knew a code review until maybe the last 5 or 6 years of my career. Clearly I never shipped stable code in the first two decades.
- kiitos 3y ago
  Stable? Maybe. Maintainable in a business-continuity sense? Possible but unlikely. Code review is an essential part of shipping sound software.
  - deleted 3y ago
    [deleted]
- stcroixx 3y ago
  More time wasting busywork of dubious value.
  - JKCalhoun 3y ago
    Most of the changes I have seen in software engineering over my career have not been for the engineers but have been in service of a nervous management that are desperate for numbers and reins.

rabbits_2002 3y ago

Do you work at the same place I do?

adhesive_wombat 3y ago

Bonus if the vague comments relate to some refactoring that you'd like to do, the details of which exist only in your head. Meanwhile, yes, you know the existing code is like X, and has been like that for years, but obviously I shouldn't have followed it and it's all wrong. For details on how I'd like it to be, please read your mind. Add an ellipsis to really drive home that only will any more explanations not be forthcoming but it should be evident that this is my problem...

asu_thomas 3y ago
> Bonus if the vague comments relate to some refactoring that you'd like to do I am extremely ashamed to say I have left a comment like this before. At the time, I was frustrated about never being able to discuss anything over a call. There is no excuse, though.
- bornfreddy 3y ago
  To be fair, it's not always a bad idea to present such ideas. However you need to make the comment resolvable, or better yet, create an issue for refactoring, add a small "fyi" comment and let them resolve it when they read it. This way they are aware of possible upcoming changes to this code but the MR/PR is not blocked.
  - bluefirebrand 3y ago
    I think "fyi" comments are valuable in PRs. Often all I want is for my team to approach similar problems differently in the future, not necessarily to refactor the immediate code. Or at the very least consider alternative approaches
    - desi_ninja 3y ago
      Also nit:
      - bluefirebrand 3y ago
        Yes, for sure. Although I much prefer to add automatic code formatting and linting and stuff to codebases to dramatically reduce the occurrence of 'nit:'
    - WorldMaker 3y ago
      Also such "fyi" comments can be a great way to get the refactoring discussion outside of just your own head. It can be a good start to boiling down the reasons for your refactor and getting external buy-in/validation that you are thinking down a useful path to the entire team and not just your own ego.
  - nielsole 3y ago
    I often add multiple comments and then approve the pr. My main question is always: does this make the codebase overall better or worse for some definition of good? If that bar is met, iterative improvements should not block deployment/merge. This way the value of the pr process is captured: preventing obviously bad changes from reaching prod and two-way knowledge sharing. Setting a higher bar for reviews more often than not blocks people for days without good reason.

fuzzy2 3y ago

What, those are rookie numbers! I opened a pull request 13 business days ago. I even rebased it multiple times when conflicting code was otherwise merged. There’s been no feedback whatsoever. Everyone even remotely involved is now on vacation. (It’s not like I care either, the project sucks anyway.)

kxrm 3y ago
The best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.
- Maxion 3y ago
  The best is when you are the technical lead with admin rights, but there's no one on the team senior enough to actually want to review your code. You leave itfor two weeks when the person who should be PO, but is never in any meetings, doesn't look at the tickets or the pull requests, and only reachable by email calls you asking where the feature is.
- hobs 3y ago
  That's lucky to have a manager who even logs into GitHub. Many jobs of mine I have been my own approver.
  - kxrm 3y ago
    I think it is a huge assumption to believe my Engineering Manager even has time to review anything within the next 2 weeks. Every time I have hit this at my company I have had a minimum wait of almost 3 weeks on average. However, yea, against just approving your own code, this is better.

pak9rabid 3y ago

And in the case of merge conflicts, steamroll your changes over everyone else's, as you know what's best and everybody else is wrong.

kiitos 3y ago
Merge conflicts to the main branch? I've never even heard of an org where it's OK to force-push to main. That's wild! And a huge red flag, that ought to be fixed at priority zero. Wow.
- fuzzy2 3y ago
  Ah, but it wouldn’t be something as inelegant as a force push. No no. You do merge. It’s just that the outcome is slightly… biased.
  - kiitos 3y ago
    OK, so we're talking about allowing PR merges without a +1 from a reviewer? That's less-bad, but still a red flag :grimacing:

agentultra 3y ago

Minimum 5 days hurt. Too real. If there’s one way to destroy productivity and morale it’s to leave a vague comment on a review and not come back to it for a few days.

cjfd 3y ago
5 days is very quick. I have an open pull request that has been open for 6 months or thereabouts.... :-( No, I am not suggesting this as a good practice.... Actually, I would say that a pull request should be reviewed in 24 hours. Pull requests made on Friday may wait until Monday.
- withinboredom 3y ago
  I have an open PR from my first week. Not a single review. I’ve merged entire features since then…
  - polishdude20 3y ago
    Do you specifically ping people to review? At my company we've got a habit of not only assigning reviewers in GitHub but also pinging them in slack.
    - withinboredom 3y ago
      It’s a dev environment improvement so I pinged the whole team. Nothing like a chilly start at a new company eh?

duxup 3y ago

Code reviews seem like a hellscape of "could do X". Like man it's code, there's an infinite amount of "could do" with an infinite amount of contexts. I found google's advice to be pretty good https://google.github.io/eng-practices/review/ https://google.github.io/eng-practices/review/ while they give a lot of good advice / suggestions, they also make a point that there aren't a lot of hard stops and generally if the code works and isn't horrendous you let it go.

s__s 3y ago
If the code works let it go? That’s an incredibly low bar to set. Unless you’re working on an all-star team, that approach will quickly get your codebase in a really bad place with incredible amounts of technical debt.
- duxup 3y ago
  I think you have to read their process to be clear. They don't want to hold up things over style quibbles and etc. It is something they're ok with talking about, but not every aspect that could be improved is a hard stop.
polishdude20 3y ago
I feel like if you're looking for design feedback during the code review that's already way too late. There should be much more communication going on before the coding even happens.
- JKCalhoun 3y ago
  Usually no one is looking for design feedback, but plenty of offers....
- duxup 3y ago
  Yeah. I think a nice little "Hey I saw this that doesn't match this design doc here, but this all looks good regardless." type reminder would be ok. Obviously being diplomatic / soft skills is huge and the google doc addresses that a great deal. But yeah especially if this is a non-trival change / lotta work ... not time to re-invent the wheel.

JKCalhoun 3y ago

I never knew a code review until maybe the last 5 or 6 years of my career. Clearly I never shipped stable code in the first two decades.

kiitos 3y ago
Stable? Maybe. Maintainable in a business-continuity sense? Possible but unlikely. Code review is an essential part of shipping sound software.
- deleted 3y ago
  [deleted]
stcroixx 3y ago
More time wasting busywork of dubious value.
- JKCalhoun 3y ago
  Most of the changes I have seen in software engineering over my career have not been for the engineers but have been in service of a nervous management that are desperate for numbers and reins.