How can I defend reducing the strength of code reviews?

Question

I have started in a new team. I have 20 years experience as a developer, and I have been in the role of a team lead in several projects.

Normally I am very much pro code reviews, but I ended up in a team that use TDD up to religious fundamentalism. Mostly this is led by a single senior resource, me being the second senior. The result is that they have implemented a code review process that requires approval for merge. Not only that it requires approval, but it also requires each individual comment being responded. All that is very nice until you start getting pull requests that can not be approved in days with tens of comments each.

In addition, when requests are done, the team does not focus on what is IMHO important (patterns, interfaces, encapsulation, layering, and method signatures), but on small details.

Example: There is a code convention that methods doing things logically connected should be in close proximity to each other. But then if you actually require that the methods must be ordered by their chronological execution, that goes a bit further than the general rule.

No one is just reasoning that if we, in the first place, did not have 50 methods in the same class, the positioning of the methods would possibly not matter that much.

Code is just full of examples where developers just go in the nitty picky details instead of focusing on the general problem.

Such a heavy code review process in my opinion creates a hostile atmosphere where a newcomer feels simply bad.

How can I justify and defend the thesis that:

1. The merge button should be enabled by default. IMO after some iterations, if the team is conscious about quality code and if someone is non-cooperative, the team will kick him/her out.
2. The code review should be a recommendation, but not mandatory. I believe we are all grownups and it is natural to follow good advice. Again, if someone is stupid enough to not follow, in time the team will kick him/her out.
3. The code author should have the right to merge the code within six hours, let’s say, of the pull request creation no matter if there is approval or not.

Answer 1

How can I justify and defend the thesis that:

1. The merge button should be enabled by default
2. The code review should be a recommendation , but not mandatory
3. The code author should have the right to merge the code within 6 hours lets say of the pull request creation no matter if there is aproval or not.

I don't think you should try and justify any of those, because they are almost certainly bad ideas. Code review is just about the one thing that has been consistently shown to significantly improve code quality, and you're effectively proposing to stop doing it.

Instead, put your efforts into improving your code review process:

the team does not focus on what is important(patterns, interfaces, encapsulation, layering, method signature) but on small details.

This is the problem you need to fix. Work with your team to improve their abilities, both in writing code and reviewing it. Then you'll have changed a bad process into a good one.

Oh, and never, ever use language like "religious fundamentalism" when discussing this. I hope I don't need to explain why.

Answer 2

How do the company's stakeholders feel about the productivity of their developers?

My previous boss told me a story once about this kind of zealotry. The developer team was using that zealotry as an excuse to drag their feet. As a result, it took months to implement the simplest of changes. He said that fixing the problem would have required firing half of the development staff, so after a few months, he moved on.

But there's always two sides to every story. You haven't said much about the nature of the application under development. If this is a mission-critical application that involves the potential loss of life (e.g. a medical device) or the handling of millions of dollars in assets, that level of rigor may be entirely justified.

Where I work, every software project goes through a "characterization" process. Based on that characterization ("does it control a fighter plane, or is it just a command-line utility"), each project undergoes a different level of scrutiny. Formal reviews are costly; that cost has to be justified by the nature of the software project.

In any case, as the new team member, you will encounter significant resistance to your ideas, especially if the team has been operating this way for a long time without anyone challenging their point of view. You will have to establish trust with your team members, do things their way for awhile, and gradually build your case for change.

Further Reading
Parkinson's Law of Triviality

Answer 3

You bring up some real problems that need fixing. But your proposed solutions are unlikely to make things better.

No-one is just reasoning that if we in first place did not had 50 methods in same class possibly it would not have mattered that much the positioning of the methods.

In my experience the best way to get people to see "the better way" is to jump in and code it. Once you have a good example don't force it into the code base. Invite people to review it with you. Let them see the benefits themselves. I've completely reworked classes other people created and then humbly asked their opinion. At first they show some resistance but if you show that you take them seriously enough to learn from them they calm down and start learning from you as well.

