Hacker News Reader

Comment by notjustanymike | Hacker News Reader

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!

View on HN

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...

> 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.

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.

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

Also nit:

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:'

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.

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.

I always make sure that author can resolve my comments. If they can't do that, the pr is not approved - because I would like them to address something. If I approve it, then each of my comments should be resolvable.

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.)

The best is when the only possible reviewer is your Engineering Manager because you have no other engineers in your domain on the team.

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.

That's lucky to have a manager who even logs into GitHub. Many jobs of mine I have been my own approver.

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.

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/ 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.

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.

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.

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.

Usually no one is looking for design feedback, but plenty of offers....

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.

For anybody else curious (maybe this was just new to me), the abbreviation CL (used all over the place in the linked docs but never actually defined anywhere I could find) stands for change log, or change list, near as I can tell. Am I the only one out of the loop here?

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.

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.

I have an open PR from my first week. Not a single review. I’ve merged entire features since then…

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.

It’s a dev environment improvement so I pinged the whole team. Nothing like a chilly start at a new company eh?

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.

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.

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.

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:

Do you work at the same place I do?

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.

Stable? Maybe. Maintainable in a business-continuity sense? Possible but unlikely.

Code review is an essential part of shipping sound software.

More time wasting busywork of dubious value.

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.

Reply on news.ycombinator.com