It's hard to work with people who see the code review space as a battleground. I think the problem is not the overdoing but the problem may be the purpose.
Ask HN: I'm tired of intense code review cycles
31–40 of 55 posts
Re: Ask HN: I'm tired of intense code review cycles
#32I had a Lead like this. It go to the point where I would spend a significant portion of my working time in existential dread about submitting a PR because I knew they would just rip it to pieces. The worst part was small styling issues that would have been picked up by any linter - but the lead refused any requests for the team to use linting because “we have never used it and everything works fine” I lasted 4 months…
Agreed styling code comments are asinine. Teams need a common linter to leave it to the CI to determine the style, whatever it may be. It’s a waste of everyone’s time.
I always prefix such comments with "nit" so the owner of the PR knows I'm not saying they should/must make this change to get a +1 from me but that I think it should be considered.
That said we already use auto linters and formatters.
I am more mindful of my code review comments now that I'm more senior. When it's easy for people to take my words as gospel I much prefer to foster an environment where that isn't the case.
Re: Ask HN: I'm tired of intense code review cycles
#33Earlier quoted context omitted.
Indeed, it might be time to move on. Or at least talk to the reviewer that bothers me. Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough. I'm usually okay with all of these suggestions. Still, lately, I'm fell in a positio…
> Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough. This pedantry is a cancer. If your PR comment starts with "Nit:" just shut up and go do something useful.
I don't think so.
Nit comments are how I indicate areas where I would have done something different but I don't feel strongly enough to block your PR over it. Could be some formatting that I think could increase readability or some code you didn't touch in your PR but if you have time while you are in this file you could improve while you are there.
I always stress these don't block PRs and for the most part they are subjective. I still find it valuable to share these perspectives as they offer gentle ways to impart the smaller parts of "good taste" that make programmers well versed in such things a pleasure to work with.
Re: Ask HN: I'm tired of intense code review cycles
#34Earlier quoted context omitted.
> Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough. This pedantry is a cancer. If your PR comment starts with "Nit:" just shut up and go do something useful.
I don't think so. Nit comments are how I indicate areas where I would have done something different but I don't feel strongly enough to block your PR over it. Could be some formatting that I think could increase readability or some code you didn't touch in your PR but if you have time while you are in this file you could improve while you are there. I always stress these don't block PRs and for the most part they are…
"Good taste" is just your opinion. Framing it this way makes it sound canonical, which it is not. We all have opinions on what good taste is, and they all differ. To see this, all you have to do is go read the codebases of all your dependencies. They all taste different. Which is my point. These comments are useless noise.
Re: Ask HN: I'm tired of intense code review cycles
#35I think it helps if a team considers everything to be temporary, and code reviews are more for understanding how things work, with some some captured discussion of how it could be re-done. The author should merge, declare victory, and move on, while someone else re-writes it to their liking. It shouldn’t be considered done until it’s been re-done twice by separate authors. One thing I’ve realized about software “engi…
Especially for non-production code, setting the bar to 'is it a net improvement?' makes code review quite a bit shorter. When it has become 'is it perfect?' then nothing gets done anymore. For re-factorings, do one kind of small change but do it over the whole code-base.
Re: Ask HN: I'm tired of intense code review cycles
#36I had a Lead like this. It go to the point where I would spend a significant portion of my working time in existential dread about submitting a PR because I knew they would just rip it to pieces. The worst part was small styling issues that would have been picked up by any linter - but the lead refused any requests for the team to use linting because “we have never used it and everything works fine” I lasted 4 months…
Your lead was a junior.
One of the reasons I always fought to have black from almost the day it existed was because (I thought) it would put an end to this type of petty human linting.
Re: Ask HN: I'm tired of intense code review cycles
#37I work in such an environment. My advice is to:
1. push for tooling such as linters, formatters, static analysis etc. which reduces ambiguity and discussion based on personal preferences. These tools also help prevent any nitpicking comments.
2. write code to the best of your abilities without thinking about reviews and ask for review early. I find that most people have a strong need to say something on reviews, even if the code is already acceptable, so that they can feel that they are adding some value. Asking for review on an unpolished version of the code helps such people fulfill their need which allows them not to nitpick later on. Doing things this way also allows your coworkers to pull the brakes early if you started going down the wrong path.
3. It is ok to disagree, so if you are in a senior position and have confidence in your skill and domain knowledge, using these words can help you get a lot of stuff done quickly when time is critical: "i see your point, but so lets just disagree and commit to this"
Re: Ask HN: I'm tired of intense code review cycles
#38>- The focus of reviews is not on correctness but stuff like naming, docstrings, and design. I agree that focusing on naming/docstrings doesn't make much sense, but design feels like it's fair game. What type of "design" stuff is showing up in your reviews?
Indeed! Design suggestions are fair game, but they are quite rare nowadays and their quality has been dwindling.
The last one I got felt too arbitrary: I decided to improve the behavior of an unused feature from one of our core libraries to unblock a colleague working in the application. The change was straightforward and it took me 1hr to get it into a PR. The feedback was that we should keep the old behavior available, just in case. So I spent a full afternoon wrangling C++ templates to fit in a new API that offered both the improved and old behavior for which there is no requirement.
What started as a small detour from my regular work to help a fellow engineer ended up costing me a day.
In the end, I told my colleague that he would have to take over the PR as I had lost too much time on it. Sure enough, he had to change all parameter names cause the types already describe them well enough: `foo(Timeout connection_timeout)` -> `foo(Timeout connection)`.
The resulting code looks great, but now it is clear that I have a more pragmatic style. I recognize that I simply failed to stand my ground, and I'll take that into account.
Re: Ask HN: I'm tired of intense code review cycles
#39Earlier quoted context omitted.
Indeed, it might be time to move on. Or at least talk to the reviewer that bothers me. Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough. I'm usually okay with all of these suggestions. Still, lately, I'm fell in a positio…
> Although I push for a lot of upfront agreement, most of the back and forth arises from little things, like a variable name being too long, a newline between struct members, or a docstring that could be removed because the code is expressive enough. This pedantry is a cancer. If your PR comment starts with "Nit:" just shut up and go do something useful.
> This pedantry is a cancer. If your PR comment starts with "Nit:"
I have worked in companies where sloppyness was culturally acceptable. I found that a much worse cancer because there is no fixed line when sloppyness is cosmetic and when it affects code quality and maintainabilty. Code quality just did deteriorate over time as little things started to accumulate everywhere.
Yes, review discussions are tedious, but having an ever deteriorating code-base is worse.
Re: Ask HN: I'm tired of intense code review cycles
#40I don't really mind fixing my typos in comments and such.
But in really hairy code I often don't get feedback about real bugs. Well, it took me a lot of time and domain expertise to write it. The reviewer uses less time and has less deep domain knowledge of that detail in question.