Ask HN: I'm tired of intense code review cycles

61 points by krimbus 4 years ago | 51 comments

I work on a small software team that develops code for scientific instruments. Our team runs very efficiently, and the focus on code quality is a big part of our culture.

However, lately, I have noticed that I'm unhappy with the intense code review cycles. The features work, have test coverage, and good git history, but we end up with a lot of back and forth until everything is precisely how our architect wants it to be. My perception is that I reached a point where I spend 50% of the time developing a solution (since I have enough architectural and domain knowledge) and the other 50% figuring out what will fly on review and merging back.

I do have some conflicting points in mind: - My skills have evolved a lot in this process, and I take pride in delivering good-quality code. But now that the learning curve is flattening, I feel the weight of not having enough creative freedom to write something up quickly or try different designs. - I see a lot of value in the shared ownership from the lengthy reviews, and we usually end up with better code/design as the suggestions are technically sound. - I'm starting to feel like code is implementation detail since our high code standard does not necessarily add more value to users. - The focus of reviews is not on correctness but stuff like naming, docstrings, and design. Whenever a colleague makes a suggestion that helps the code improve, I'm happy to oblige. But lately, I wish I could slip a docstring that explains what is done instead of the why. - In essence, we follow open-source library development standards while making a closed source application. - The nature of our projects requires big additions to the software, so it is very common to have chained PRs.

Have you ever found yourself in such a position? What do you think about code review? Can too much of it be wrong?

lpapez 4 years ago |

I 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 <reason for dismissal goes here> so lets just disagree and commit to this"

pg_1234 4 years ago | |

You missed the key comment "the learning curve is flattening" in tech this means it is time to move on. There is no point trying to redeem a system you have outgrown, unless that is your official responsibility.

siquick 4 years ago |

I 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 before resigning but I took longer to get my confidence back.

pydry 4 years ago | |

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.

peakaboo 4 years ago | | |

I have to disagree with the view that not using black means the lead is not senior.

Being senior means you understand the tradeoffs and you make your own decision.

If you ask a senior why he choose not to use kubernetes, you will get a bunch of real reasons. If you ask a junior the same, he won't have a good answer because he lacks experience with the pros and cons.

bracketslash 4 years ago | | |

what is “black” in this context?

bewestphal 4 years ago | |

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.

jpgvm 4 years ago | | |

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.

tossaway9000 4 years ago | |

I don't know why someone would want to waste time arguing about code style.

Standardizing on things like `cargo fmt`, `go fmt`, and `terraform fmt` remove a ton of nitpicking out the gate. The javascript world can't seem to make up their mind though (jslint is rarely used these days, jshint died, I think eslint is the thing now?)

Hasnep 4 years ago | | |

Eslint is a linter, the most common formatter for JS that I've seen is Prettier.

osivertsson 4 years ago |

My suggestion is try and get feedback earlier.

Whiteboard with your coworkers and architect what you are building once you have a pretty good grasp on how you want to solve a task. You probably have some code at this stage to feel pretty confident that it will work.

Agree on naming of concepts and design at this point, allowing you to change direction without reworking too much. Of course further changes to design will occur as you learn more during implementation. If they are major run them by your team again to keep them in the loop.

It is probably true that you could work faster, at least for a while, if reviews were more slack. That the company chooses to prioritize less risk instead.

Maybe it is just time to move to the next job since your learning curve is flattening?

krimbus 4 years ago | |

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 position where usually the person who reviews my code is our architect (an amazingly talented engineer). He always flags all the minutiae (very politely, of course), and I find it very hard not to comply as I want to ship better stuff.

Funnily enough, a few days ago, I found myself reviewing a colleague's PR and repeating the same behavior. As soon as I saw myself doing it, I deleted these comments, made the suggestions that I judged valuable, and approved.

Thanks for the input! Just replying gave me a lot of clarity on the subject.

