Some sensible ideas - however, I'm on the fence about this one: "Unhelpful behavior: asking engineers to solve problems they didn’t cause"
Generally, I agree with breaking out bugs or issues that are found 'along the way' into separate subtasks or tickets. However, often what happens is a ticket is thrown into the backlog and not addressed.
Because the developer has just touched the code, this is often the best time to refactor as its still fresh in the mind, and this creates a culture of constant improvement.
Of course everything depends, in this case, the complexity of the fix is the main limiting factor.
We truly believe in Refactoring along the way instead of trying to break out tech debt stories separately.
Seems to me, this is more a problem of ticket/workload organisation.
> However, often what happens is a ticket is thrown into the backlog and not addressed.
Every management method is different, but e.g. in Scrum (I think), there is the idea of a person reviewing and priorizing work tasks according to the roadmap. this person would decide if a "fix this messy code" task is important enough or not.
Then again, I think there is the every day a good deed philosophy of refactoring, too. I think in that case, one should just make sure that the "fix along the way" task doesn't disturb the original problems too much (so commits can still be reviewed) and that there are no unintended side-effects.
Yes, I've been on the fence about this one as well. What I generally do is only add such fix-it comments if I identify other issues in my review that WERE caused by the current PR and need to be addressed anyway. So I would not hold up a PR for such matters, but if it is otherwise incomplete, I feel it's OK to ask for a modest amount of extra work.
I agree with refactoring along the way, but I think it's important to clearly state the _purpose_ of a branch or pull request, and to make sure that refacors don't wildly blow out the scope of the PR. I've been on the receiving end of too many PRs that refactor a bunch of things all at once because "well I was in there" to feel good about unchecked scope creep in PRs.
I'm a little torn on this as well, but I think I'm in the author's camp:
1. The person making the change to the code for their purpose may not have the required overall understanding to execute a refactor.
2. I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible.
3. The person making the change has a purpose in making that change, and likely has committed to some sort of schedule or timeline for getting the work done. Forcing them to play janitor could easily derail that.
4. The change at hand might just be a small piece in a larger body of work, and shifting focus to a refactor would be disruptive.
Having said that, I do often take the initiative in doing refactors when making a change, if it makes sense to me to do so. But I don't think it makes sense to push that expectation on others. Hell, it's possible/likely that the person making the change has noticed the opportunity for a refactor, but has decided against it for whatever reason.
I might suggest a refactor in a code review, but couched with language that it's entirely optional, and won't block merge of the change as-is if the author decides it's not the right call for them.
I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible.
One change at a time has the benefit of not polluting traceability of what was done and why, in the ticketing system. If a commit for ticket1234 was to fix a bug in the payment system, and the commit has unrelated code fixing up user data access, that's bad in my opinion.
I've found myself in many code bases where the only trail of breadcrumbs I've had is ticket references in the commit messages. When these aren't reliable, you're left with not much at all.
> Why didn’t you just do ___ here? as opposed to "i think xxx is a better approach because"
This is such a good and deep point - being forced to defend your decisions is one thing - using a form such as "the whole
world knows not to do this why don't you" is another.
i would argue that there is value in developing a longer for review comments - it's probably viable in fact - such that the comment is non judgemental.
As a good rule of thumb, I've managed to set a trigger in my brain that goes off whenever I use the word "just." I've been a recipient of that word too many times to not know better. It's amazing how much "just" can cover up.
It is perhaps mostly orthogonal to the specific phrasing used here, and certainly the word "just" isn't in and of itself bad. But my feeling is that my technical writing has improved dramatically if I carefully consider each use of it. It often exposes a perspective gap that I have with my target audience.
The most toxic behavior is being overly protective or offended about your code. This goes both ways: Both as the review-seeker (that opened a PR) and the reviewer (That most of the time got his original code modified by the PR).
As a general rule I think there is too much emotion in what should be 100% technically driven. And again, this goes both ways. Being judgemental in a review (like on some of the link's slides) or being offended by a technical comment on the other side. Too much sugar coating is not helping anyone
>I think there is too much emotion in what should be 100% technically driven.
Assuming that the prospective solutions offered by each side both fulfill the basic technical requirements, the choice of two different engineering approaches would seem to be a 100% human decision about tradeoffs, no?
I don't expect a team that is fearful of code reviews to be the best work they can do. IMO this isn't about offense, it's about removing barriers and wariness around code reviews.
(But as you side, that goes both ways, to both the reviewers and reviewed)
The flip side of an author who is overly attached to his code is the reviewer who is overly attached to an excessively pedantic and dogmatic view of how things should be done. As you say, it goes both ways: the former should acknowledge valid technical criticism of his work, and the reviewer should refrain from using general arguments that are not relevant for the specific case.
"100% technically driven". That's a silly statement. You aren't dealing with objective facts here, it's opinions. Tastes. There are always egos, personality, and differing points of view.
I think a better view is art/music. When you go to art school, you learn how to take criticism well. You learn to make it about the work, and view it as a chance to learn, and get better. I try to nurture this attitude for reviews at my company.
Opinions, tastes are essentially bike-shedding in programming and reviewers shouldn't be asking it to be fixed, unless there is a rulebook or automated tool for the bike-shedding. Otherwise programming decisions should always be fact/experiment driven not opinions.
> The most toxic behavior is being overly protective or offended about your code.
I disagree with "most", but this is absolutely often an issue. Often people take criticism of their code as criticism of themselves and their abilities, which is always counterproductive.
> As a general rule I think there is too much emotion in what should be 100% technically driven.
In my experience, things that are 100% technically driven are vanishingly rare. Most decisions come down to balancing trade offs and opinions, after examining the actual facts and evidence at hand.
Regardless, people have emotions and don't always act fully rationally, despite concerted efforts to do so. That's just a fact of human nature, and we'd do well to design our professional processes with that in mind.
> I disagree with "most", but this is absolutely often an issue. Often people take criticism of their code as criticism of themselves and their abilities, which is always counterproductive.
I think it's easy to fall into "this comment is a value judgement on my skill/person, and not my code" if code review feedback is targeted at the person and not the code.
Hence the discouraged "why did you do this" form vs the more helpful "this might be better because" format.
Something that's been on my mind is the value of code reviews as a line-by-line exercise, vs the value of a code review as a round table.
I find line by line reviews useful for small PRs of limited scope (bug fixes, etc), but find it a difficult space to talk about things like approach, architecture, etc.
For the latter, I've found round tables where the dev in question have the time and space to explain their reasoning in a synchronous real-time manner to be valuable.
I find that focusing on negative behaviors and "what not to do" can have some undesired side effects. Namely, discouraging people from communicating because they don't want to hurt someone's feelings or undervalue their own ideas. If you want to leave a comment, there's probably a great reason! Shutting yourself down does nobody any favors. I liked that there were a few ideas about helpful behaviors at the bottom, but they were less detailed and didn't include examples in the same way.
Over-communicate, notice positives, and build a culture of trust, where feedback is seen as the gift that it is. Those are the secrets to great code reviews... IMO
I think it’s far better to develop a culture where code reviews are opinions and suggestions and if you disagree you can reply to a comment and discuss further. Using aggressive tone is not helpful for this, but documentation needed to correct any mistake is unhelpful as well. If it’s hard to ask for additional information or hard to disagree with a review comment that isn’t going to be solved by increasing the quality of review comments. In fact I’m more likely to be upset if I make a mistake in my code and someone assumes it’s intentional, explains why it’s a mistake, and then provides documentation to explain further. That feels like in bold, triple underline, all-caps text to me. From my perspective a better comment might be “Let’s discuss further as I prefer alternative.” Highlighted on appropriate line of code with a this made issue specific. A lot of the time the reaction will be ‘oh, oops, so do I.’ The other times you spend the time on a discussion rather than carefully crafting a written statement (which is far more time consuming).
These are good guidelines, I generally agree with all them (and I've messed up tons of times as well). It's important to note that you should write comments for the developer, whoever that is. That means understanding their perspective, their experience, and what they want.
Some developers really won't take offense to many of the "bad comments" suggested, and could even feel patronized if you take some of the guidelines - for example, over explaining something could be overkill for someone who just made a mistake.
I've also found that taking a lighthearted approach to code reviews really lightens up a stressful interaction. Simple jokes, comments like "dead code! woo!", and whatnot make a difference.
And lastly - don't forget positive comments!! Celebrate people's work when you can!!
While that is certainly excellent good advise it is centered on online code reviews, such as those in a git pull request. Although this advise is excellent, these aren't hard problems to solve. Particularly since if a submitter is out of line a collaborator or moderator can easily shut them down or close the pull request. Case closed (over simplification).
Far more challenging are offline code reviews where you have to deal with people face to face and contend with people's emotions. If everybody were mature and confident emotion wouldn't be a serious factor, but this isn't the case in the corporate world.
People are frequently guarded, offensive, or deceptive most usually without any awareness of their emotional communication. It takes a special kind of soft skill to get people to open up, be relaxed, and attain greater disclosure. This can be particularly challenging among young people who may be more emotional, face greater technical hurdles, and lack the communication experience to contend with emotional pressure.
I think these are hard problems to solve. We haven't solved them yet; the need for this sort of article is empirical evidence of that. Toxic review behavior continues to be a problem.
I think face-to-face code reviews are much easier, because when you're sitting across from a real human, you pay much more attention to the social aspects of the interaction than when you're hiding behind a screen. You can also adjust your manner, tone, and word choice in real time based on the verbal and non-verbal cues you get from being physically present with the person you're talking to.
I completely disagree. Listening to people is paramount, but the subject of the conversation is the code in review. If you are paying more attention to the communication than the code you have lost focus and are reviewing communication opposed to code.
I have been through this when people get hostile or defensive. They make their communication more important than the code.
I don't have to deal with any of that online. As a project maintainer if somebody is being hostile I have several ways to deal with them from a command perspective. If I am a contributor and the project maintainer is being hostile then I simply won't contribute to that project.
Also, online there is the convenience of time. You can take as long as you need to draft a proper well thought out response. Speed of thought in offline communication can cloud judgement and impose emotional distractions.
If you believe that how you communicate is irrelevant, then I don't think we're going to have a productive discussion here. Because it does matter, as much as you or anyone (or even I) think it shouldn't.
I find the use of "Toxic" to be somewhat inflammatory and hyperbolic. I would say this would more accurately be titled as "Improving your Code Review Interpersonal Skills".
I wholeheartedly agree with the suggestions -- they are all great ways to be sensitive to your fellow coworkers, and I tend to favor this style. However, I know a lot of great developers who have a more direct style of communication. Also, I know several people who might interpret similar comments as patronizing or passive aggressive.
In fact, the article seems to be more guilty of the transgressions discussed than the transgressions discussed.
For example: "passing opinion as fact". And of course, calling these things "toxic". In fact, I disagree. Sometimes a little tough-love is needed, and this is the code we are talking about, not the person.
I'd suggest the author look up "egoless programming". Her suggestions seem to pander to ego-full programming, and this will mask underlying issues and allow them to fester, causing significantly greater problems in the future.
UPDATE: And of course, something unqualified is always an opinion (for a hilarious artistic rendition of this point, see the judge on The Good Wife who requires lawyers to append “in my opinion” to everything they say)
Can you state specifically which of the transgressions this article mentions that you would consider appropriate tough love, and how exactly they improve the situation? Examples would help.
(b) I am not going to break it down for you like that, because that would not be useful, missing the forest for the trees.
I think the whole approach of the article is mistaken and harmful, particularly stated as absolutely, "toxically" and without evidence as it is.
I've participated in code-reviews in roles as contributor, manager and mentor, with widely different teams in a variety of companies. The best teams that produced the best code were the ones that were the most "ruthless". Now of course: "hard on problems/code, soft on people", but with the first part being the priority.
Because the machine doesn't care about your feelings, and if you screwed up, you don't benefit from not being told that you screwed up.
And by tightly constraining the format to "helpful suggestion", you lose the range of feedback, from "awesome" via "yeah whatever" or "maybe try this" to "nope" and "what on earth were you thinking?".
Because it's important to not forget that the range always exists, compressing it into a lossy channel just muddles it.
Good teams have the rapport that makes the entire range of feedback possible and helpful. If that's not possible, it's an indication of a problem in the team. The more I am exposed to those sorts of teams, the more "toxic" I feel they are.
It also incorrectly shifts responsibility for negative feedback from the cause (the bad code) to the bearers of the bad news, and with it the burden for avoiding it. In order to avoid negative emotions due to negative feedback, don't police the feedback, write better code!
(Of course that assumes the feedback is roughly appropriate and proportional, a nuance that the article completely misses)
And once again, that has to be tempered with your role. As a manager, for example, it is important to take yourself back a bit, to let people code in their style even if you think you can do better (and particularly if you know you can). As a mentor, on the other hand, you are specifically there to help people grow, so comments at the level of "yes, you could do it this way, but I think this might be better" are more called for, because they are there as helpful guidance, without the implication of "you must do it or else" that goes with the formal power position (and will be understood that way even if you don't mean it).
And the most important part is fostering a team culture where there is a level of trust that feedback can be as harsh as necessary, where people actually want the toughest possible feedback. To create the best code and the best coders.
I will say that my comment was an attempt at making your concerns more concrete because they honestly don't make a lot of sense to me, but I feel like you just made them even more abstract. Based on the content of your comment and the seeming certainty of your opinions, I suspect we probably couldn't disagree more.
I have made things more concrete, just not torn it into little unrecognizable micro-fragments that miss the point.
Can you be more concrete as to what you disagree with? So far, you have said nothing of substance, just asked me to do the work for you and then made a broad and dismissive claim.
I was interested in an answer to my question to understand your perspective. You won't give me an answer. That's okay, but so is my desire to set parameters on conversations that I engage in.
Those two are contradictory: the answers to your very constrained questions would not have given you my perspective. My answer gave you my perspective.
Anyway, I don't let others dictate "parameters" to me...as that is not a conversation. And as far as I can see, you are part of this problem.
I'm not sure if this is related but maybe egoless programming is what I am thinking about here... I felt uneasy reading the OP. I follow much of the advice myself, and I can see how it is necessary to be diplomatic when there are tensions in the team.
However, I much prefer it when people give me their raw and direct thoughts on the code. "indentation wrong", "really hard to follow this code", "why does this class exist?"; now we're two developers looking at code, and we both want it to be as neat as possible.
If I can tell that the reviewer goes out of their way to be diplomatic (e.g. by following the OP or by randomly praising code that is not broken), they're obviously worried about hurting me; and suddenly we are not managing my code, but me! Why? Am I on the brink of being fired? Did I ever overreact? Am I too attached to my source code? It also becomes impossible to tell how strongly the other side actually feels about their suggestions when everything is posed as a "what do you think" question.
It could be a cultural thing too? I'm German and American politeness often makes me paranoid because every flowery statement could be criticism in disguise.
What's interesting is that Americans don't notice/understand that their expectations are not universal in the least bit, but very, very culturally specific, not shared by most of the rest of the world and, quite frankly, rather broken.
Thanks for at least providing examples. From my perspective, you didn't really understand the OP. The point isn't to use "flowery" language. The point is to use more constructive feedback to avoid wasting valuable time and resources.
> It could be a cultural thing too? I'm German
No. I work with people all over the globe in open source. In my experience, humans everywhere appreciate helpful honesty.
I think that all your example comments are entirely appropriate. And, in the case of, "Why does this class exist?", I think it's extremely important that such questions are asked. In my opinion, knowledge transfer is one of the primary functions of code review in the first place. And that question is as much about knowledge transfer as it is about the code. Maybe there's a consideration that led you to create that class, and this is a chance for that other person to learn it. And maybe you will learn something back about another potential solution.
I think the real negative that needs to be avoided is using code reviews to attempt to force people to do things exactly the way you would do them. I think many of the articles points kind of go towards this, but indirectly. Diversity of thought and approach is a good thing, and learning to live code that's not done in the fashion that I think is ideal was a lesson that took quite a while for me.
EDIT: And, to be clear, "Why does this class exist?" is a potential lead-in to such a problematic viewpoint. The problem happens when, after explanation of the class' existence, the response turns into, "You should remove this class." That's where it transitions away from knowledge transfer and into forceful monoculture of approach.
I think 'toxic' is highly appropriate, I've quit one job in particular because of horrendous code review culture.
I think the article is spot on, on just about every point. But one point I believe is missing is a discussion of "appropriate level of abstraction". This article has a terrific discussion on this point[1].
> Even if a developer is extending or modifying a messy part of the code that is rife with bad practices, don’t ask the developer to fix them in that pull request just because their changeset happens to touch the messy code.
I don't think that's good advice at all. If you touch messy code have a go at fixing it. If you don't you'll just end up with increasing tech debt.
Not necessarily. If said refactoring requires very little work, then sure. But, as it often is the case, refactoring often takes a fair bit of work and probably more than expected. Thus, it is out of scope of the original PR/branch.
If the PR is for adding a feature, then focus on that. Refactoring is better served by having its own dedicated focus and branches, respectively.
People who keep getting deflected from fixing deeply troubling problems with the code tend to become unpleasant to be around after a while.
Edit: people who enjoy the status quo like to argue about doing it later and it can be exasperating and also a challenge to filter the people making a helpful objective decision from an obstructionist one.
As someone else said, it’s best to dig into a problem when you’re already there instead of having to come back.
There's little more irritating then pointing out something is going to bite you in the ass if not fixed, have it declared out of scope, and them be blamed when, as you predicted, it comes around to bite you in the ass. Especially when it makes it look like you're some amateur-hour bozo to a third party.
There is a spectrum. I find that when a side-refactor is too much work to tack on a ticket, it's helpful to instead ask for some smaller improvement that can help with cleanup in the future. E.g. add a line of logging that could prove down the line that a particular piece of code is not used, or switch to using a new field to let someone else delete the deprecated one later more safely.
Most of these points all relate to having empathy for the developer and being kind, and focusing on the point of code reviews (which is to improve quality and reduce ongoing costs).
Example:
> Unhelpful behavior: overwhelming with an avalanche of comments
Code that isn't consistent to standards absolutely needs to be fixed. And while it's a lot of work to keep code up to standard, the author is correct that the nitpicking approach is unproductive.
A better approach is to review code like this, see that it falls short, and suggest realistic (and in turn kind) ways to improve things. Picking at each missed space is clearly counter productive, and there are more standard and helpful solutions.
I don't think things like minutia fit in code reviews either, but the small stuff does matter.
I feel the same about the other suggestions, they can be boiled down to: don't be an ass; be kind; be constructive. Just imagine that you're reviewing your own code from 5 years ago.
She did suggest those things further down the article:
> Helpful Behavior: automate what can be
> Reviewing issues that can be caught by linters, git hooks, or automated tests are unhelpful because they often result in an avalanche of comments and come off as nitpicking. People are not particularly good at catching these issues, hence why automation tools exist.
I don't agree with making tickets to come back and fix things. Lots of times changing a line in a file points out how smelly it is. If it can be fixed there, I say give it a shot.
Sometimes "I'll open a ticket" is code for "I'd love to do this, but I'm never going to."
Translated again: “I’d love to be perceived as willing to do this, but I’m never going to.”
I got a big dose of this on a project. Requirements got hung up for weeks and the do it later crowd simply had no idea what to work on. To me this means all the stuff they “didn’t have time for” was stuff they couldn’t be bothered with.
If you don’t tell me what to work on I can think of ten different things that are about to break.
I think it cuts both ways. Yes, it's definitely easier to change code while you're already there, but on the flip side, it can potentially create a huge burden for the programmer. A simple change might turn into a multi-day slog when there are more pressing things that need attention.
I feel that this could be recontexualized for the person who is receiving feedback.
If you don’t take things pwrsonally, and you take things one at a time, there’s no reason to let yourself feel overwhelmed or attacked in many situations where at first you might be inclined to do so.
Specifically I disagree with point #2. I absolute love it when testing/QC labels every single instance of a minor mistake. It saves me from having to go through and find each place where it happened, and it lets me check off a little Asana task as complete each time I make a minor correction.
Feedback like “it looks like you made this mistake a number of times” can be just as condescending, with the added frustration that I now need to go try to find everywhere I might have made the mistake.
When they do the work of naming each instance, they are saving me time.
So I’m wondering how much of this is about putting the burden of not feeling attacked on the reviewer, and neglecting the role of the reviewee?
> Asking such questions implies that a perceived simple solution should have been obvious. [...] Instead, provide a recommendation [...].
> You can do ___, which has the benefit of ____.
I don't think that's safe either. If I use a char[] instead of the more obvious String for whatever technical reason, a comment like "You can use a String." would sound incredibly patronizing. The trouble is that the reviewer now has to guess what happened - did I intentionally choose a more convoluted solution, or was I honestly not aware of the straightforward way to do it? I've had reviewers who erred on the side of underestimating my knowledge and found it super awkward.
What helps as a reviewee is to always add a code comment explaining why you are not using the obvious tool for the job; both the reviewer and the next developer will thank you.
63 comments
[ 4.7 ms ] story [ 132 ms ] threadGenerally, I agree with breaking out bugs or issues that are found 'along the way' into separate subtasks or tickets. However, often what happens is a ticket is thrown into the backlog and not addressed.
Because the developer has just touched the code, this is often the best time to refactor as its still fresh in the mind, and this creates a culture of constant improvement. Of course everything depends, in this case, the complexity of the fix is the main limiting factor.
We truly believe in Refactoring along the way instead of trying to break out tech debt stories separately.
> However, often what happens is a ticket is thrown into the backlog and not addressed.
Every management method is different, but e.g. in Scrum (I think), there is the idea of a person reviewing and priorizing work tasks according to the roadmap. this person would decide if a "fix this messy code" task is important enough or not.
Then again, I think there is the every day a good deed philosophy of refactoring, too. I think in that case, one should just make sure that the "fix along the way" task doesn't disturb the original problems too much (so commits can still be reviewed) and that there are no unintended side-effects.
1. The person making the change to the code for their purpose may not have the required overall understanding to execute a refactor.
2. I believe in a One Change At A Time policy: I'd like to be able to, at least in theory, measure the effects of any change. If you slap two changes together, that becomes difficult or impossible.
3. The person making the change has a purpose in making that change, and likely has committed to some sort of schedule or timeline for getting the work done. Forcing them to play janitor could easily derail that.
4. The change at hand might just be a small piece in a larger body of work, and shifting focus to a refactor would be disruptive.
Having said that, I do often take the initiative in doing refactors when making a change, if it makes sense to me to do so. But I don't think it makes sense to push that expectation on others. Hell, it's possible/likely that the person making the change has noticed the opportunity for a refactor, but has decided against it for whatever reason.
I might suggest a refactor in a code review, but couched with language that it's entirely optional, and won't block merge of the change as-is if the author decides it's not the right call for them.
One change at a time has the benefit of not polluting traceability of what was done and why, in the ticketing system. If a commit for ticket1234 was to fix a bug in the payment system, and the commit has unrelated code fixing up user data access, that's bad in my opinion.
I've found myself in many code bases where the only trail of breadcrumbs I've had is ticket references in the commit messages. When these aren't reliable, you're left with not much at all.
This is such a good and deep point - being forced to defend your decisions is one thing - using a form such as "the whole world knows not to do this why don't you" is another.
i would argue that there is value in developing a longer for review comments - it's probably viable in fact - such that the comment is non judgemental.
It is perhaps mostly orthogonal to the specific phrasing used here, and certainly the word "just" isn't in and of itself bad. But my feeling is that my technical writing has improved dramatically if I carefully consider each use of it. It often exposes a perspective gap that I have with my target audience.
As a general rule I think there is too much emotion in what should be 100% technically driven. And again, this goes both ways. Being judgemental in a review (like on some of the link's slides) or being offended by a technical comment on the other side. Too much sugar coating is not helping anyone
Assuming that the prospective solutions offered by each side both fulfill the basic technical requirements, the choice of two different engineering approaches would seem to be a 100% human decision about tradeoffs, no?
(But as you side, that goes both ways, to both the reviewers and reviewed)
I think a better view is art/music. When you go to art school, you learn how to take criticism well. You learn to make it about the work, and view it as a chance to learn, and get better. I try to nurture this attitude for reviews at my company.
I disagree with "most", but this is absolutely often an issue. Often people take criticism of their code as criticism of themselves and their abilities, which is always counterproductive.
> As a general rule I think there is too much emotion in what should be 100% technically driven.
In my experience, things that are 100% technically driven are vanishingly rare. Most decisions come down to balancing trade offs and opinions, after examining the actual facts and evidence at hand.
Regardless, people have emotions and don't always act fully rationally, despite concerted efforts to do so. That's just a fact of human nature, and we'd do well to design our professional processes with that in mind.
I think it's easy to fall into "this comment is a value judgement on my skill/person, and not my code" if code review feedback is targeted at the person and not the code.
Hence the discouraged "why did you do this" form vs the more helpful "this might be better because" format.
I find line by line reviews useful for small PRs of limited scope (bug fixes, etc), but find it a difficult space to talk about things like approach, architecture, etc.
For the latter, I've found round tables where the dev in question have the time and space to explain their reasoning in a synchronous real-time manner to be valuable.
Over-communicate, notice positives, and build a culture of trust, where feedback is seen as the gift that it is. Those are the secrets to great code reviews... IMO
Some developers really won't take offense to many of the "bad comments" suggested, and could even feel patronized if you take some of the guidelines - for example, over explaining something could be overkill for someone who just made a mistake.
I've also found that taking a lighthearted approach to code reviews really lightens up a stressful interaction. Simple jokes, comments like "dead code! woo!", and whatnot make a difference.
And lastly - don't forget positive comments!! Celebrate people's work when you can!!
Far more challenging are offline code reviews where you have to deal with people face to face and contend with people's emotions. If everybody were mature and confident emotion wouldn't be a serious factor, but this isn't the case in the corporate world.
People are frequently guarded, offensive, or deceptive most usually without any awareness of their emotional communication. It takes a special kind of soft skill to get people to open up, be relaxed, and attain greater disclosure. This can be particularly challenging among young people who may be more emotional, face greater technical hurdles, and lack the communication experience to contend with emotional pressure.
I think these are hard problems to solve. We haven't solved them yet; the need for this sort of article is empirical evidence of that. Toxic review behavior continues to be a problem.
I think face-to-face code reviews are much easier, because when you're sitting across from a real human, you pay much more attention to the social aspects of the interaction than when you're hiding behind a screen. You can also adjust your manner, tone, and word choice in real time based on the verbal and non-verbal cues you get from being physically present with the person you're talking to.
I have been through this when people get hostile or defensive. They make their communication more important than the code.
I don't have to deal with any of that online. As a project maintainer if somebody is being hostile I have several ways to deal with them from a command perspective. If I am a contributor and the project maintainer is being hostile then I simply won't contribute to that project.
Also, online there is the convenience of time. You can take as long as you need to draft a proper well thought out response. Speed of thought in offline communication can cloud judgement and impose emotional distractions.
I wholeheartedly agree with the suggestions -- they are all great ways to be sensitive to your fellow coworkers, and I tend to favor this style. However, I know a lot of great developers who have a more direct style of communication. Also, I know several people who might interpret similar comments as patronizing or passive aggressive.
For example: "passing opinion as fact". And of course, calling these things "toxic". In fact, I disagree. Sometimes a little tough-love is needed, and this is the code we are talking about, not the person.
I'd suggest the author look up "egoless programming". Her suggestions seem to pander to ego-full programming, and this will mask underlying issues and allow them to fester, causing significantly greater problems in the future.
UPDATE: And of course, something unqualified is always an opinion (for a hilarious artistic rendition of this point, see the judge on The Good Wife who requires lawyers to append “in my opinion” to everything they say)
(b) I am not going to break it down for you like that, because that would not be useful, missing the forest for the trees.
I think the whole approach of the article is mistaken and harmful, particularly stated as absolutely, "toxically" and without evidence as it is.
I've participated in code-reviews in roles as contributor, manager and mentor, with widely different teams in a variety of companies. The best teams that produced the best code were the ones that were the most "ruthless". Now of course: "hard on problems/code, soft on people", but with the first part being the priority.
Because the machine doesn't care about your feelings, and if you screwed up, you don't benefit from not being told that you screwed up.
And by tightly constraining the format to "helpful suggestion", you lose the range of feedback, from "awesome" via "yeah whatever" or "maybe try this" to "nope" and "what on earth were you thinking?".
Because it's important to not forget that the range always exists, compressing it into a lossy channel just muddles it.
Good teams have the rapport that makes the entire range of feedback possible and helpful. If that's not possible, it's an indication of a problem in the team. The more I am exposed to those sorts of teams, the more "toxic" I feel they are.
It also incorrectly shifts responsibility for negative feedback from the cause (the bad code) to the bearers of the bad news, and with it the burden for avoiding it. In order to avoid negative emotions due to negative feedback, don't police the feedback, write better code!
(Of course that assumes the feedback is roughly appropriate and proportional, a nuance that the article completely misses)
And once again, that has to be tempered with your role. As a manager, for example, it is important to take yourself back a bit, to let people code in their style even if you think you can do better (and particularly if you know you can). As a mentor, on the other hand, you are specifically there to help people grow, so comments at the level of "yes, you could do it this way, but I think this might be better" are more called for, because they are there as helpful guidance, without the implication of "you must do it or else" that goes with the formal power position (and will be understood that way even if you don't mean it).
And the most important part is fostering a team culture where there is a level of trust that feedback can be as harsh as necessary, where people actually want the toughest possible feedback. To create the best code and the best coders.
See also: The Coddling of the American Mind https://www.theatlantic.com/magazine/archive/2015/09/the-cod...
Can you be more concrete as to what you disagree with? So far, you have said nothing of substance, just asked me to do the work for you and then made a broad and dismissive claim.
Anyway, I don't let others dictate "parameters" to me...as that is not a conversation. And as far as I can see, you are part of this problem.
However, I much prefer it when people give me their raw and direct thoughts on the code. "indentation wrong", "really hard to follow this code", "why does this class exist?"; now we're two developers looking at code, and we both want it to be as neat as possible.
If I can tell that the reviewer goes out of their way to be diplomatic (e.g. by following the OP or by randomly praising code that is not broken), they're obviously worried about hurting me; and suddenly we are not managing my code, but me! Why? Am I on the brink of being fired? Did I ever overreact? Am I too attached to my source code? It also becomes impossible to tell how strongly the other side actually feels about their suggestions when everything is posed as a "what do you think" question.
It could be a cultural thing too? I'm German and American politeness often makes me paranoid because every flowery statement could be criticism in disguise.
> American politeness
What's interesting is that Americans don't notice/understand that their expectations are not universal in the least bit, but very, very culturally specific, not shared by most of the rest of the world and, quite frankly, rather broken.
> It could be a cultural thing too? I'm German
No. I work with people all over the globe in open source. In my experience, humans everywhere appreciate helpful honesty.
I think the real negative that needs to be avoided is using code reviews to attempt to force people to do things exactly the way you would do them. I think many of the articles points kind of go towards this, but indirectly. Diversity of thought and approach is a good thing, and learning to live code that's not done in the fashion that I think is ideal was a lesson that took quite a while for me.
EDIT: And, to be clear, "Why does this class exist?" is a potential lead-in to such a problematic viewpoint. The problem happens when, after explanation of the class' existence, the response turns into, "You should remove this class." That's where it transitions away from knowledge transfer and into forceful monoculture of approach.
Directly saying what changes might improve the code, and why the code is better with those changes is direct communication.
I'll admit to being guilty of some of these on occasion; must do better.
I think the article is spot on, on just about every point. But one point I believe is missing is a discussion of "appropriate level of abstraction". This article has a terrific discussion on this point[1].
[1] https://dev.to/lpasqualis/the-5-problem-solving-skills-of-gr...
I don't think that's good advice at all. If you touch messy code have a go at fixing it. If you don't you'll just end up with increasing tech debt.
If the PR is for adding a feature, then focus on that. Refactoring is better served by having its own dedicated focus and branches, respectively.
Edit: people who enjoy the status quo like to argue about doing it later and it can be exasperating and also a challenge to filter the people making a helpful objective decision from an obstructionist one.
As someone else said, it’s best to dig into a problem when you’re already there instead of having to come back.
Example:
> Unhelpful behavior: overwhelming with an avalanche of comments
Code that isn't consistent to standards absolutely needs to be fixed. And while it's a lot of work to keep code up to standard, the author is correct that the nitpicking approach is unproductive.
A better approach is to review code like this, see that it falls short, and suggest realistic (and in turn kind) ways to improve things. Picking at each missed space is clearly counter productive, and there are more standard and helpful solutions.
Why not suggest adding:
- lint tools - requiring code meets standards - auto formatting tools
I don't think things like minutia fit in code reviews either, but the small stuff does matter.
I feel the same about the other suggestions, they can be boiled down to: don't be an ass; be kind; be constructive. Just imagine that you're reviewing your own code from 5 years ago.
> Helpful Behavior: automate what can be
> Reviewing issues that can be caught by linters, git hooks, or automated tests are unhelpful because they often result in an avalanche of comments and come off as nitpicking. People are not particularly good at catching these issues, hence why automation tools exist.
Sometimes "I'll open a ticket" is code for "I'd love to do this, but I'm never going to."
I got a big dose of this on a project. Requirements got hung up for weeks and the do it later crowd simply had no idea what to work on. To me this means all the stuff they “didn’t have time for” was stuff they couldn’t be bothered with.
If you don’t tell me what to work on I can think of ten different things that are about to break.
If you don’t take things pwrsonally, and you take things one at a time, there’s no reason to let yourself feel overwhelmed or attacked in many situations where at first you might be inclined to do so.
Specifically I disagree with point #2. I absolute love it when testing/QC labels every single instance of a minor mistake. It saves me from having to go through and find each place where it happened, and it lets me check off a little Asana task as complete each time I make a minor correction.
Feedback like “it looks like you made this mistake a number of times” can be just as condescending, with the added frustration that I now need to go try to find everywhere I might have made the mistake.
When they do the work of naming each instance, they are saving me time.
So I’m wondering how much of this is about putting the burden of not feeling attacked on the reviewer, and neglecting the role of the reviewee?
> Why didn’t you just do ___ here?”
> Asking such questions implies that a perceived simple solution should have been obvious. [...] Instead, provide a recommendation [...].
> You can do ___, which has the benefit of ____.
I don't think that's safe either. If I use a char[] instead of the more obvious String for whatever technical reason, a comment like "You can use a String." would sound incredibly patronizing. The trouble is that the reviewer now has to guess what happened - did I intentionally choose a more convoluted solution, or was I honestly not aware of the straightforward way to do it? I've had reviewers who erred on the side of underestimating my knowledge and found it super awkward.
What helps as a reviewee is to always add a code comment explaining why you are not using the obvious tool for the job; both the reviewer and the next developer will thank you.