What helps me the most is I go find people to look at my code while it's fresh in my brain. While I'm still willing to rewrite it. I sit and their desk, or they sit at mine (and play with my desk toys). We spend time coding together. E-mail only code reviews tend to devolve into nothing more than brain dead check-the-box inspections. They cause the nit picking because no one feels like they can get you to really change the code so all they want is to make it look like they reviewed it. Which is true because these usually happen days after you finish and have started other work. Those are time wasters. Sorry but that's too late for a good code review. Nothing you do with your merge tools will fix it.

Being willing to learn from each other is the fundamental requirement for collaborative work. Otherwise it's design by committee.

How can I justify and defend the thesis that:

1. The merge button should be enabled by default

You should have some kind of branch that you can merge to as you please that everyone can see. This doesn't have to be the release branch.

2. The code review should be a recommendation , but not mandatory

Mandatory code reviews != Formal code reviews. Just get someone to look at your code before you go think about something else.

3. The code author should have the right to merge the code within 6 hours lets say of the pull request creation no matter if there is aproval or not.

I'm all for getting it done within 6 hours (that's a long time for this) but unreviewed code needs to be stopped somewhere. I'd rather completely dismantle the code review checklist than leave things unreviewed.

Answer 4

Making code reviews optional cannot be justified. The desire to make reviews optional stems from your frustration with the current code review process. And I cannot blame you.

When doing a code review I look for 3 things (in this order):

1. Obvious bugs — if I can't find a defect at first glance, I move on.

2. Proper architecture and design — I probably spend the most time here.

3. Style issues that prevent me from identifying issues in points 1 and 2.

It sounds like people are focusing more on #3 than 1 or 2.

Since this is a new team, you will need to take time to learn their way of writing code. If people consistently point out that methods should be grouped by functionality, make a mental note about that for the future. For changes that fall into the #3 category, it will be a learning process for you.

But learning by trial and error is frustrating. The team should have a code style guide you can refer to. If they do not, then as a senior you should start writing one. Put it some place so other team members can read it. The next time someone brings up a stylistic issue, refer them to the style guide. If you find people requesting conflicting changes, bring this up with the team so they can all agree upon the style.

It may take some time to learn or write a style guide, but it pays off when the team writes code more consistently and spends less time nitpicking. They can all focus more on items 1 and 2 in my list above, which seems like the items you would rather focus on.

There is a code convention that methods doing things logically connected should be in close proximity to each other.

This is certainly an opinion-based rule. Each rule in your style guide should provide justification. Perhaps this rule came about because some of your teammates view their monitors oriented vertically. Grouping methods in this manner allows people to view all of those methods in one screen. This should be the justification for that rule in the style guide.

But then if you actually require that the methods must be ordered by their chronological execution, that goes a bit further than the general rule.

Ask people why this is desirable. If this is necessary because the program flow is difficult to follow, then restructuring the code to make it easier to follow might be the answer. Maybe adding some class or sequence diagrams as a visual aid for certain use cases would actually be more beneficial. Do this instead of adding a rule to the style guide. As a senior developer, don't be afraid to suggest these sorts of non-code solutions to what appears to be style problems.

A style guide does not need to be a novel. Start by hitting the main things people nitpick about. Consider it a work in progress. Each time a rule is brought up, ask why it is desirable, and phrase your question from the standpoint that you are just trying to learn how they do things. Then leverage your 20 years of experience to identify alternate solutions to the myriad style rules, which could include selective documentation of components, additional tools, or even standardizing how your existing tools are configured.

Your two decades of experience does not just include writing code. It should give you a wider view of the process and tools your team uses. This knowledge can be used to help trim the fat from your code reviews by reducing the number of style rules that must be followed. A style guide gives everyone a chance to weigh in and provides a single point of reference when differences in opinions come up.

Then the entire team can focus more on the architecture.

Answer 5

It's worth reminding code reviews have fantastic outcomes when they are done correctly. I remind myself being mostly rubber-stamping reviews until I found code that could make our production crash, or was simply not conform to expected result. Catching it in the review saved us precious time.