fivelessminutes 4 years ago | | |

Sounds like the reviewer realized he enjoys exercising power over others and feeling respected, and that metastatized into pedantry.

ss108 4 years ago | | |

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

Jeez

patrick451 4 years ago | | |

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

brianolson 4 years ago |

Reviewer needs to lighten up. I'm the senior software engineer on a lot of reviews lately and I see a lot of things I wish were different -- but they're not important. I won't let _bad_ style stay, but if it's just personal preference and their way isn't actually _wrong_, I should let it go.

greatpostman 4 years ago |

Some of this is a senior engineer justifying his own job. One time I copied code almost verbatim from another highly regarded team. Senior engineer ripped it, claiming it wasn’t at standard for the company. Then I told him it was from team x. A lot of this is bravado and a dog and pony show. You need code quality, but the important things are at the software architecture level/abstractions.

Ancalagon 4 years ago | |

That’s hilarious, what did the senior engineer say?

krallja 4 years ago | | |

DRY

mindhash 4 years ago |

I had a similar experience when I joined a team. The architect was highly opinionated and wanted things to be done in certain way. The problem however wasn't with him. The management would usually pin all technical debt and issues on this architect. He was supposed to be answerable for anything that goes wrong. Which made him a control freak. He wanted to know anything and everything in detail.

It's more of a culture problem.

When I realized this, I started sympathizing a bit with him. Made sure he understood what I was trying to achieve and comfortable with the code base. He was reluctant to accept new ideas but I pitched them anyway.

This is the nature of the job I realized. And it brought peace to me. I did however consider new opportunities with more freedom. But the controlling nature of the team did not bother me

jl2718 4 years ago |

I 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 “engineering” is that it’s layered thick with opinions. If you force your opinions on someone, they check out and you get nothing of value from them. It’s probably better just to let them do things their way, and find the place where they can add the most. Some architects think their job is to walk around the beach kicking sand castles.

Fronzie 4 years ago | |

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.

meeeu 4 years ago |

It’s honestly a win-win. You’re getting paid to get better. It’s on their dime and not yours. The inverse of the situation is that you’re expected to churn out shitty code and it’s never fast enough. Thing break constantly and you never learn how to craft quality code. I’m in the same boat where I’m expecting the PR process to be rigorous. And at the end of the day I can tell the difference between the correct and elegant solution and what’s halved assed but gets it done. At the end of the day I want to be a craftsman where the art of the craft is clean correct software solution. You’re lucky to be in a position where code quality trumps all.

nine_zeros 4 years ago |

Some code reviewers are just not qualified to review, just as some interviewers aren't.

Code reviewers are often not trained in what they should be looking for. Sometimes they are badly trained.

Ultimately this is a management problem. Record the time wasting back and forth and ask your management what they prefer. A skilled manager will separate your project from the reviewer and coach the reviewer in how to be an effective reviewer.

If management can't handle it, you could do what I did, let projects slide and interview around. Let them hold the bag.

galkk 4 years ago |

There are different levels of suggestions, and if your team don't have them you could work on introducing them.

On my current job, one of guidelines says that if something is not important/very personal preference, you can prefix the comment with words "nit:" that means "nitpicking", and author can either change if it sees fit, or just acknowledge comment and go on.

Other thing is not to be shy to tell that suggested change is outside of the scope of this changeset, and will/may/might be addressed later.

Sometimes, if there is a lot of back and forth, it is easier to set pair programming session and address everything in one session.

Last option is raise that with management, if you can justify your opinion that such loop doesn't bring the value for the time spent on it, especially if you know that there are other engineers in the team who feel the same.

aredplug 4 years ago |

I believe a variant of: "It is a well-known fact that those people who must want to rule people are, ipso facto, those least suited to do it... anyone who is capable of getting themselves made President should on no account be allowed to do the job."

... applies to code reviewers.

I like Google's review standard:

