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?
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"
I lasted 4 months before resigning but I took longer to get my confidence back.
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.
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.
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.
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?)
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?
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.
Jeez
This pedantry is a cancer. If your PR comment starts with "Nit:" just shut up and go do something useful.
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
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.
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.
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.
... 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...
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.
If people choose to ignore comments, then that's a behavioral issue worth escalating.
The grass is always greener.
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.
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.
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.
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?
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.
I would love to have a chat with your architect. If you (or he) gave me a PR with
foo(Timeout connection)
send(data, connection)
Instead your original
foo(Timeout connection_timeout)
send(data, connection_timeout)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.