0

I have been working almost for a year in my current place. It is a product team over some domain. Here are some example reviews I am getting every now and then (Scala).

  • Removing unneeded wrapping curly brackets from a function definition with single expression inside which does not even give a warning or anything. Should be a linter job if you ask me.
  • Got another one for changing the location of single import in the beginning of the file from line 5 to line 2.
  • Yet another one for calling some function inside a block and not outside. (no bugs or anything)
  • Philosophical discussions about variable names.
  • When I don't write tests, review is to write tests. When I write them in another PR, review is to delete since it can be understood easily this time.
  • Suggestion is to split some PR (20-30 changed files) because it is easier to review that way. Then the same people have PRs with 300 to 1500 changed files and they end up getting reverted multiple times.

I can make this longer and longer but there is no need. The only reason I do not publicly make this an issue is that from time to time these people actually suggest good stuff too. But it is very easy to have PRs with 50+ comment on any day too if they are not busy with their own work.

Of course, it is hard for you to comment on the above list, but maybe you have more awareness and better evaluation of the reviews you are getting and tell some signs about toxic or non-productive reviews. I wish there was something like "code-review review".

12
  • 3
    Sorry, but there is no real question here. This is somethin you should discuss with your team members, not with strangers from the internet.
    – Doc Brown
    Dec 7, 2023 at 22:23
  • 3
    I get useless code reviews from time to time. Sometimes people outright suggest bad things. I simply challenge them. If the reviewers insist on blocking the merge while QA agrees features have been implemented, I simply escalate to management.
    – Ccm
    Dec 7, 2023 at 22:27
  • 2
    @DocBrown the question is obviously: how to suggest improvements to workflow?
    – Basilevs
    Dec 7, 2023 at 22:27
  • 1
    Non-owners often provide invaluable input on cross-boundary concerns and may have a different qualification level. Definitely, review comment should not be evaluated by its author. A proper review conversatuon is never personal.
    – Basilevs
    Dec 7, 2023 at 22:55
  • 2
    @Basilevs: the title does not match "how to suggest improvements to workflow". The title is a "yes-or-no" question which the OP already answered in some kind of rant. If the question is "how to improve the workflow", the OP should state it. Or maybe they wanted to ask "what are the root causes for these signs of bad code reviews"? I honestly don't know. IMHO the wording should be fixed ASAP.
    – Doc Brown
    Dec 8, 2023 at 6:11

4 Answers 4

3

I hear your frustrations. "Bad" reviews? Let's just call them "unproductive" reviews.


goals

A reviewer is trying to establish two things about code before we merge it to main:

  1. Is it correct?
  2. Is it maintainable?

Beyond that, maybe the Author taught the Reviewer something by sharing the code. Often the Reviewer will seek to teach the Author, about concepts that may provoke edits to this PR but usually will influence how Author writes future code.


be considerate

It is seldom that I encounter a PR with an outright bug on which I must write an imperative "Fix this!" Most of my advice comes across in a gentler tone: Doing X was odd because of .... Consider doing Y. Prefer Z.

For such discretionary items, Author always gets final say. Having viewed the code from another person's perspective, you might agree and decide to make an edit, or feel it doesn't rise to that level and just merge, with no hard feelings.

When Author submits short PRs, they are easy to approve and quickly get through the process. Larger PRs OTOH create additional work for reviewers, even if three small ones show up combined as one big PR. Why? Because now the Reviewer has to pick his battles, choosing and prioritizing the most egregious issues. This requires more effort than just blurting out each issue you see as you read down the list of edits. Also, big PRs tend to create greater cognitive load due to more forward references.

If you're making a mechanical "whitespace change" or "rename of popular function" that touches a bunch of files, do go to the trouble of breaking that out as an initial PR, and then a subsequent PR can build new features upon that groundwork.


lint

This should just not be a thing. You shouldn't be bringing it up, your reviewer shouldn't be making remarks about it, it should just not happen. Life is too short. We have features to implement.