"In general, reviewers should favor approving a [PR] once it is in a state where it definitely improves the overall code health of the system being worked on, even if the [PR] isn’t perfect."

https://google.github.io/eng-practices/review/reviewer/stand...

Osiris 4 years ago | |

I often approve a PR with the comment "Approved with comments".

In other words, I didn't notice any bugs but I have some suggestions. I'll let you decide if you want to make the changes or not.

aredplug 4 years ago | | |

+1. The job of reviewer on shared codebases is advisory, not as a gatekeeper.

If people choose to ignore comments, then that's a behavioral issue worth escalating.

un_montagnard 4 years ago | | |

I've been in teams where those comments are never addressed because the pressure is on "push something that roughly solves the task so you can show something during demo".

Tyriar 4 years ago |

I used to care a lot more about code style, now when I get those feelings while reviewing I'll instead invest that effort into setting up a lint rule so it's the last time I need to make the comment.

voakbasda 4 years ago |

Be grateful you have a review process and people willing to review your changes. I am working in a Wild West, so I would love to have that kind of rigor.

The grass is always greener.

mypalmike 4 years ago |

My unpopular opinion is that code reviews tend to provide negative net value. They take a lot of time, rarely catch structural problems, and lower morale. For this, you generally get consistent braces and spaces, and you might catch some detail that would raise obvious alarms in your pre-prod environment.

MathYouF 4 years ago |

> But now that the learning curve is flattening, I feel the weight of not having enough creative freedom to write something up quickly or try different designs

This sounds like the real problem, more than anything.

Perhaps it's a sign that the code reviews have done their job, and you've been trained well enough to become a code reviewer yourself.

It might benefit you to start asking for more authority to be discussing these new design ideas with whoever is in charge, and see what they have to say.

If you really think you know well enough to architect things on your own or improve architectural decisions, you should assert that belief and ask for opportunities to test it.

floydian10 4 years ago |

It's a case of the perfect being the enemy of the good.

Is it possible that there's not too much work to do? In my experience, this happens when the reviewer has too much time on their hands. Parkinson's law comes to mind.

redsh 4 years ago |

The only positive thing I see about them is that it forces people to read code and get some understanding of the project they’re working on. But in general, we should rebel against code reviews - they are one of the things that make programming a corporate chore and not something fun. 99% of the time is spent on useless nitpicks and the rest of the 1% can be done with more testing or design sessions.

usr1106 4 years ago |

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

gruez 4 years ago |

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

krimbus 4 years ago | |

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.

tharkun__ 4 years ago | | |

I read all of the comments here with much interest. Thanks for putting in this specific example.

I would love to have a chat with your architect. If you (or he) gave me a PR with

    foo(Timeout connection)

I would ask him to improve the naming. That thing is not a connection. In the places this variable will be used it makes no sense to use a connection. Take this example usage, potentially way down in the implementation of foo. Let's say an imaginary send function that takes the data and a timeout).

    send(data, connection)

If I read this code I will think that it takes data and a connection. But it doesn't. It takes data and a timeout. I have to know or look up the type of connection to understand what is happening.

Instead your original

    foo(Timeout connection_timeout)

Will make absolute perfect sense without knowing or having to look up the type. The following code can be read without having to stop and cross reference information. What you see is what you get. Less cognitive load, which is awesome. You put in thought to make awesome readable code once and every subsequent reader for all eternity can enjoy being able to just read and immediately understand what your code does and what everything is.

    send(data, connection_timeout)

fmakunbound 4 years ago |

Leave. Life is too short and your profession is in too high a demand.

However, make sure management knows it’s the architect’s micromanaging/senseless pedantry that is the problem. If you’re feeling dread about creating a PR, others are also.

actually_a_dog 4 years ago |

You haven't mentioned anything about disliking code reviews other than the way this architect conducts them. It sounds more like you're tired of micromanagement than code reviews.

pictur 4 years ago |

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.