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.<p>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.<p>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.<p>Have you ever found yourself in such a position? What do you think about code review? Can too much of it be wrong?
I work in such an environment. My advice is to:<p>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.<p>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.<p>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"
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”<p>I lasted 4 months before resigning but I took longer to get my confidence back.
My suggestion is try and get feedback earlier.<p>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.<p>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.<p>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.<p>Maybe it is just time to move to the next job since your learning curve is flattening?
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.
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.
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.<p>It's more of a culture problem.<p>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.<p>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
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.<p>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.
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.
Some code reviewers are just not qualified to review, just as some interviewers aren't.<p>Code reviewers are often not trained in what they should be looking for. Sometimes they are badly trained.<p>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.<p>If management can't handle it, you could do what I did, let projects slide and interview around. Let them hold the bag.
There are different levels of suggestions, and if your team don't have them you could work on introducing them.<p>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.<p>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.<p>Sometimes, if there is a lot of back and forth, it is easier to set pair programming session and address everything in one session.<p>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.
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."<p>... applies to code reviewers.<p>I like Google's review standard:<p>"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."<p><a href="https://google.github.io/eng-practices/review/reviewer/standard.html" rel="nofollow">https://google.github.io/eng-practices/review/reviewer/stand...</a>
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.
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.<p>The grass is always greener.
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.
> 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<p>This sounds like the real problem, more than anything.<p>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.<p>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.<p>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.
It's a case of the perfect being the enemy of the good.<p>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.
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.
I don't really mind fixing my typos in comments and such.<p>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.
>- The focus of reviews is not on correctness but stuff like naming, docstrings, and design.<p>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?
Leave. Life is too short and your profession is in too high a demand.<p>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.
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.
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.