First rule about code reviewing is that you don't tell someone how you would've written it, but rather try to point out pros/cons of current vs proposed approach. It's as easy as that. Every time I see a piece of code I don't like I ask myself: why do I not like it?<p>I've gotten PR reviews with people literally writing "I don't like this" even after I wrote a very detailed technical argument about my reasoning. My only approach to that is to remind them that I really don't care about their personal preferences, because no discussion will come out of them addressing none of my technical arguments and instead relying on what their personal preference is like.
I'm kind of shocked at how many recommendations are merely, "have tact and don't be a dick to your coworkers." How many code reviews are shitslinging mudfests where you guys work??<p>Now admittedly I <i>mostly</i> work on projects where I'm the sole developer so structured code reviews aren't part of my daily routine, but during the period where I managed interns we'd do code reviews before every merge which mostly involved sitting down and going through the functions. I'd point out deficiencies and work to teach the student developers new tools/libraries/resources they weren't aware of. At no point did we exchange shit like "This change sucks", "Can’t you see that this is obviously wrong?", or "This is stupid code written by a stupid person."... all examples from an article which is also toting the "Avoid hyperbole" mantra. Even a single one of those snide remarks would be grounds for discipline or potentially termination if it were consistent.<p>My tip? Separate your ego from your code. If you can't discuss criticism objectively and professionally you shouldn't be involved in a professional career. Hostile workplaces are shit to be part of and I will never stand for that among my peers.
> 9. Say please<p>I actually don't like this. Maybe it's just me, but I always read these as being condescending, written by someone who thinks that I won't take a suggestion without unnecessary pleasantries and/or begging, or they're frustrated and trying to mask it.<p>> “Could you please align these variable definitions so they’re easier to read?”<p>> “Could you align these variable definitions so they’re easier to read?”<p>I like the latter.
My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion.<p>In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. Reviews shouldn't go on in perpetuity because different reviewers would all have written something differently than the author submitting the code. I've experienced this phenomenon at every company I've worked for at one point or another, and it's basically a way for Waterfall to sneak its way back in to an Agile process. (not that I'm an advocate of Agile) It doesn't happen too often, but I find it more frustrating than just about anything else.<p>Beyond some reasonable adjustments to be made in the first wave or 2 of review comments, any other non-critical changes should be separated into tickets for the next sprint or simply be labeled as DIY or "do it yourself". If you want something done, but there's no actual standard for doing it on your team, then ordering people to write things your way via code review is really just a form of <i>backseat coding</i>.<p>My code is never perfect, but that doesn't mean I want to change variable names, swap out libraries, or use different approaches merely because someone else prefers them that way. You think the variables should have different names? Wonderful! <i>Go do it yourself</i>. I'm not trying to be rude, but I've got <i>other shit to do</i>, and the software already works beautifully.
The problem I have with most similar articles/comments is that the only two options for Code Review now seem to be:<p>- point out strict violations of the team's enshrined code style guidelines<p>- accept any and all code as-is, so long as there are no "bugs" or performance issues<p>Which, at that point, why are we even doing PRs? Unit test and lint...<p>When I receive a Code Review I want to know about the function that reduces my 10 lines to 1. I want someone to notice that my git mistake introduced a bunch of unneeded diff noise. I want to change variable names that are confusing to the first reviewer, because that means they'll be confusing to me when I read the code again next month.<p>Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.
I feel like code reviews comments should be of several types.
1. There is a bug in this code(most important)
2. This violates the norms on the project
3. This is less maintainable because it can cause a future bug(two pieces of code in different that need to be consistent)
4. I personally had trouble understanding this code, here's what took me a while to understand. Would you mind adding a comment or temp variable to speed this up.
5. Here's a cleaner way to do it(totally in writers court whether or not they implement this unless they're a jr. dev.<p>I make a lot less code review comments than I used to because I've seen too many bugs introduced by fairly innocuous looking code review changes and the amount of bikeshedding that can go into code reviews comments that will likely never materially effect the application.
This seems like a pretty good list, I managed a team previously where they would make these mistakes in pretty much every code review - and the result of one engineer being nasty during a code review was very quickly a culture where code reviews were an opportunity to settle scores and showboat rather than an actual functional process.<p>A lot of this advice could be boiled down to "Would you make this comment on reddit, or would you say it to your colleague's face?"<p>The only thing I would add is:<p>>If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward.<p>Doesn't seem right to me - or atleast the focus really should be making sure that no one submits code for review that's too big for review, and that means early feedback about a feature getting too complex and breaking it down. It's much easier to go into a problem saying "I'm going to do X, it's going to be huge, so I'll do A then B then C and submit them separately". If a submission is too big to reivew it's probably non-trivial to go back and break up so that can be quite annoying.
Honestly, in my opinion, the fact that this type of thing can even happen suggests the format of code-review has a problem. I propose it's better to:<p>1. Sit at the desk of the person and pair-code review. Helps avoid basic misunderstadnings, and also is more private that a permanent written record where people may feel defensive about their choices. Also it's easier for a lot of people to be rude through the internet.<p>2. Write in small notes if any must-address concerns arise and are agreed-upon (e.g. "Security?", "duplicates function x")<p>If your devs can't interact face-to-face, then I think you've got major cultural problems or hired the wrong devs.
I review a LOT of code, and honestly, I didn't find this article helpful. It's common sense on how to behave in a civil manner, but it doesn't really help with having effective code reviews.<p>Code reviews accomplish a few things:<p>- They are primarily a <i>discussion</i> about code changes. ("Why did you do this?"<p>- They help onboard new members of the team, both in terms of how to work most effectively with the code, and with style. (No style guide is 100% exhaustive, "We normally name variables for that fooBar, not barFoo.")<p>- They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!")<p>- They help enforce good engineering practice. ("I'm not merging that until you write tests.")<p>- They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.")<p>- They also quickly identify people who shouldn't be part of the team. Everyone sees that someone moves slowly in a pull request, or that someone resists cleaning up sloppy code.<p>The thing with people who resist cleaning up their sloppy code is that it shows to management. This happens when everyone else in the team gets their pull requests merged quickly, and one person's pull requests pile up with lots of minor comments.
I disagree with the basic premise of the article that maintainers are obliged to merge code changes by default.<p>One reason is that the maintenance burden falls on more than just the author, so there needs to be some consideration on whether it is maintainable and whether the people involved are willing and able to maintain it.<p>Particularly if the change adds a public API or behavior, it may have to be maintained for the life of the codebase.<p>Also, some projects have very high reliability expectations and you have to assume that code is broken until proven otherwise. I won't merge code until the author has demonstrated via tests, comments and clearly-written code that the change does as expected and doesn't introduce potential issues.<p>The attitude in the article isn't particularly conducive to the long-term health and quality of a project, since you get a bunch of authors ramming through their changes based on their agenda and end up with an inconsistent mess that nobody wants to maintain.
I use to hate receiving comments like this (the bad ones) yet I use to give them myself almost without realising it. Obviously, I was right to give them and wrong to receive them ;-><p>It took me a while to realise that the same message can be delivered in a far nicer manner, which generally leads to a lot less conflict.<p>Now it almost comes naturally to write <i>nice</i> comments that deliver the same message, namely: the code could be improved
> 1. Rephrase your objection as a question<p>Careful with this one. In my experience, you aren't fooling anyone when you ask someone a question to which you yourself know the answer. It can quickly come off as condescending.<p>One helpful practice that we have is that we make a distinction between blocking and non-blocking comments. Nonblocking comments are desired changes, but aren't required by the submitter to fix (formatting, code simplification, naming, etc). This steers people in the right direction without preventing them from getting work done. Blocking changes are critical things which must be resolved before the code gets merged (security risks, performance concerns, complexity concerns).<p>And of course, every comment should be followed with a suggestion for how to resolve it (even if it's meeting in person). "This is bad" obviously doesn't fly.
I have started to love these "hey programmers, here are some tips for being a nice person for once"-articles.<p>I haven't been in too many code reviews, but my experience is that it is often taken personally if developers aren't friends with each other. I vastly prefer unit and integration tests to measure code quality.<p>I can also understand the reluctance of maintainers to include code of a different style. Here the tips from the article probably help, but I think it happens often enough that code is rejected for non-technical issues.
> It’s not a political or emotional argument<p>For FOSS at least, ultimately maintainers and authors are not beholden to anyone who wants to submit code, it's perfectly ok to say NO even for political or emotional reasons... so if not in the direct context of a PR then where?<p>I think heated and unconstructive PRs probably arise as a result of pushy and entitled submitters, after a certain point authors will inevitably get fed up and abandon diplomatic approaches.
I think that this is a very good article on code reviews: <a href="https://jml.io/pages/your-code-sucks-and-i-hate-you.html" rel="nofollow">https://jml.io/pages/your-code-sucks-and-i-hate-you.html</a>
I think the spirit of these is applicable to any situation where multiple parties need to come to consensus - i.e. try to focus on positive, respectful, inclusive, and objective modes of communication.
I'll just say that if you think Linus' comments are bad, there are much more (needlessly) rude people in LKML.<p>Linux might be incisive but he's often right and it's usually about an impactful issue. Some other people are just gratuitously more rude and/or obnoxious.
I have noticed that when doing a code review, I ask a lot of why questions. I want to understand why the coder has chosen for the given solution and if any other solutions were taking into consideration. I also want to know if the coder things that all use cases are covered. I find that I often happy with the answers, when I realize that the coder thought about the solution.<p>Usually, there are multiple ways to implement something and that there is no clear choice which is better. Trying to make into a black/white problem, is not the best way to deal with the inherently grayness of software engineering. Make a fuss about coding standards, for example, is often not productive.
First thing before any code review is that your team/company needs to have a coding standard. This is the first rule of engagement and eliminates most problems. Without a coding standard, code reviews quickly devolve into crap.<p>Once you have a coding standard, then the reviewer needs to differentiate between opinion and fact. If you don't like the variable name but it conforms to the coding standard then it's not a valid review comment. The coding standard should give some guidance as to what a variable name or function name should be.<p>If there's a bug or if there are potential for issues then that's what I focus on.
I found three different reasons for people doing a bad job at code review.<p>- Not having the required soft skills to do it.
- Cultural differences. Because, What is considered offensive, rude, or negative in many cultures, might be just direct in some cultures. But I do believe cultural awareness is also a soft skill.
- Having a bad mentor/peer/environment. Many junior developers write reviews with the same style as they receive reviews.
This article is complete garbage. :)<p>We absolutely must consider the non-technical aspects of a change, and it behooves us to consider them before other aspects.<p>Changes are often driven by new requirements. If we approve the change, we are approving the adoption of those requirements.<p>There isn't always a technical reason for such an adoption other than "someone wants it that way".<p>Based on this article, I mustn't reject, for instance, GPL-ed code being added to a BSD-licensed program. There is no <i>technical</i> argument against it. We can just switch everything to the GPL license and march forward; what's the problem? Can we put religion aside and just discuss the code?<p>There also no technical reason why a compiler shouldn't have a --tetris option for playing a little game on the text console. No wait, I didn't say anything about text; we can just link in SDL and have it graphical! Anyway, can we just discuss the best way to do this, and not emotional debates about whether or not to do it?<p>So, "someone wants it that way"? Whooptee doo, so effin' what? Let them fork the code.
The most annoying thing is when the reviewer doesn't recognise the complexity of some problem, and just insists on doing it in some "simple" way that doesn't solve the whole problem.<p>"Oh you spent 2 weeks figuring this solution out? Well here's my 2 minutes of naïve ideas..."
The most infuriating code rewiev was the one where I ended up with code that I considered worst then my original one.<p>The second most infuriating one was series where I had to rewrite code because rewiever did not knew syntax I used amd claimed it is difficult to read. Three weeks later he was forcing me to use that same syntax - because he found some blog where he learned it so it became necessary. Like what the hell.
Most of the advice seems to amount to watering down one's criticism. If we do that, won't the result be accepting less-good code into our projects? Is that the outcome we want?