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?
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.
Agreed. In these cases, the reviewer should always aim for learning as much as teaching (if not more so).
I've gone into code reviews where my reaction began with, "ugh, this is terrible", but evolved into, "well, I probably would have done it differently, but this approach handles an edge case I hadn't considered".
In cases like this, I explicitly preface it with "nit: <text here>", explain how I would do it myself instead and why, and then (also explicitly) say that this is more of a preference thing and that they should feel free to leave it as is. It might turn out they like my approach more and change the code, but if not, it is still totally cool with me, as they learned of just one more way to do the same thing that might come useful later.
Same. I've heard it all. From "terrible", "You don't know what you're doing", "does it work?", to "ok, I see why you did it so try this way instead because X". It's honestly who you are working with.
This sounds similar to one of the best exercises I ever had during a sculpture degree: write about a piece of work I didn't like, and explain why I didn't like it. I ended up liking quite a few of them after that, and those that I still didn't like, I didn't feel the kind of animosity towards them that I did before. I found both of those pleasantly surprising outcomes.
This advice and the other commentary agreeing with it is surprising to me. Showing how you would have written it seems like an extremely useful part of concretely explaining why you don't like it. I agree that you also need to do the part where you explain why.
I almost do the opposite of this advice -- refrain from criticizing unless I can show precisely how I would prefer to do it and why I think my way is better.
My point is that, if there's a specific reason why the code needs to be rewritten (i.e. code needs to be clearer, or more performant, or adhere to coding standards) then you can point out what's wrong with it with a simple explanation. If you just write the code "as it's meant to be" then the other person a) will not get why their code is being rejected and b) you'll be writing the code for them.
Code reviews are not coding exercises. You ought to be able to explain using human language primarily. The only only instances where I've actually written code in my comments was because I was reviewing Scala code written by a very old person who had worked 20+ years in Java and it was very very difficult to get some points across - and only after several tries where he didn't get what I meant.
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??
Now admittedly I mostly 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.
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.
After 2 years of developing at a scrappy design&dev shop, I got a new job that was a big step up at a hip SAAS company. I was in my mid-20s and I still hadn't worked through a lot of my anxiety issues, and I started getting code review from these two intimidating brothers-- one an engineering manager and one a principal dev. Their review style was very terse, and they called out _every instance_ of a syntax 'mistake' I made, and it just made me feel like absolute shit.
It wasn't just me, several of my coworkers felt the same about just getting hammered on by them.
This has kind of permanently changed my approach to code review. I always call out the things I like in a batch of code. I acknowledge that there are some code styles preferences that are arbitrary, but that I have a preference in and it's good to have a consistent codebase. If the person is working for me, or if a coworker took a nasty task that nobody wanted, I thank them for the code with the LGTM.
This sounds like a case where having an agreed-upon code style can remove a lot of conflict. Even if you personally don't prefer a particular way of doing things, if it's in the style guide, then there shouldn't be any argument.
Same things go for automatic code-formatters. If every file has to be formatted according to the canonical formatter, then you remove tons of chances for nitpicking. Your code doesn't match what the formatter outputs? You can't send your code out for review.
That way it's not personal; you're not saying "my way is best", you're saying, "this is OUR way".
There are probably a fair amount of code reviews where constructive feedback is needed but not given because the reviewer adhered to the old addage "If you don't have anything nice to say, don't say anything at all."
Tact takes effort and discipline, just like programming. To draw an analogy, maybe it's like when a dev foresees a bug but can't figure a good way to address it (or isn't motivated to address it, or doesn't have the confidence, isn't fully convinced it's really an issue, &c) decides to turn a blind eye and figures it'll get resolved further down the line if it becomes a serious problem.
Additionally, I think github can do more harm than good here.
There is so much code-review you can do before it hits a PR. But then the PR invites nitpicking, over-thinking, and mistaken assumptions. The PR was good when it was submitted but the consensus has all manner of tweaks that are, most of the time, stylistic. Only rarely do you get a serious issue and it gets drowned out in the noise.
This isn’t separating ego from code, it’s accepting that maybe you can do peer review on your code before the fact and any requirement to approve/comment after that must offer something more insightful.
also remember that the person doing the harsh reviews is probably suffering from developer defensiveness and anxiety too. A strong attack can be cover for a weak defence.
I heard this from a therapist: "everyone is just trying to feel safe and loved". It really helps me understand and empathise with people in conflict situations ("how is this action helping this person to feel safe and loved?"). It might help with understanding the code reviews too ;)
You actually bring up a question I've wondered about for along time. To most people here does "code review" mean a synchronous meeting (either in person around a conference room, or remotely via screen share) where you talk out loud about the code? Or does it just mean setting up "review/approval" requirements for pull requests in your GitHub/GitLab/BitBucket instance?
1:1 code review in person, sort of like pairing at that point. 1:1 remote via PR, 1:N in person in front of a screen or projector, 1:N remote where you need multiple approvals and different sections may interest different people, and N:M where a small team worked on a feature and request another teams review.
I think of review broadly as the remote/pr kind. But all of these are valuable in different situations.
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.
> “Could you please align these variable definitions so they’re easier to read?”
> “Could you align these variable definitions so they’re easier to read?”
Unpopular opinion but I think you're reading way too much into it. People try to insert emotion/intent and end up getting offended by something that may or may not be behind the words. It's pretty natural because in normal conversation it's much easier to tell when someone's being condescending to you compared to text on a screen.
Maybe we just can't help but be offended by nitpicking words because we're all a little insecure from imposter's syndrome, I dunno.
I agree that "please" is not helpful. Only say "please" if you genuinely mean "well, this is my preference, but the alternative is fine too". It's impossible to take something impolite and make it polite by adding "please" - and you may obtain the opposite result. No matter what a dictionary says.
There's actually differences between Australia and England in the utility of "please". In England, fines get paid faster and more reliably if you say "Please pay now". In Australia, they get paid faster and more reliably if you say "Pay now". If I got a fine that said "please pay now" I'd genuinely think I was freely allowed to not pay now if I didn't want to.
And when I was over there, it was really jarring/humorous where there were all these official signs saying "please do this, please don't do that", especially the ones that said "please don't do that, penalty 50 pounds".
So obviously this is regional. I don't really expect anyone to know who they're code reviewing for, but everyone should be aware that there's nothing universal in the interpretation of "please".
I agree about "please" coming across as condescending.
One way I write around it is to frame discussion in terms of "we" and not "you." I think "we" reinforces the collective ownership of the code and de-emphasizes the idea of the review being me vs. you.
I wrote more about this technique a couple years ago:
I think you'll find we're capable of making or reading any phrase as sarcastic :)
Less flippantly (but still true), knowing that there's at least two ways to interpret something and that at least one of them may be humorous helps me remember not to assume bad intent when I hear or read something I don't like. Often it's just their clumsiness or my over-sensitivity.
On the other hand, perhaps it also helps me hear more of the slights that are in there too! You can't win 'em all.
It's fine for a review to be passive. The review is about the code, not the person who wrote it.
When the reviewer prescribes a solution, it requires the author to push back if they want to solve the issue differently. For example, if they noticed they could delete the variable rather than align it.
Though, I'm used to code reviews where the author and reviewer are both professionals and equals. I suppose if the author is an unknown person submitting a patch, you might want to rely less on their professional judgement.
Could it be the placement of the 'please' in the example? I always include a 'please' when making a request, but I've also had similar thoughts about the mid-sentence 'please' in the past.
If you put it at the end it reads (to me, at least!) more like a polite request:
"Could you align these variable definitions so they're easier to read please?"
If it's an extremely common style in the code (something I think definitely should be done), I'd do:
nit: align variable definitions for better readability
If it's a suggestion or new style that I think we should adopt moving forward, I'd say:
Do you think we should align these variable definitions? I think it improves readability.
Then, ideally I'd leave a non-blocking task for them so they can either do it or not but it's not holding up submission whichever way it goes.
Also, I'd likely raise a ticket against a style guide or project-level autoformatter for that class of suggestion, otherwise it just adds noise to every PR.
Yeah, I also get mildly insulted by the "please". I get that it has friendly intentions, but still... I feel "managed", and it also feels non negotiable.
I often go with "I would align these variable definitions so they’re easier to read", to indicate it's a suggestion, but in the end it's up to you.
And I like "Could you align these variable definitions so they’re easier to read?" better than "align these variable definitions so they’re easier to read.", or worse, "align these variable definitions".
This is why code review should be done in person if possible. There's too much confusion about what tone people intend to convey via text alone. Instead let's just have a conversation about it.
IMHO, the equivalence you propose is definitely not spelled out in the semantics of English so you really need to be reading a lot of sarcasm into the sentence to make that leap.
Part of functioning well in groups is understanding that not everyone shares your specific quirks - everyone has their own weird shit.
Of course, trying to accommodate everyone's unique perspective and behaviors would never scale, so instead we fabricate social norms that represent a sort of common denominator of human behavior. Part of these social norms are manners and polite behavior. These act as a sort of protocol for human interaction.
This all a long way of saying that even though you may think saying "please" is condescending or implies a favor, you are almost certainly in a vanishingly small minority and this will not serve you well socially.
I am trying to give you some insight into why you struggle with social interactions and often rub people the wrong way without intending to or even realizing it. My strong recommendation is you be polite even when you think you don't need to be.
Tolerance for and expectation of "please" can be cultural. OS code may receive contributions from global contributors.
Anecdotally, in my team which spans individuals from each continent except Africa and the poles, US english speakers are the least likely to use 'please', whereas our APAC (asia-pacific) contributors use it liberally.
Was i was trying to say was mainly that saying please implies that what is asked is more of a personal taste, and not an objective thing that really should be fixed.
If it's really something subjective I would say "This would be nicer if..." or "I think that could be changed to..."
Instead of formulating some almost passive-aggressive request with "please".
or SE Asian team members who say "yes boss, sure" and then do nothing... and then you find they disagreed with the thing you proposed but there's no way in their culture that they can disagree with the boss, so they just say yes and do nothing. That one was fun to deal with ;)
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.
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.
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 backseat coding.
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! Go do it yourself. I'm not trying to be rude, but I've got other shit to do, and the software already works beautifully.
While I do not know your exact situation, I find such attitude a bit selfish. Yes, you've got other shit to do, but guess what, writing good code is part of the shit to be done.
I consider working code being a minimum standard. Code can be working but can still be bad. On top of correctness it should be easy to read and modify. And by telling your team go modify if themselves if they don't like it, you're kind of saying "if you expect me to do more than minimum, do it yourself".
Again, it varies from team to team and you might have colleagues who focus on meaningless details instead of the essence.
What I try to do in my teams is to encourage people to go together through changes suggested by the reviewer. If the sides agree that one version is objectively better than the other, go with that version. If there's no agreement or if the difference is too marginal, the author of the code makes the call.
Also for me the review has easy "done" definition. It ends either in "code approved" or "changes requested" state. The first one leads to merge, the other one to further work or discussion on the code.
Then we're both in agreement, because my point isn't that code shouldn't be subject to standards, but that a lot of feedback can end up being pedantic and that there needs to be a line drawn to prevent code reviews from turning into an endless cycle. If the difference is marginal, the author of the code should be able to move on to other things.
Code working is only a first step to being good software. Software should be maintainable, and readable, clear, conformant code is necessary for future maintinability. Granted most of the style should be written down, but when I'm reviewing code, most of what I say is essentially "if you do this, it will be easier for others to maintain in the future."
Code review is very bad at catching bugs (static analysis and tests do that). If all your code review does is attempt to catch bugs, you're just wasting time.
Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach)
I'm not advocating for flimsy or convoluted code, yet I don't advocate for perfection. If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code, then there should be a clear point where the review is considered done, the code is merged, and any further notes from the review are used to generate new tickets that may get handled by anyone on the team.
> Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach)
This doesn't really matter. Yes, there will always be some level of disagreement. But you aren't writing code in isolation, and it is your job to write code that your team can maintain. If I, a reviewer (who is presumably well versed in the language in question) cannot understand what a block of code is doing without expending significant mental energy, that is a red flag.
I'm talking about refactorings on the orders of 10s of lines of code, not changes to design patterns of architectures, those should be decided much earlier (and in general PRs should be small enough that you aren't forced to redo a design as part of a code review).
> If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code,
And this is where we disagree. A reasonable person being able to understand the code is only the starting point. Code should only be merged when, to a good approximation, you're comfortable with anyone on your team (or not on your team, consider onboarding) being able to understand the code quickly.[0]
One pet peeve of mine in this regard is loops that break or continue, especially those that are nested. They're often easy to write, and sure I can figure out what's going on eventually, but it's not easy to maintain, and there are straightforward, mostly mechanical changes that can make things easier to follow:
a loop with a break is almost always an attempt to find something in a container. If you're continuing, it means you've got a function that finds and then does something with the found object. Split it in two. Have a function that loops through and returns the result, and then a second function that processes that result.
A loop with a continue can almost always be refactored into a filter (in practice, I am not saying you should be using map/filter everywhere) by reversing the conditional.
There are exceptions to this: early breaks/guards (sometimes), "returning" by reference in C, and sometimes building state machines can be done most clearly with continues. But those are fairly context specific (and they aren't the context I work in, for the most part), so can be called out explicitly. That's the kind of thing I look at.
[0]: Another way of thinking about this or justifying this: If someone gets paged in the middle of the night because there is some production issue happening, and the trace happens to cross through this chunk of code, will it significantly delay this groggy person, who is neither you nor the author, and who has perhaps never looked at this code before, from quickly isolating the root cause of the issue?
So much of this is subjective. For example, I find break/continue to be far clearer than anonymous functions and map/filter (because you can skip/bail early and for single conditions).
Generally for code review, I find the best strategy isn't "this could be better", and _definitely_ not "I would have done this differently". Currently, I think it's "this gets the job done (is tested, passes acceptance tests), isn't overly clever, and looks like most of our other code". Anything more than that should either be a larger refactoring/rearchitecting job, or threatens to quagmire you in a tarpit of subjectivity.
> So much of this is subjective. For example, I find break/continue to be far clearer than anonymous functions and map/filter (because you can skip/bail early and for single conditions).
To be clear, I am not suggesting anonymous functions, nor am I suggesting using the map/filter constructs. I'm suggesting that
def my_process_seq(seq)
for obj in seq:
if meets(obj, cond):
break
return process(obj)
is better as
def get_obj(seq):
for obj in seq:
if meets(obj, cond):
return obj
process(get_obj(seq))
This gets even more helpful if you have complex conditions, or nested loops and apply this hoisting operation at each level. I have even stronger opinions about how to do this the "best" way. But break and continue are (at least in my line of work) generally the worst way.
"if you do this, it will be easier for others to maintain in the future."
Can be pretty subjective. And unless it's a very specific comment like "This is less maintainable because if someone calls these methods out of order you end up with a bug, so we should make these methods private and expose one public method that calls these methods in the specified order" then I think if the convention isn't worth writing down it's not worth arguing about in a code review.
Some people who are less in a hurry want this kind of feedback. (See sibling comment.) So you have to expect some negotiation over what kind of review you want.
I think the key here is that it should be possible for reviews to mention improvements while not treating them as blocking the commit. When I was reviewing code I would often say this explicitly: "You don't have to do this, but..." Or, I would prefix some comments with "nit: ".
If you're always pushing people to say less in code reviews and they listen, it can turn into a rubber-stamp process. Reviewers need positive feedback too!
This is why I love code review tools with a "task" function (e.g. BitBucket). Adding a task blocks submission until the task is marked completed, but anyone can do that. So, you can approve a CL but add tasks for nits or non-interactive suggestions (things that can be done without needing a response from the reviewer).
Then, the author can implement the tasks they find reasonable and ack the rest and not be blocked on submitting by a bunch of back-and-forth over style details.
I use the term ‘consider’ often in code reviews. “Consider using x...” conveys the idea of it being optional but worth their time to think about. I leave ’should’ for things worth arguing about. :)
> 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.
I've actually been experiencing this myself, as I've recently started a job with code reviews, and it's been driving me crazy.
> 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.
One of my biggest personal issues currently is that I have a fear of hitting the GO button, doing one more look-over just in case. I suppose it's just a lack of confidence in my own abilities, or maybe being anxious that my code isn't up to the standards of everyone else and the code base as a whole.
IMO one of the great benefits of working physically near your coworkers is the ability to sit in a room and discuss things. It never made sense to me that PR's would happen mostly over the internet when the person who wrote the code might be sitting right next to you. After in-person discussion, comments could be dropped in the PR as a reminder.
The "do it yourself" mentality needs buy-in at least from your team and a spirit of frequent refactoring otherwise you're being difficult... I think if you can get one reviewer to ok the merge, then merge it, fix followup issues subsequently (as new tickets you can pick up or someone else) but if you can't even get one other person to sign off... that's a problem if you then merge anyway. Ultimately what is merged without signoff can be reverted without signoff, but hey, you might at least get someone to take a look at it then.
Just getting to code reviews at all is a huge win for a team, every process after that (including "definition of done" for reviews as well as tickets, merging without signoff (I do it for small trivial things) and the post's "be nicer" guidelines) should be discussed as a team in retros. Something that's worked for me (mostly on remote/distributed) for making sure reviews don't stay in limbo is as an author to be a bug and aggressively ask specific individuals for a review, even going so far as to schedule time to hop on a video screen sharing call if some background context and walkthrough is helpful. But the team needs to be accepting of this, otherwise again you're being difficult...
At a team level another successful practice was for everyone to block off a convenient (not identical) chunk of time once a week to dedicate to making sure you have no pending reviews waiting on you, and also letting you say "please wait until [time] or see if someone else is free" when you're busy and the author bugs you. Sometimes a good review of a large change takes more than a focused hour or two -- it's fine to push back for smaller changes, but if you just block off some time, you can plan for it and just do the big review and then a limbo period will just be at most a week.
If you have to argue about naming conventions you are already doing it wrong. The style/conventions should be written down in a doc so they are objectively right/wrong -- no discussion.
I have been experiencing a lot of what you are mentioning for years at the place I current work. The problem is that any code review feedback can be provided under the guise of 'quality' - once that Q-word gets into the discussion, it's very hard to argue that it doesn't matter or is not worth the time.
Not that I don't want to write quality code, but nitpicks in the name of quality are the code review equivalent of a death by a thousand cuts.
Many times when someone asks me to change something without pointing to a previously agreed-on coding standard that all team members approved before, I just call it bullshit and let the PR sit there indefinitely while I get to my next task. If someone is interested why the PR is not released yet, I always point out that some people wanted to have changes but they were lazy to make those changes themselves even though the code base was accessible for the criticizer too. And I never fail to mention that I am already working on something else so going back is just the waste of my time and my time is as precious as the criticizer's.
Of course I am not this arrogant when there are agreed-on coding standards and the reviewer points out mistakes based on those standards (but that almost never happens). I am more than happy to make changes in that case.
So to sum it up: pull requests have become the prefect tool for workplace bullying.
First, you have the choice to be arrogant or not arrogant. I would suggest that you’ll be more successful if you choose the latter.
Second, not all team norms and guidelines can practically be discussed in advance. It is much better for a reviewer to give a sincere and detailed review than to leave out useful feedback.
Well I can't even imagine a more arrogant thing than criticizing someone's work without the right to do so. Just because people have a keyboard it doesn't mean that they have all the fundamental skills and experience to give feedback on anything.
> not all team norms and guidelines can practically be discussed in advance
I would suggest that you’ll be more successful if you grow a spine and try to tackle the fundamental issue first: having norms and guidelines. If that's your attitude to programming your professionalism is very low.
You ignored the most important thing in a successful development team's life and now you are suggesting other people how to become more successful? Ridiculous.
I was trying to constructively and civilly offer feedback. You did not reply in the same spirit.
Your comment was rude; many people would ignore it.
In the past (and occasionally even recently) I’ve been rude, arrogant, or not valued people properly. It didn’t get me very far. This is the reason I’m writing despite your tone —- because I want to help you see more broadly about what is important.
If you disagree, fine, but open your mind to the possibility that you should revisit this topic later with more experience and perspective. Defensive, aggressive, and ad-hominem attacks are not useful and simply make you look worse.
Being right is not sufficient. Life and work are bigger than that.
If not me (someone you don’t know), I sincerely hope you ask several people you trust for feedback on this topic. I think you might be surprised by what you can learn.
Your comment also makes unwarranted and illogical assumptions. For example, I never said that having norms and guidelines is a bad thing. I only said that there will inevitably be areas that are not covered in advance.
Most importantly, you have a choice in how you act. Aspire to be both correct and kind. This will get you farther.
Look worse than who? A random no name person on the internet? Cry me a river.
Just so you know I manage 6 people right now and we are doing quite well. The sort of yours would not be welcome for sure. People who cannot think out of the box will always be boring and predictable.
If you cannot imagine a world where code reviews don't help at all it shows your lack of experience. Work 10 more years in 8 different companies as I did and you will quickly learn that what you say has no grounding.
Being correct and kind is cool. But it won't get you anywhere.
My interpretation of your comment is that if someone applies their experience and expertise during a code review, and comes up with issues or suggestions that are not part of a written standard, then you will assume that your evaluation (maybe of the merits of the case, or perhaps just the importance) is more worthwhile/accurate than theirs.
Also, your preferred approach is to passive aggressively let the PR sit until external pressures build enough to force it through.
Do I have that right? If so, I personally find that to be a deeply problematic approach. Though I can imagine situations where it would be defensible and even optimal -- but those situations would involve working with idiots and/or control freaks. In which case my preferred approach would be to find a different place to work.
Well you got it right. I was surrounded by control freaks. Today I am the happiest man on earth because I don't have to put up with the idiocy of code reviews anymore.
Pair programming, live coding sessions and study sessions. When we start implementing something we define the definition of done and the acceptance criteria. When the code passes linting and meets the acceptance criteria it is accepted. If it doesn't then it's clear why not. There is no such thing as a commit needs improvement. It either passes or fails based on the above.
The problem I have with most similar articles/comments is that the only two options for Code Review now seem to be:
- point out strict violations of the team's enshrined code style guidelines
- accept any and all code as-is, so long as there are no "bugs" or performance issues
Which, at that point, why are we even doing PRs? Unit test and lint...
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.
Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.
I know the pain - but its no use reducing it to a dichotomy (All or Nothing).
The strong reactions are due to pedants who block the source base doorway and pick at everything going by. It can sink a project. So folks push back, say things like "Only comment if its a real bug!" and so on.
There's lots of middle ground in a company source process. For instance, point out real bugs if present. But for anything else, comment and approve, respecting the contributing Engineer to do what they can in the time available.
Code review at least ensures that more than one person has seen and read the code. Hopefully they've at least kind of understood it, too. That's valuable in itself against the bus factor or "WTF is this?" surprises.
I think you might have to ask for that sort of feedback since people differ in how much criticism they can take. (Sometimes based on how busy they are.)
But the tips in this article work for subjective comments too. Rephrasing comments as questions is nearly always applicable.
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.
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.
>code review comments that will likely never materially effect the application
I think that is actually a good bar for a CR comment. Issue of maintainability affect the application. Bugs affect the application. A lack of general good software architecture has long term effects on the application.
All other comments I either tag on an "as an option, you could..." or force myself to shut down the "that's not how I would have done it!" voice in my head and just leave no comment.
On the receiving end, few things are more obnoxious than "deck chair rearranging" comments which have no bearing on performance, maintainability, style, or correctness, but just boil down to you satisfying the benign preferences of whoever is looking at your code.
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.
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?"
The only thing I would add is:
>If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward.
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.
The number 1 reason why I actually try to make code reviews small is because the few times I made big code reviews, they just sat there. They get a few comments here and there, but the reviewers seem reluctant to review them at all, or just keep doing partial reads without the confidence to approve the whole thing.
I even found the relationship non-linear. I.e. if you can break up a big change into smaller changes that have more overall code, you can still probably get those reviewed quicker.
I probably would have learned the lesson faster if reviewers had just rejected the changes for being too big, or at least a "next time you could have split it this way".
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:
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.
2. Write in small notes if any must-address concerns arise and are agreed-upon (e.g. "Security?", "duplicates function x")
If your devs can't interact face-to-face, then I think you've got major cultural problems or hired the wrong devs.
Agreed. Pair reviewing is great. Especially with people you don’t know that well. Once you know each other and trust each other the traditional review tools work better.
Code review is like any other interaction pattern that happens asynchronously. Many teams work this way. Remote teams, teams with people on different time zones, open source projects (what this article seems to be geared towards), etc. Face-to-Face interaction isn’t always possible, and that’s why the advice like the article’s is useful.
This makes it much harder for people to review stuff since they'll need to schedule for such reviews. Normally reviews happen asynchronously whenever people find time and often code review tools allow for writing a review piece-by-piece (that is, your progress is saved on the server that hosts the review) instead of all-in-one.
Introducing any form of friction makes reviews less desirable for everyone involved.
Agreed. But for complicated reviews, a high-bandwidth synchronous meeting can be useful. Perhaps start out asynchronous, with the reviewer requesting a live meeting/call if they feel it's necessary? That's always worked well for me.
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.
Code reviews accomplish a few things:
- They are primarily a discussion about code changes. ("Why did you do this?"
- 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.")
- They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!")
- They help enforce good engineering practice. ("I'm not merging that until you write tests.")
- They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.")
- 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.
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.
> It’s common sense on how to behave in a civil manner
I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread.
[Conjecture/opinion] I believe that many people who enter software development and related fields are high on the autism scale- eg aspergers, etc., and that many such people are unaware of how badly that they’re treating others. It’s not “common sense” for many.
> I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread.
I personally think that terseness is actually polite since it wastes a lot less time for both sides.
I disagree with the basic premise of the article that maintainers are obliged to merge code changes by default.
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.
Particularly if the change adds a public API or behavior, it may have to be maintained for the life of the codebase.
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.
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 was mentally thinking about refuting your argument, but then I started wondering about systemd (which I do not like, although I do have concrete criticisms).
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 ;->
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.
Now it almost comes naturally to write nice comments that deliver the same message, namely: the code could be improved
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.
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).
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.
What helps here is to then actually listen to answer and try to learn from it. As in, it becomes less condescending when the answer is listened to and actively acted and reacted upon in follow up conversation.
I have started to love these "hey programmers, here are some tips for being a nice person for once"-articles.
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.
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.
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?
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 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.
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.
This is why it doesn't matter if you are right are not, if the attitude that comes from the top is rude and obnoxious, then other "less right" people will follow suit. The culture then degrades as people generally emulate the behavior of elites. The example of behavior should be set at the top. Being technically right (this time) is no license to train others how to be an ass.
In all fairness Linus often makes pretty detailed comments with a lot of patience. He only blows up from time to time. There aren’t many tech leaders who have the capacity to understand issues and then explain them.
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.
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.
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.
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.
- 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.
We absolutely must consider the non-technical aspects of a change, and it behooves us to consider them before other aspects.
Changes are often driven by new requirements. If we approve the change, we are approving the adoption of those requirements.
There isn't always a technical reason for such an adoption other than "someone wants it that way".
Based on this article, I mustn't reject, for instance, GPL-ed code being added to a BSD-licensed program. There is no technical 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?
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?
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.
"Oh you spent 2 weeks figuring this solution out? Well here's my 2 minutes of naïve ideas..."
When the reviewer can't understand why the complexity is needed, I add a comment explaining it. This is a very frequent response by me: "Yeah, it's not obvious why the simple approach doesn't work. I've added a comment."
The problem is that the reviewer often doesn’t have time enough time to really understand the problem. So you often end up making trivial comments because you don’t really understand what’s going on.
Sometimes I self-review when I can anticipate that, i.e. just leave some (non-code, though it might make me consider that they should be in code) comments with a bit more context.
The most infuriating code rewiev was the one where I ended up with code that I considered worst then my original one.
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?
It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them.
Second, when you first start on a project you don't understand it all. We want to grow your understanding and bring you in as trusted contributor. Setting the right tone in those first days makes all the difference in the question of will you contribute more code or not.
Last, most open source projects are essentially unmaintained. Almost any contribution is better than that, even if the code is bad, if it works at all...
> It is often hard to tell the difference between great code and bad code. I've written code that was fast and very flexible, at the expense of being complex - the flexibility turned out not used at all (after 8 years in production I'm confident it never will be), so on hindsight this wasn't great code - but if the flexibility envisioned had been used the complexity would be required and the solution would be great. I've seen code that cleverly handled multi-threaded issues such that can't be understood or modified by anyone without breaking - but it great code because it abstracts the threads so nobody else has to understand them.
At my previous company, the lead dev did this (the complex code part with the lack of usage too), unfortunately his view was that it might be used sometime.
I think, after 10 years, we were handling about 100 events per second peak load and what I pointed this out he countered with: What if we had to handle 100000 or 1 Million?
I gave up after that.
For context we were a B2B company with a relatively long sales cycle, so ramp ups were generally very strongly correlated with on-boarding of new customers and we normally had notice of what was coming. In other words, the naive solution would've worked fine ( the 100 events (not web) were across ~ 5 servers (servers did other stuff too)) and we would've been able to scale up while we did improvements to the naive solution.
In my experience, this is not the case when both the reviewer and the reviewee are working together and want to find a good solution.
Rephrasing criticism is a pretty common technique not exclusive to code reviews and imho helps avoid a defensive stance which makes it easier to be productive.
The idea is to express the intent without the emotion. I don't see any watering down there; in fact, it's likely the criticism will be clearer, more direct and more palatable.
Your comment is factual untrue. Most advice is actually removing reviewers emotional reactions in exchange for more factual statements. The only watering down is that it gives benefit of the doubt to reviewed code where difference of opinion may be the issue.
The first comment actually means a strong opinion. Impossible usually really means impossible from technical standpoint and is not used for things that are heavily inconsiderate or inconveniencing future development. The suggested option is Machiavellian and patronizing at the same time, and pushing burden of proof on the author of the code instead of reviewer. Puts them on defensive. Both are evil, but first is clearly an opinion, second is also a personal attack.
Proper review really must have "X because Y" form with perhaps exception for trivial syntax fixes. Like form 5.
Part two also forces additional burden of proof on the author. Generally for performance changes documentation requirement is the only thing. Preferably runtime numbers or profiles rather than abstract notions. We've seen people going for algorithmic gold plating where it actually list performance to constant factors.
Doing 6 in excess is viewed as patronizing. There is no good way around it.
And finally, presuming you know the teammates making the review as bland as possible without a bit of color can be the worst thing as reviews will be felt as a chore. They are one still but humor can help. Some people like their reviews to be ultimately super boring instead, and you have to be careful.
I don't think OP wants a project to accept anything "less-good". It's just a post asking to be respectful to the contributor. Attack the idea(in a civilized manner) - not the person.
Seems to be advocating more for thoughtful criticism (and meaningful discussion) than anything. Is it going to make things better? To what degree, I don't know, but I'd imagine it helps with people creating pull requests and then disappearing from the project (which I've seen multiple complaints of this behavior over the years). It'd make them feel more of a stake in it I'd imagine if they're not receiving blowback from the maintainer for daring to create a pull request (based on the "bads" in the examples).
I've personally never seen anything as bad as the examples given (I've read stuff from Linus that are there, but seems like he's trying to change the atmosphere there now), but if the communication is that prickly, I can't imagine it is going to receive a ton of outside help in the form of pull requests (that could be intentional though).
Exactly. I seems like most of these are jumping off points for starting a conversation.
I may be wrong but I read the article as a senior reviewing a junior's code, in which case the big win is not just getting better code in now, but getting a better developer. Starting the conversation instead of jumping all over them is important.
Even when reviewing a peer, while more terse we tend to phrase things in questions, "Shouldn't this be in a lib instead?" Frequently it's oversight on their part, but sometimes they have their reasons and I learn something in return.
"How do we do X with this change?" is less clear if what you really mean is "This change makes X impossible". "Thank you for your contribution" is misleading when talking about a contribution that simply doesn't meet minimal quality standards. "Please" suggests that you consider something optional, which is misleading if you are actually expressing a requirement. Snide or hyperbolic remarks convey a message; perhaps not a welcome one, but one whose meaning is presumably intentional.
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.