Your team (or project) should have an on-boarding document that writes down policies like "lint your code!", describing the expectations up front, including particular tools and settings. Auto-formatting of source code works well for some teams. Sorting of imports is work for the computer to do, not humans.

If your code doesn't lint clean (e.g. $ make lint), then that's a code defect and the code wasn't ready to begin the PR process much less being ready for merging. A reviewer should never be raising lint issues during review. If that happens, it indicates a process problem.


meaningful identifiers

Inventing good, accurate names is surprisingly hard. Sometimes you'll come back to yesterday's code and say "what was I thinking?!?", and ask the IDE to globally rename some unhelpfully vague identifier.

Reviewers add value by proposing name improvements. Author doesn't have to accept all suggestions.


merge

If any review remarks are show stoppers, they should be addressed before merge, or minimally they should spawn new issue(s) to be worked on soon and be approved in a subsequent PR.

Any other review remarks should not hold up approval. Author can agree or not, make edits or not, and merge it down.

4
  • 2
    I only see one unproductive type of issues in OP - lint. Everything else is valuable, albeit minor and negligible input.
    – Basilevs
    Dec 7, 2023 at 22:52
  • @Basilevs I see value in most of the reviews too and that is the reason I am not escalating this. Yet, it is good to question things and understand the norm here because I am only entry level and my awareness is not sufficient. If the culture is like this and it applies to everyone, I do not even complain. But it is very easy to see buddies approve each other without further comments while I am having these conversations and getting slowed down with a review like that. Dec 7, 2023 at 23:00
  • @user2694307 again you are evaluating a review process by participants. There seems to be a major miscommunication.
    – Basilevs
    Dec 7, 2023 at 23:05
  • @Basilevs I think for this discussion to be fruitful, you are right we should only discuss the review process itself and it makes sense to me that we put the participants outside for that. There have been mentions to people only because my reasoning was that I compare similar PRs, same reviewers -- almost like a controlled experiment. But that is another thing and might not fit in this community I agree. Dec 7, 2023 at 23:24
2

Signs of a bad code review process.

  1. No written rules for what constitutes a fail
  2. No assigned named reviewers
  3. Too many reviewers

I think some of your examples maybe fall under these categories. For example maybe you have too many random reviewers instead of just the people directly involved the code you edited, and no set naming convention rule

However, the gist of your examples suggests to me that you are making unneeded changes to the code.

Reviewing code is a pain and you want to make the job as easy as possible. Which means keeping the changes to a minimum and always including tests which show the code you changed/added actually works.

1
  • Great observations about how some standards might be lacking and some rules are not defined. Things become a personal preference then. Dec 7, 2023 at 23:39
1

One option is to have a distinction between major and minor issues. Major issues have to be fixed and are verified on the next iteration of review. Minor issues are left to author's discretion.

Even without this tagging, as an author you have the right to not act on a review comment (resolve a discussion with a short "minor/optional/won't do").

1
  • 1
    That seems like a solution indeed. One concern of mine related to that is if I start to tag or resolve most of these small things like that, there is a chance this show continues in a bigger fashion. Nevertheless, it is a candidate for solving the problem. Dec 7, 2023 at 22:37
0

I look at code reviews as the govt, and I hear Thomas Jefferson's sagacious quote: "The government that governs least, governs best." The reviewer's assumption always seems to be that you never thought about what you were doing when you did it, therefore, they feel obliged to say something, anything.

2
  • Can you give an example of this?
    – Dominique
    Dec 11, 2023 at 9:53
  • It's a very common human fallacy, the infamous Stanford Prison Experiment being a prime example. If one has ever programmed in a group of programmers, what seems to inevitably settle over the group is a sense of daily "dick measuring". Any personal example that I have is going to take longer to explain than the characters I am allotted here. Again, in the realm of politics, how much argument is about serious things, and how much is about trivia simply to have something to whine about? Dec 12, 2023 at 13:41

Not the answer you're looking for? Browse other questions tagged or ask your own question.