101–110 of 302 posts
Worth noting that Google engineers had this level of satisfaction long before the mostly useless AI suggestions came on the scene. Those additions are comparatively recent.
Source: me, C++ readability reviewer - this means I review code from lots of different people, all over the codebase.
Earlier quoted context omitted.
To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message a…
> I have Github connected to Slack; if someone needs my review I get a message and can have it done within 5 minutes. Do you not do anything else? If I'm working on something else, I'm not getting to any new task within 5m (at least, not without a net loss of productivity to reload context)
Usually, the code review is the very last step before value becomes available to the business. There is absolutely no reason to delay a code review in this case, none, whatsoever.
Even if it isn't the last step, you're delaying the chance for a developer to continue working. More often than not, when a PR is submitted, there is a period where the submitter is twiddling their proverbial thumbs to prevent context switching from a reviewer.
So, delaying a review prevents value from being available and reduces overall developer productivity for the entire team.
Or, keep working on making an imaginary idea into a real one while someone else's already built idea sits there wasting money.
Earlier quoted context omitted.
Good point. Making renames a suggestion, not gating (unless it contradicts the name in some design doc somewhere that another team is relying on not to move) may be the balance point there.
But "another team is relying on not to move" is an objective point, right? Things like "I find this code very hard to follow, and I think it could be made easier" is also objective, and even "I don't understand what this variable name means, and I think it could be clearer". I once names a function mkdir(). This created a directory tree. In the review it was called "obscure" so it became createDir(). Then someone poi…
The "I" in that sentence suggests this should be considered subjective. And that's I think the cleave-point between gating and non-gating: "Other people have already agreed on this" vs. "In the moment, I, a single code-reviewer, think this name could be improved."
Critique is nice but the ML suggested edits are a waste of attention and cant be disabled.
> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…
I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is: Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow”…
I think the word you are looking for is "hazing":
> haze (v): force (a new or potential recruit to the military or a university fraternity) to perform strenuous, humiliating, or dangerous tasks.
I had this at my current job. I almost quit because it was downright humiliating to get called an "idiot" in so many nice words. I don't do well with hazing... so I brought out quotes from text books, papers, and famous authors to point out how wrong they were.
I think it's funny that devs will go through all kinds of untold suffering when it comes to code reviews...stacked pull requests, waiting days (or more!) for a review, all kinds of nitpicky BS that is a lot of time and effort to rework by the time you have the code all written. But suggest to people that they pair program and that it's a real-time review that obviates the need for formal async review and folks want t…
I wonder if there are studies on the effectiveness of pair programming vs code review and bugs caught. Anecdotally, I catch more bugs doing code review than pairing. When pair programming, we naturally start to think similar thoughts, so it doesn’t get a great real second set of fresh eyes. Even when I have pair, I like to do an alone-time code review and things almost always pop up.
Whenever folks ask me this, I want to turn it around: are there studies on the effectiveness of working solo vs pair programming?
> Even when I have pair, I like to do an alone-time code review and things almost always pop up.
Agree - even with code that's been mostly paired on it can be helpful to let it sit and get additional review.
In my experience, the killer combo for pairing is to combine it with test-first programming. The tests help create a shared construct for discussion and understanding.
I freely admit I have a very strong pro-pairing and pro-test-first bias. But in my defense it's based on almost two decades of experience.
example 1: - a engineer caused multi-million dollar loss in ad revenue due to change which passed review with flying colors. and he is the only engineer whose name ever put in a post mortem document. - same engineer few months before this incident forced a junior engineer to waste 2 months by making him to stupid nit picks. after a month of addressing nit picks, the junior engineer showed a slight resistance. then this engineer removed himself from the review list (but this engineer is the only one who is not manager in that code owner list) and made the junior engineer to beg on his knee to provide approval
example 2: - a engineer in my team in double click ad always puts tons of nit picks. he got easily promoted.the justification gives but the leadership that the engineer created extremes value. the leadership was from double-click acquired by google few years ago and they are extremely in-competent.
example 3: - there is a tool in google which analyses code review comments and those reviewers are very argumentative are given badge that they are very throw in review and I have seen those stupid badges counted during promotions.
> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…
Not doing it every time, will come back to haunt me later.
Most of the times, the reply on my comment for a more obvious variable name is ( not: ) is: yeah, I thought about a better name but couldn't find any or something is found in between.
Ofc, sometimes the change is so ugly I'll feel bad because they should rewrite it. I'm still struggling with it.
Some people just can't code too. These will come back to haunt us.
And will have bugs too...
Earlier quoted context omitted.
Badly written, or badly designed? Bad code, to me, would be an unrolled for-loop, or a for-loop where a foreach loop would do better (though now, we're getting in the realm of nit-picking). A PR is waaaaay too late to bring up architectural/design improvements (unless it is a WIP PR opened expressly for discussing the approach). I very rarely see bad code in PR's unless it is from a Junior programmer, and even then,…
> A PR is waaaaay too late to bring up No. That’s literally the point of a review.
For starters, it results in a waste of time for literally everyone involved:
1. The person writing has to rewrite it (probably).
2. The reviewer could have sat down with the person before a single line of code was written.
3. Anyone else reviewing just wasted their time because it will be rewritten.
If you see code badly designed, walk over to, or call the developer and say "hey, can we discuss the design of the code."
The saying "measure twice, cut once" doesn't just apply to wood. Design before you ever write any code.
Earlier quoted context omitted.
> [...] while also mashing the "approve" button +1 - It is easy to forget that it is possible to approve with comments. Not all review feedback should block a merge.
It is a merge block in some code bases. You need to know the code the person said they were going to commit is the code they actually commit. Especially when there is financial incentive and state actors that want code inserted. In my project we used to be allowed to approve with nits but recently they changed it that the code needs a re-review for almost any edits. The system has some criteria for which it will allo…
- Modify it and accept that this will trigger re-review
- Merge as-is... maybe they don't even agree with the nit
- Merge as-is and followup with a new MR under the same JIRA. This sometimes makes sense if it gets them into an important preprod environment, but they are really still working on the issue.
- Merge as-is and follow up with a JIRA on the backlog. Maybe they realized that the "nit" is really a bigger issue and should be addressed separately.
Regardless, there's value in not blocking the merge for non-functional issues.