> Critique code instead of people – be kind to the coder, not to the code
A fellow co-worker taught me by example to try to always phrase each code review comment as a question.
Instead of
> This loop never terminates
Write
> Does this loop terminate?
Instead of
> There's no test for this condition
write
> Is there supposed to be a test for this condition?
I'm probably not thinking of good examples but for some reason, more often than not, the direct comments came across as "you screwed up!" where as the questions instead made it feel like I was discovering my own mistakes rather than have someone tell me I made one.
It's probably subtle and maybe some people will think it's dumb but I really appreciated it. It felt like it went a long way toward me feeling like saw my own mistake and fixed it before I checked it in instead of the direct approach which made me feel stupid and embarrassed.
I've tried to adopt that approach (though it's a struggle to remember to apply it).
I feel obligated to mention that this strategy, while it often works to some degree, sometimes doesn't work. For people like me, phrasings like that sound passive-aggressive.
I prefer the strategy of wrapping my sentences in things like, "I think" or "I prefer".
So, I'd write, "This loop doesn't seem to terminate." or "I think that this condition needs a test." It's an implicit admission that I could be wrong and an invitation to push back. Just as often, I'll walk over and say, "Could you explain why you did that to me?" This is especially acceptable when it's agreed that a good review requires the reviewer understand what's going on.
I feel the way I communicate has been influenced strongly by voice training - I've noticed in particular I say "I feel" a lot in similar circumstances. (Apparently, valuing emotions above thought is some paragon of femininity? I'm not sure I agree with that.)
I am reluctant to go for the direct "wtf?!" response, because as above, it bruises egos and people get defensive - and that is unhelpful to actually resolving issues. I tend to phrase it as "Perhaps I'm confused, but...". (One advantage of this is that sometimes, I am wrong, and this gives someone an opportunity to explain why, and in the process hopefully re-examining their thought process and testing it just in case.) I think I caught that pattern, as well as excessive usage of "though", from a good friend. One interpretation of this is that perhaps I've spent nigh-on 2½ decades being confused. <g>
There is, however, a place for the genuine WTF. I reserve it for "there's no way you could be that stupid" situations, like someone cheerfully telling me about the 256-bit encryption they used, and my discovering it's RSA-256.
The management-by-Perkele has however seemed to work to keep the Linux kernel clean up 'til now, although they don't really want to insult people.
The success of the approach mentioned (ask instead of assert) depends a lot on the culture of the receiving developer. Also, if there's a language barrier, being subtle can actually muddy the waters instead of help the situation.
You can phrase things directly, even adding minimizing language (to save face), but be clear about the problem during the review.
>For
people like me, phrasings like that
sound passive-aggressive.
I second this notion. When you have doubt whether a loop terminates the question is appropriate but next to something as obvious as "for(;;) {}" it will simply sound insulting to the recipient's intelligence.
I do this too, and I also overuse the phrase "I speculate" to tag when I'm, well, speculating, I wish other people did too because too often people unintentionally present their speculations as fact
Also, think that the programmer might have already thought of that issue. Does that loop really never terminates? Isn't that test obvious/really needed?
But in all seriousness, projecting issues as character flaws is a really destructive thing to do, not just in programming but in any relationship. It's a really good habit to avoid it whenever possible.
I read somewhere (I think it was Blink) that marriage counselors could tell which marriages would fail by observing those that generalized around their spouses character rather arguing about the issues, ie "You always..."
Siracusa said something similar on Accidental Tech Podcast a while back, and it really struck me because it made me look back on every argument I've ever had in a different light. He said he always phrased disagreements in such a way as to leave the person he was disagreeing with out of it. In general, always voice disagreement as "the thing you said is wrong" rather than "you're wrong". The former is about the point of disagreement, the latter is about the person.
It's not even about being diplomatic or being less likely to hurt someone's feelings (although you get that for free), it's really just about clarity as to what's being discussed. The most confident, secure person in the world might still get defensive when confronted with "you're wrong", because there's still a whiff of it being about questioning that person's credibility, as opposed to the thing that you think they're wrong about.
That's very true! I've found that the key to having a constructive argument is always doing your best not to hurt their ego, assign blame or back them into a corner where they'll lose face (always leave an out). Otherwise they'll slip into "defensive mode".
That together with the old chestnut "seek first to understand then to be understood" to ensure that you respect and understands their position are real game changers.
Its funny and scary how important these kind of social and interpersonal skills are working in any corporation. Yet often are lacking and leads to a lot of unnecessary friction.
It's something I almost wished was taught in schools, if only so they could recognize the patterns later when they go into working life.
I was curious about what I read in Blink so I looked it up and it seems to be a bit of the usual Gladwell hyperbole even if it was based in some kind of studies. Needless to say marriages probably can't be simplified that much but it still is a pretty good general rule
I follow this advice because I've found from experience that often there is a good reason, or at least _a_ reason, that I don't see. Maybe the loop does somehow terminate, through an obscure mechanism. Or maybe it's not supposed to terminate. Or maybe I missed something obvious.
I make mistakes all the time. I probably make mistakes more often than I do things right. Therefore, asking questions rather than making bold claims is usually betting the right way.
This doesn't mean you have to be a pushover. If you don't see something, you can say, "I'm sorry if I'm being a little slow here, but I still don't understand. Can you explain how XYZ works in more detail?" Most people are very happy to help you understand.
> I follow this advice because I've found from experience that often there is a good reason, or at least _a_ reason, that I don't see.
Exactly. I pulled in devs from two teams recently to discuss confusing modelling of domain objects on one end of our app vs. the other end, as my team were trying to deal with both. The naming involved was completely contradictory, which always makes fitting a system into your head so much harder when you have to mentally translate.
Turns out that each team had very valid reasons for modelling the entities as they did - the front-end team implemented the entities as understood by the business guys who would be using the GUI to configure them, and the other implemented the entities as understood by the API spec they were ultimately being transmitted over.
The initial response of "this is so dumb" was very easy and very wrong. Much of my personal development as a dev has been learning to suppress that initial dismissive response when encountering 'bad' code, instead asking why the code is as it is.
There's always a bit of a fundamental attribution error[1] in "your code is awful" discussions with devs.
(Although I'd say that this points to the issues that can arise from having two teams implement two separate parts of a business process.)
This, a thousand times, but I believe it also requires an intention of humility behind the question. There are still plenty of ways to phrase a critique as a question and come across as a jerk. People who are intentionally jerks will find a way to do it no matter what, though.
Asking a question leaves open the possibility that you, the person issuing the critique, are wrong. From my experience direct factual critiques are fine as long as they're correct. Some people might mind being told that a loop never terminates (even when that is trivially the case), but those people have their own ego issue to work on.
I feel the issue comes when your critique is either flat out incorrect or when it misses the context of why seemingly incorrect behavior might actually be desirable. Asking it as a question is an implicit exercise of the 'reasonable person principle'. That tiny shift in voice turns an accusation into a conversation that effectively starts with "My expectations have been violated by what I'm reading here. Have I missed something or was this intentional? If it's intentional, can you help me understand why this is desirable behavior?"
I've encountered engineers who takes things too literally for this to work:
Me: "should this be documented in the team wiki?"
Engineer: "Probably"
"should we document it now before we forget?"
"Okay"
"um, do you want to do it? I could do it for you after I'm done with my tasks if you need help"
"okay, you do it"
whoosh hint not taken. It's not rare either since the stereotype of socially awkward literal engineers is not entirely undeserved.
That's different. You're describing an attempt to get a particular answer to a completely subjective question via hinting, whereas in the GP comment the recommendation is to ask questions about matters which have objective answers (e.g., "does this loop terminate?").
Don't volunteer for that! Of course they will say yes, just as if someone asked if you wanted them to document something, or just as customers always say yes if you ask if they want a feature!
Just tell them, "... You should document this in team wiki before we merge this."
One approach I took in a recent project was keeping the documentation in documentation folders under each project, using markdown.
It was good because documentation was tied to the source control and easy to locate and read when you were working on the project.
It's easy that wiki's become neglected and forgotten if there's not someone that "cracks the whip" so to speak so I think any way to lessen the friction and bring it into the coding environment is a good one
Pointing out a bug in the code is not a comment on the programmer. Programs have bugs! Humans make mistakes. Commenting on a review with 'you made a mistake' is needlessly confrontational. but the alternative is wasteful: bending over backward to avoid even suggesting that the code might have a bug. If the reviewee's ego is based on never making mistakes, then the review will hurt their feelings regardless of how the comments are phrased.
Yes, that is exactly what I do too. When I just started I treated code reviews as a way to show my knowledge and feel superior, which reflected the environment I was in. Once I moved to a new job with a jelled team and a positive sharing environment I noticed the use of questions in CR, and now it is my default mode.
If there is an oversight your part, the amount of rebounded embarrassment is far smaller. Bad feelings on both sides are minimised no matter what the outcome with your suggestion.
If you continually do this though it could certainly come off as passive aggressive. I feel there's a line to draw; if you have 100 different items to go over, it's going to quickly become annoying if all 100 are phrased as questions. Sometimes you just need to say 'this should be in a function.'
I think the key is that if you ask a question, you should be willing to accept "no" as the answer. Sometimes the answer is "not now" or "no", and perhaps adding a TODO or a comment explaining why not.
Particularly well-explained code shouldn't raise any questions.
I suppose so. Most often I don't harp on stylistic choices that don't affect the integrity of programs. I might encourage another coder to lint their code, for example, but I won't specifically single any of those out. I just won't waste time asking questions if I find ten places the program could error out on.
I agree. My preferred phrasing uses “we” (we are working on this code together) instead of “you” (you are working on this code and I am telling you off) and is usually a sort of polite suggestion with concrete justification. For example:
Another thread could have modified ‘x’ between when we test the condition and when we take the lock, so we should test that ‘x’ is ‘NULL’ if ‘y’ is true here, and if not, sleep and retry.
I've been criticized as being too harsh in code review before, and I'll try this.
I'm not sure how to apply it in all cases though. Frequently the issues I see are coding standard violations... use of exceptions, bad use of virtual or function pointers, ...
How do you say 'we dont use throw here' as a question?
You can ask a leading question or series of questions about what bad thing(s) might happen down the road as a result of what they did - which could hopefully lead to them having a greater understanding of the 'why' of the best practice, and less likely to repeat it in the future.
Well, the biggest why of exceptions is that we want to be able to compile with exceptions turned off.
And there are lots of reasons why not to use virtual functions or function pointers, but aside from 'it kills performance if used too much', they're fairly subtle. But really, the issue is typically with something like this the solution is 'rewrite it', which is brutal no matter how you phrase it.
Something like a period of pair programming/mentorship might ease the transition (we only do code reviews on new hires), but it's tough to justify.
If they really need dynamic dispatch, we prefer using an enum because of serialization, efficient sorting by type, code readability (this is debatable), etc., but really even this isn't what we want, because most of the time we don't need dynamic dispatch. Doing this wrong in one place is unlikely to be a huge deal, but if we can't trust them to write code that performs well... it's bad.
I would add that it is not only useful to phrase your criticism as a question, it is also useful to try to think through the possible answers, because there may be a logical one.
Doing this can save a lot of embarrassment as well; it can be a rotten feeling to give a harsh assertive criticism, only to find out that you were completely incorrect (not that has ever happened to me!).
Moderators at research and question social networks need to learn to critique and codify questions instead of people, and repress the aggressive tendency to escalate people based on how intelligently they form terms alone against help.
Always good to see Jerry Weinberg's work mentioned. I tried reading 'The Psychology of Computer Programming' when I was in college, it didn't grab then. Finally read it 15 years later, and realized that the material there would've especially helped in my first couple of jobs.
Also... my dad was 45 when I came along. I didn't get to spend much time with him before he was gone, but every moment was valuable and is cherished. I still try to make him proud, and suspect your dad would be proud.
The best part of this list is that with some tweaks, the commandments can pretty much apply to any field of personal and professional pursuit.
One rule I might add (and one that is also easy to apply elsewhere): Remember that far more time will be spent reading your code than writing it.
Grokking this not only has practical implications for how you write code (suddenly, the importance of style guides becomes clearer), but it makes you realize that this is how projects get built: programmers re-using libraries that contain encapsulated, clear functionality. Realizing that someone has to read your code -- even if that someone is mostly just you -- requires a modicum of empathy...and more often than not, this aids in the design process.
A common complaint about learning via something like Codecademy is that you might learn the syntax and how to solve problems...but you don't get much guidance on how to apply that knowledge to real projects. I think part of this is because with Codecademy, you're only writing code for the auto-checker to compile and test. Once you start writing code for humans to comprehend and re-use, it becomes easier to see how code becomes a real-world project.
Jerry Weinberg wrote those "commandments" in 1971. (It's sad that the article gives a link to his book but never mentions his name.) It's interesting how well they've kept their relevance over the years, despite the huge changes in the practice of programming. However, we who do the programming still keep on forgetting these things and need to be periodically reminded of them.
I have these on display at my desk. I've always wanted to know how to encourage cultural adoption? What are the arguments against these rules? There must be some.
My reasoning about these rules is that they derive from the idea that as teams of programmers, we're professionally motivated (and being paid) to produce the highest possible quality results. To that end, these rules compensate for human biases and faults to maximize quality.
Many of the practices you've read about some companies enforcing, such as "there are no bad ideas" or "no stupid questions" can be related back to these rules under the same reasoning. None of us do a perfect job of compensating for our human faults and biases, so some rules help to improve our performance.
My dad also passed away very early, soon after I finished college, and I credit him for making these rules so easy to follow.
> What are the arguments against these rules? There must be some.
I don't actually find this set terribly objectionable, at least when they're treated as guidelines rather than rules, but I'll have a go anyway.
Excessive focus on issues like this is seen as a symptom of a managerialist culture. While few developers I know would object to the ideas that everyone has something to learn, and that apprentice and journeyman coders should be supported and encouraged, there is -- sometimes justified -- suspicion that the real agenda is to commoditise developers and turn programming from a craft into a factory-floor activity. Or, to refer to another article that's on the front page this morning, to turn antiwork into work.
The kind of things that get put in place to "compensate" for ego are rules which value maintenance over re-writes, and third party components (even if they're only a tenuous fit for the problem) over in-house development. While I agree that there are biases to be aware of here, dismissing every proposal for a re-write or new internal development as egotism ends up shutting down discussions about the actual trade-offs.
>While I agree that there are biases to be aware of here, dismissing every proposal for a re-write or new internal development as egotism ends up shutting down discussions about the actual trade-offs.
I'm not sure how this behavior could be attributed to any of these rules. Could you elaborate further?
When I read things like this, it strikes me particularly that my dad is one of those rocket people the author is talking about. I'm glad he's done what he has to put things in space so that maybe someday people can go places other than the Earth on a slightly more than part-time basis.
All dad-boosting aside, this is a brilliant list, and I'm glad to have it, OP!
I don't think egolessness means not participating or taking the lead. See the point about not being the programmer who only comes out of the office for soda. If you take the lead and deliver good results, you can definitely be promoted, especially if you have good social skills. Also, people definitely notice when developers work on their own time to improve their skills. They may not say anything, but they notice, and that translates to promotions.
Depends on your employer. At mine, people who can work well with other developers and help the team get better overall are very much noted, appreciated and rewarded.
But our culture has always been about the team, not individual developers.
Is "team lead" a real position defined by HR that comes with a pay raise higher than whatever the most senior dev title is? If it is, that's awesome. I've never actually seen that in any place I've worked. It's always more responsibility without pay, which I consider a net negative.
'No matter how much “karate” you know, someone else will always know more.'
As stated, there is always at least one person for whom this is not true. However, even for such a person it is likely the case that there are still people who know things they don't, and the rest of 3 continues to hold:
'Such an individual can teach you some new moves if you ask. Seek and accept input from others, especially when you think it’s not needed.'
Am I really that rare that I treasure people above all others who will speak to me directly if they don't like something?
With the themes in this article and others like it, are they how you should behave if you want to meet your own goals, or about how people you're subordinate to prefer you behave, in order to not rock the boat too much?
The thing is, EVERYBODY thinks this about themselves by default, not just you. Nobody is sitting there thinking that they wish people would just sugarcoat everything for them and be as passive-aggressive and indirect as possible. Likewise, everybody probably feels like they have to be this way when interacting with other people, and that that can be burdensome sometimes.
It can be very, very different when you're actually in that situation though, and your boss is giving you the Torvaldsing that you always thought you wanted. If there is even a teensy part of you that thinks your boss is a jerk, or that they don't respect you, or that you know more about whatever it is that they're Torvaldsing you about, that part of you is being given a golden opportunity to assert itself really hard. Maybe it will, maybe it won't, but it's human nature and why spark the flint if you don't have to. I thought I was completely immune to this kind of defensiveness for most of my life, and it wasn't until I got into my 40s that I realized that it was always there.
Another way of looking at is, what is gained by being as blunt as possible, as opposed to the rules in this article? I'd say nothing. Nothing recommended here makes communication any more ambiguous or time-consuming. In fact it's about being more clear about what exactly you are discussing.
There are different approaches to being direct - one can be brutally blunt while still having the recipient maintain dignity. I prefer this approach in general - the trick is if the person does not take it well, then you stress that it is not personal, or meant that they are necessarily a bad developer.
It is shocking to be on the receiving end, but it forces the person to think about it more and internalize it better. I have seen it give great results for people's subsequent code, including my own. The developers have all thanked me for it and feel the approach has helped them grow immensely.
Egoless programming is about accepting that you are flawed, your knowledge is flawed and your work is flawed, and then drawing appropriate conclusions from that. That's how you get to "critique code, not people", because everyone is flawed, and almost all work is flawed, so there's no point blaming people for their flaws. The focus should be on the flaw and how to get rid of it, not the person whose flaw it is.
You are indeed rare for being able to take direct criticism of your work and respond in a constructive way.
> Am I really that rare that I treasure people above all others who will speak to me directly if they don't like something?
You could well be. People vary. One senior dev I work with can never accept direct criticism, he just shuts down and gets very defensive and adversarial.
But then another senior dev's preferred mode of discussion is yelling and hand-waving, and it took me a bit to learn that I had to yell and hand-wave back to get him to listen to me.
When I first became involved in programming as a career, I got this unexpected and uneasy impression that people in the industry (extrapolated from my real-world experiences early in my career) were generally passive aggressive about how they handled conversations about code and how they agreed or disagreed with others about how the code should "be". I think those groups of people would have benefited greatly from this list. Simultaneously it would have prevented alot of the angst I experienced as a young developer.
A little late to the party, but I would like to ask:
is there still some research going on in the areas of psychology and programming? Not AI, but how the state of the programmers mind affects collaboration, the quality of the code and the general outcome of projects?
Any hints and links would be very appreciated, thanks!
It seems to me this list, while having positive ideas, reeks of condescendence. it's mostly about how you are "much better than everybody, that's why you have to play cool with everybody"
What is missing is a healthy dose of "why do you think your ideas are better than everybody else's?"
Readability is mostly subjective. Code quality, maintainability, etc, are mostly subjective as well.
So don't give me that BS "I'm just trying to get the best code possible" when it's your opinion
Sure, I'm not saying that something like xx = strinvcatpfr(yy * zky_eng) can't be improved, but beyond a certain point it's mostly about taste and endless nitpicking
I wouldn't say that those things are "mostly" subjective, that sounds like a defensive argument.
I'm pretty sure there are good objective metrics both for readability and maintainability, they might not be the only aspects but calling it subjective is going too far.
For instance maintainable code could be said to minimize side effects, be somewhat modularized and be as compact as possible while being readable. I find it hard that anyone could claim the opposite and be right "as a matter of taste"
"I'm pretty sure there are good objective metrics both for readability and maintainability, they might not be the only aspects but calling it subjective is going too far."
I think you explained better what I mean.
I'm not denying the good objective metrics (for example, one of the aspects would be following Pep8 in Python)
But beyond that it's mostly about subjectivity.
So, evaluate on the objective metrics as much as possible, but don't waste my time saying that "A is better than B" when it's mostly a subjective aspect
This is very true. My brother just told me about a class he took on C# in which the teacher gave him a C on an assignment because of 2 things: 1) he had a using statement that reached far into a library, and 2) he used the "var" keyword. The program was correct otherwise.
A fellow co-worker taught me by example to try to always phrase each code review comment as a question.
Instead of
> This loop never terminates
Write
> Does this loop terminate?
Instead of
> There's no test for this condition
write
> Is there supposed to be a test for this condition?
I'm probably not thinking of good examples but for some reason, more often than not, the direct comments came across as "you screwed up!" where as the questions instead made it feel like I was discovering my own mistakes rather than have someone tell me I made one.
It's probably subtle and maybe some people will think it's dumb but I really appreciated it. It felt like it went a long way toward me feeling like saw my own mistake and fixed it before I checked it in instead of the direct approach which made me feel stupid and embarrassed.
I've tried to adopt that approach (though it's a struggle to remember to apply it).