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