I understand that the way you implemented them raises several problems and I have several propositions to address that:

nitty picky details instead of focusing on the general problem
pull requests that can not be approved in days
hostile atmosphere where a newcomer feels simply bad

If a PR don't get accepted because of details, ask yourself first, is that a business issue ? I suppose it depends from PR to PR. Development missions are rarely time-critical though, and in many context additional couple of days of delay on a PR is acceptable. You could imagine passing the review as part of the development process and account for it.

If that delay is not acceptable, or you see room for improvement, there are several ways to accelerate your review process without entailing too much on the goals:

• More flexible code standards
• Tooling that enable to pass code standard prior to review
• Notify systematically new review/review acceptation/review rejection
• Possibility for reviewer to amend PR, and approve the amended PR, when suggested changes are small enough

These changes has to be decided with the agreement of the whole team. If you suggest some of these process changes, that for most are reasonable, you would have a higher chance of success than tearing down the review process entirely (which making it optional does).

You pointed out a hostile environment also. It's worth reminding reviews should:

• Never point things (naming, design etc.) as bad, poor, dirty etc. but rather not conforming to the guidelines you should have agreed on and written somewhere.
• Include propositions when things don't pass
• Occasionally ask for opinions and/or insights to the reviewee.
• When necessary (when it's complex) be done with the reviewee.

Within this framework, even I who didn't like reviews have come to agree systematic reviews are an excellent thing to have.

Answer 6

First of all, I agree with Philip Kendall that none of the changes that you say you want to make are actually desirable. While bypassing the review process by making it optional might increase productivity in the short term, in the long run it's going to result in more bugs, messy and inconsistent code and other kinds of technical debt — precisely what code review is meant to prevent.

Also, to be frank, I'd suggest starting by letting go your ego and honestly considering whether at least a small part of the problem might lie with you. It's certainly possible that the code review process in your team is objectively painful and inefficient — but it's also possible that it's not that bad, and that you're simple making it painful by struggling against it and insisting on having things done your way.

In particular, based on what you wrote, I'm wondering if just maybe your long experience as a developer and as a team lead might be making you subconsciously reluctant to adapt to the practices of your new team and to accept suggestions from others with less "seniority", even if it's actually good (or at least not objectively bad) advice. Frankly, a lot of what you wrote sounds a lot like the (unfortunately all too common) case of a senior developer moving to a new team with a different culture and development practices, but insisting on doing things the way they're used to because "it's better and I'm senior so I know better". Which could even be objectively true in some cases, but fails to account for the not insignificant value of having the entire team be on the same wavelength and following the same practices, even if they're not perfectly optimal.

---

Fortunately, there's an effective (though not always easy) way to check for and deal with such issues: when you join a new team with an unfamiliar culture, open your mind, let go your pride and pretend you're a new junior developer for a while. Let the other members of your team teach you their way of working, even if you feel it's stupid and wrong and cannot possibly work. Accept all their corrections and advice, do things their way even if it feels inefficient. Learn to understand how they do things, and why they do it that way.

In a few months, you should be able to adapt to and begin to understand the new culture you've found yourself in. At that point, if you still feel that things could be improved, is when you should start offering suggestions. But you can't improve a process without first understanding the process, just like you can't really fix buggy code without first understanding what it's trying to do.

Also, just to be clear, even during the "adaptation phase" you don't have to turn off your brain, and your experience can still be a valuable asset. If you think that something could perhaps be done better, offer it as a suggestion. If you notice that a change suggested in code review would introduce a bug, point it out. But don't push your way of doing things, and certainly don't try to lean on your experience and seniority to intimidate others into accepting your suggestions. In this phase, you're the one who's trying to learn from the other members of your team, however much more junior they might be.

(By the way, this is also a great way to learn a new programming language with an unfamiliar paradigm: find a good comprehensive tutorial, temporarily put aside everything you already know about how things should be done and even what names they're called, and just learn to do everything exactly the way the tutorial tells you. Once you've learned that, only then start thinking about how to synthesize your new and old knowledge into an improved and consistent whole.)

---

As an addendum, let me elaborate on the point I tried to make above by responding specifically to some remarks from your question:

All that is very nice until you start getting pull requests that can not be approved in days with tens of comments each.

Is this really a problem? If the PR fixes an urgent bug in production, maybe it is. But if your code review and testing processes are working, such showstopper bugs should be rare. And hopefully in such cases you can successfully make an argument for pushing a quick stopgap fix now before taking the time to implement a cleaner and better long term fix.

In my opinion, there usually shouldn't be any problem with having a PR stay in code review for days (or even weeks!) while you work mainly on something else in the mean time. Sure, it means that you'll sometimes have to return to the PR to make and/or discuss suggested changes, but you don't have to do that instantly. Maybe consider setting aside a few hours for code review every morning or at the end of the day.

If your PRs never get approved, even after you've either made all the suggested changes or provided good reasons for any that you really feel shouldn't be made, then that is certainly a problem that you should take up with the team. In some cases, it may help to simply ping the reviewers in chat and ask them what they think still needs to be addressed before the PR is ready to be approved, and what they feel would be the best way to get it moving forward again. Besides hopefully getting you a clearer idea of what the blockage is, it also puts the ball in their court to be proactive and to suggest solutions instead of objections.

In addition when requests are done, the team does not focus on what is IMHO important (patterns, interfaces, encapsulation, layering, method signatures) but on small details.

That's a common issue with code review, and I feel it's to some extent unavoidable. Whenever one starts going over a bunch of code in detail, the first things that will jump out are the small issues: inconsistent indentation, confusing variable names, lack of const / final, typos in comments, etc. One needs to get past those superficial things before one can really proceed to deeper levels of code review, such as checking the program logic and data flow for correctness and the code structure and interfaces for clarity and maintainability.

And since even the typos and other superficial issues are issues that could and should be fixed, why not note them in the review so that they can be fixed? It shouldn't take you more than a few minutes to fix such minor things, and the result is more consistent and readable code. That is, unless you insist on making it difficult and time-consuming by arguing about every single suggestion.

(Of course, there may be a problem if all the suggestions you receive in code review are about such minor superficial issues. But the solution to that problem isn't less code review, but rather more and better code review.)

Example: There is a code convention that methods doing things logically connected should be in close proximity to each other. But then if you actually require that the methods must be ordered by their chronological execution, that goes a bit further than the general rule.

Sure. Is there a good reason not to order the methods as suggested, though? If not, why not just make the change? It's just a few seconds of editing, after all — probably quicker and easier than arguing about it.

That's my one super secret trick for efficiently dealing with "trivial" code review suggestions: accept, apply and move on. Even if you secretly think that your way of writing it looked nicer.

Of course, if you can articulate a specific reason why you think that e.g. rearranging the methods makes the code harder to read or otherwise objectively worse, by all means say so. But arguing with code review suggestions should never be your first choice. Only do it if you feel that the cost (in your time and possibly in code quality) of just making the change exceeds the cost of arguing about it (in time, stress and distraction, for both you and all others involved).

No-one is just reasoning that if we in first place did not had 50 methods in same class, the positioning of the methods would possibly not matter that much.

Did you actually refactor the class in your PR to make it have fewer methods, though? I would guess not. Are you going to do that? If not, why complain about others at least trying to keep the methods in (some kind of a) consistent order — even if it's not the order you chose when you first added them?

Of course, you could also decide to go ahead and refactor the class to be smaller in the next iteration of your PR. And you teammates might even thank you for it, if it's currently as bad as you say it is. While you're at it, though, why not also arrange the remaining methods in the suggested order? Do you really have any reason, other than ego and/or laziness, not to do that?

Such heavy code review process in my opinon creates a hostile atmosphere where a newcomer feels simply bad.

That's possible. And if the team is treating newcomers in a hostile manner, whether intentionally or not, that's certainly something that should be addressed. (And you can do a lot about that yourself, even if you don't succeed in changing other people's attitudes, just by leading by example and personally maintaining a friendly, respectful and empowering attitude towards any new team members — whether junior or senior — in the future.)

But honestly, just going based on what you've written in your question (which is of course a very narrow and limited glimpse into a complex interpersonal situation), it's also possible that the only thing that's actually being hurt is your ego, and that the apparent hostility that you're experiencing is really just a case of your new teammates being less deferential and more open to giving (and receiving) criticism than what you're used to.

If (and I do stress the if here) that's the case, the best thing you can do is swallow your ego and accept your teammates' criticism and advice as it's intended — not as an insult or a humiliation, but simply as a sincere attempt to help you adapt to the processes and traditions of your new team and to improve the code that all of you share responsibility for.

Answer 7

You might have better luck arguing that developers should have the right to simply ignore certain comments, such as methods are not in the preferred order, you spelled 'Exception' wrong in a comment, etc...

It's important that everyone involved knows & understands what types of comments are ignorable.

The other developers might stop nit-picking so much if they realize that such comments routinely get ignored. I've had this work for me, to a certain degree.

Answer 8

One principle should be: The reviewer doesn't have stronger rights than the author. If you and I wrote code for the same problem, it would be different. Some differences due to coincidence, some due to the way I do things which is different to the way you do things.

If I wrote the code and you reviewed it and everything had to be done the way the reviewer wants it, then the end result would be code in your style. But if you wrote the code and I reviewed it, then the end result would be code in my style. That combination is obviously stupid, because it forces one of us to write code they don't like.

So the principle should be: If I review code, and it doesn't follow my preferences but it is otherwise alright, then I live with it and pass the review. I follow that principle. If you don't follow that principle, then we have a fight on our hands.

My colleagues and I also have the ability to pick up a branch that is submitted for review and add to it. So if I find small things (even spelling mistakes), I can fix them in place, and you just review those little bits. No need to add comments, and then you review them, and you answer them, and you make code changes, which is just unnecessary overhead.

Answer 9

the team does not focus on what is important(patterns, interfaces, encapsulation, layering, method signature) but on small details.

Actually, the small things are important, too. A small styling or coding issue can turn into a big issue when it's repeated 100's of times by the same person in the same codebase. The point of pointing those out in code review is not because you think it's important to fix this particular instance before merging, but because you think it's important to fix that habit.

Answer 10

I always find that setting the tone has an important place. Put the software product above all things else and think about the end-user. I'm not agreeing with the things you said as I don't believe everything you said gives you the right to merge without getting proper buy-in from the team but the question should be put on who is the stake holder and are they being prioritized when developing/designing the system.

I think your arguments will have more weight if you understand better the project from a higher level, your end-user, what they need and looking at it from the system behavior (as opposed to your pull requests). You may also benefit with laying low and observing initially to understand these dynamics. Maybe the user of your system is being prioritized but that information isn't being flow down to you yet due to not being a part of those meetings. Leads have visibility across the project that developers do not and may implement procedures/processes to reflect that.

I'm not sure which software company allows a developer to merge in code without a review, gates to verify system stability (in both building and behavior) or has a 6-hour timeout after which merging is allowable. It seems like that prevents the ability for the team to provide feedback, spread knowledge and risks lower quality code.

Answer 11

Many answers here already, and many good points about how reviews are conducted.

One thing that's missed by them all (correct me if I'm wrong) is the ownership of the coding standards being applied.

It seems you're new to a team with an existing culture that has an established coding standard. However, you joining makes it a new team. That ought to trigger a review of the coding standard (in addition to the regular reviews that should happen at intervals), to ensure that there's still agreement that all the items in the checklist are valuable.

That agreement is important, and it goes two ways. You, as the newcomer to the team, have a duty to point out things that no longer add value, but you are also expected to understand what the other team members expect to see when reading code, and how that helps them navigate the structure more easily. Obviously this requires an open culture - both open to say what you believe and open to receiving new ideas - on both sides.

Do remember that as a newcomer to the team, you have the most to learn, particularly in terms of the culture and interpersonal dynamics. No amount of experience in other teams gives you a short-cut to that learning, because every team is different.

Answer 12

What has worked for me in the past has been to argue about relative merits. This article for example spells out many of the arguments of why code reviews are less useful than other mechanisms to improve product quality, improve code quality, transfer information, or improve the coding ability of engineers (mostly that code reviews are manual processes, done at the end of the coding cycle). And like you mention, that sort of hostile review environment is counterproductive of building a safe, collaborative team environment.

tl;dr - Don't argue against their goals, argue that nitpicky code reviews are an inefficient way to achieve them.

Answer 13

I'm less concerned about the review & checkin/merge process and more about the amount of nit-picky issues.

Ideally, we write capabilities that when used are dead simple — that there's as little room for error as possible in consumption of our abstractions.  We would like to design our abstractions so that the consuming clients (often ourselves) fall into the pit of success.

We can sometimes use the type system to prevent illegal intermediate conditions, so that certain bad things don't get past a compiler.  We should bundle two (or more) things commonly stored or done together into a directly usable single abstraction, simplifying consuming clients.

If we can do this then hopefully the code is more directly self-documenting by use of clear abstractions and well chosen names — if we can get there, then comments are not as essential and don't have to be argued over (and sometimes not even written).

It is possible that tricky code and/or poor abstractions go more toward the root cause than the review process itself, which may be more of a symptom.

Answer 14

Once upon a time, I was troubleshooting production outages caused by our application server running out of memory. It was occasional, then daily, then a few times a day. Eventually, we were bailing buckets of water to keep the proverbial bathtub from overflowing.

Heap dumps and thread dumps showed the problem clearly as a particular list occupying several gigabytes of memory.

There were 3 blocks, all the same, but slightly different. They add an item to a list, then iterate over the list. There were 3 of these.

listA.add(x);
for (Something something : listA) {
    // do something
}

The lists should be tiny. How could this possibly be a memory problem?

I spent hours studying this code and couldn't figure it out. I showed it to my manager. He couldn't figure it out. I showed it to his manager, who is an expert at this sort of problem. He couldn't figure it out.

Eventually, I decided to refactor the code in an attempt to make it easier to understand, hoping it would shine a light on the problem. While rewriting it, I noticed something wasn't lining up right. The indentation was off when I rewrote it. So I went back to the original code and simply redid the indentations. Here is what it ended up looking like.

listA.add(x);
for (Something something : listA) {
    // do something
}

for (Something something : listB) {
    listB.add(x);
    // do something
}

listC.add(x);
for (Something something : listC) {
    // do something
}

Notice something different about the second list? The code originally was

    listA.add(x);
    for (Something something : listA) {
        // do a bunch of stuff
    }

    for (Something something : listB) {
    listB.add(x);
        // do a bunch of stuff
    }

    listC.add(x);
    for (Something something : listC) {
        // do a bunch of stuff
    }

3 of us spent hours trying to understand the "bunch of stuff" and never noticed that one of the instances of that pattern was appending to a list while iterating over it, hence, causing an infinite loop of ever-increasing memory.

Point is: When a member of your team gives you code review feedback on "small details" like indentation, you should listen. These things matter and your team mates are trying to help.

Also, side note, if you can leverage tools to automate some of these checks, such as linters, I strongly recommend that. Not only will it reduce the "small details" in code reviews, but it would have prevented the issue I mentioned above.

Answer 15

It sounds more to me that you want to do things your way, rather than try to strike a balance between what the team is currently doing (which apparently works for them, or they wouldn't be doing it) and what you believe is the ideal. That's a dangerous approach to take if you're new to an organization - you might be right, but trying to impose your will as a newbie is unlikely to make you any friends.

Pull requests that are too large to review timeously is a common complaint in the industry. These are generally due to poor planning/division of work on the part of the person submitting the PR - PRs should be submitted early and often precisely so that they don't get large enough to become a chore. The way to resolve this is to ensure during planning that the team is subdividing their stories into adequately-sized tasks that permit separate small PRs, and that they understand exactly what they need to do.

That said, as a senior, you should probably be spending as much time reviewing code as you spend writing it, maybe more (entirely dependent on the size of your team and the ratios of senior:intermediate:junior developers). If it's really too much, you can consider roping in intermediate devs - their performance on PR reviews is a good way to judge whether they'll make good seniors one day.

Refactoring as part of a PR is both acceptable and desirable, but if it gets in the way of reviewing the actual work being done and/or there's more refactoring than work, that's a problem. The solution is that when a member of the team encounters a large area of code that should be refactored, they instead log a work item for it, and that work item is discussed and assigned as part of the next sprint's technical debt work. And make sure it's next sprint - if you aren't servicing technical debt timeously, you will get people trying to sneak refactoring in "under the radar" via PRs.

Most nitpicky pull request comments revolve around coding styles in my experience (the ordering of methods one is a prime example I've encountered many times before). The vast majority of these can be overcome by having a team-agreed code formatting style, that is auto-applied by a tool every time code is checked in.

Note that comments around spelling etc. are absolutely not nitpicks. They may seem minor, but so-called "minor" errors have a nasty habit of sticking around forever. If you made a mistake, don't get upset that it was called out; take it as an opportunity to become better, thank the person for correcting you, and fix it. Don't assume bad intentions.

If you are concerned that the team is not careful enough with good patterns and practices, why not use PR comments as a way to point this out? Suggest the use of a particular pattern in a comment, and offer to pair with the person who wrote the code to implement that pattern. This is why you're a senior: knowledge transfer like this to upskill the team as a whole is part of your job. Don't be frustrated that people don't know things; be grateful that you have the opportunity to teach them. This can also be extended to workshop-type sessions, whereby you present a topic to the whole team - bonus points if you can show it in the context of the current codebase, and how it could improve that codebase.

Answer 16

If you want to address bigger-picture concerns, like:

No one is just reasoning that if we, in the first place, did not have 50 methods in the same class, the positioning of the methods would possibly not matter that much.

Then why not add a comment in the peer review along the lines of:

It seems we're trying to do a lot of different things in this class. Perhaps we could refactor it and break out pieces {A}, {B}, and {C} into 1 class, and {X}, {Y}, and {Z} into another?

You'll have to supplement that with supporting arguments, but under the system in place now, it sounds like that comment would need to be addressed by either actually refactoring (and, according to your argument, solving the underlying problems), or coming to an agreement that refactoring does not make sense here, and making peace with adjusting method order.

Answer 17

I don’t know how to argue for weaker code reviews, but I know to argue that code reviews aren’t being done properly.

Code reviews serve multiple purposes, skipping them simply because of time is silly. Don’t argue for that.

Code reviews are conversations, every comment should be responded to, otherwise you aren’t really having a conversation. There can be various types of comments in a code review: suggested changes, praise, request for clarification about either the code or the business rules, even questioning whether a particular approach or tool was considered.

The people participating in a code review should be considered peers or team mates, while they may have different skills, the goal should be for the code to be correct as well as understood and maintainable by all.

How one responds is an entirely separate issue. It’s not necessary and it absolutely should not be required, that the response is to change the code.

Assuming these comments are about request for changes, just changing the code is wrong, changing it with an “Ok” is marginally acceptable. Agreeing with the comment and making the suggested change is great. Agreeing while explaining that because of REASON you won’t make the change is ok. Disagreeing because of REASON and saying you don’t want to make the change should be fine. Disagreeing with proof that the suggestion doesn’t work is great.

Who approves a pull request is an organizational process issue, there’s no developmental reason it has to be done by anyone in particular. Likewise there’s no developmental reason why it can’t be done by a specific person.

In short, I think you need to focus on the fact that a reviewer says-so is insufficient reason to make a change, it has to be a convincing argument. And then push, not for the right to skip the review, but that it be given a higher priority. Doing reviews should be considered part of the job, if someone isn’t giving their input, that should be considered the equivalent of refusing to write code.