Unlearning Unhelpful Behaviors in a Code Review Culture
medium.com
medium.com
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.
> 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.
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.
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.
> 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...
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.
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
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.
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?
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.
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!!
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.
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.
> 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.
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.
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.