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

#1

Post by krimbus » Sat, Dec 18, 2021, 12:45 AM UTC

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?

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

#2

Post by osivertsson » Sat, Dec 18, 2021, 1:11 AM UTC

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?

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

#3

Post by brianolson » Sat, Dec 18, 2021, 1:19 AM UTC

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.

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

#4

Post by krimbus » Sat, Dec 18, 2021, 2:03 AM UTC

osivertsson wrote: post #2

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 wi…

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.

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

#5

Post by greatpostman » Sat, Dec 18, 2021, 2:07 AM UTC

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.

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

#6

Post by fivelessminutes » Sat, Dec 18, 2021, 2:20 AM UTC

krimbus wrote: post #4

osivertsson wrote: post #2

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 wi…

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…

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

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

#7

Post by ss108 » Sat, Dec 18, 2021, 2:37 AM UTC

krimbus wrote: post #4

osivertsson wrote: post #2

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 wi…

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…

> 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

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

#8

Post by nine_zeros » Sat, Dec 18, 2021, 3:41 AM UTC

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.

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

#9

Post by Ancalagon » Sat, Dec 18, 2021, 3:41 AM UTC

greatpostman wrote: post #5

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.

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

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

#10

Post by siquick » Sat, Dec 18, 2021, 3:42 AM UTC

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.