[WORK IN PROGRESS] Code review guide #55
Conversation
| Code Review Mindset | ||
| ------------------- | ||
|
|
||
| It's important to separate our self-worth from the code we write so that we are always open to feedback that will help us improve. |
There was a problem hiding this comment.
This appears to be a paraphrasing of a main point from Weinberg's Egoless Programming (1971). Jeff "CodingHorror" Atwood extracts 10 "commandments" here. I think it's an extreme oversimplification to extract just this one principle and omit others. In particular, the point summarized in the tenth commandment is:
Critique code instead of people – be kind to the coder, not to the code. As much as possible, make all of your comments positive and oriented to improving the code. Relate comments to local standards, program specs, increased performance, etc.
On the other hand, Atwood also mentions how egoless programming has been considered a fallacy or myth. The tl;dr here is that we want people to be personally invested in (to own) their work: we want their passion, commitment:
if you do want great software, you have to let the developers own what they're building. The developers are inevitably the ones who have the most control over the success or failure of the project. Creating an environment where your developers have no emotional attachment to the project they're working on is a recipe for mediocre software-- and job disillusionment.
So the paradox of peer review is that we expect developers to own/invest/care deeply for the work they do, right up until the point that their work is reviewed, when they should magically adopt a zen-like disinterest in having it reviewed. I submit that this is unreasonable, unless we're very careful about how reviews are done, and what reviewing is.
Karl Wiegers wrote an excellent book in 2001 called Peer Reviews in Software: A Practical Guide which looked at the psychological aspects of peer review. Chapter two is available online, and here's a relevant quote (my emphasis):
The dynamics between the work product’s author and its reviewers are critical. The
author must trust and respect the reviewers enough to be receptive to their comments.
Similarly, the reviewers must show respect for the author’s talent and hard work.
Reviewers should thoughtfully select the words they use to raise an issue, focusing on
what they observed about the product. Saying, “I didn’t see where these variables were
initialized” is likely to elicit a constructive response, whereas “You didn’t initialize these
variables” might get the author’s hackles up. The small shift in wording from the
accusatory “you” to the less confrontational “I” lets the reviewer deliver even critical
feedback effectively. Reviewers and authors must continue to work together outside the
reviews, so they all need to maintain a level of professionalism and mutual respect to
avoid strained relationships.
The reason that I quote these at this point is because where I have in the past seen code reviews go quite spectacularly wrong, it's been because of (1) a lack of attention to these points of personal interaction, and (2) reviews focusing on subjective attributes of code style or system design. The two factors multiply together and have bad effects:
- the review drifts off from looking for bugs in the code into abstract discussion of points of style, practice or formatting (which are generally not decideable in the scope of a code review, if ever)
- because the discussion ceases to be based on objective truths (this code has no test, this code is brittle, this exception isn't caught) it becomes an argument, and humans (especially male humans) feel that arguments need to be won. Objective issues with code are much, much easier to accept.
Above in this document, the statement is made that The goal of review is to ensure that changes are delivering business value to our customers - not to perfect our code. For me, this (excellent principle) means that reviews are intended to spot defects and ensure that code meets the business-value requirements: and in particular, that subjective opinions about how code should be written are not in agreement with that goal.
tl;dr - I don't think we pay nearly enough attention to the personal interaction aspects of code review, and subjective opinions exacerbate this problem because they change discussion into argument.
There was a problem hiding this comment.
Thanks @benlast. That's excellent feedback and I appreciate the thoughtfulness and the comprehensive references. I'm far less eloquent in providing my own thoughts but hope that I am able to phrase them appropriately.
I definitely agree with your point that the personal interaction is the most important part of the review and will most likely decide the tone and perception of the feedback provided. I think it is something that we should clarify in the list of bullet points above. We should make it clear that reviewing a person's code means criticizing their work and each of us handles criticism differently. This will have to be an improvement process for both the reviewer and reviewee in which they work together rather than compete. That's at least the thought that I have.
Do you have any suggestions on how to clarify this in the above guidelines?
I also think you have a valid regarding the subjective opinion on how code should be written. I think a code review should mainly be about its suitability to the problem it is trying to solve and the pros and cons of the chosen implementation over alternatives. What I am not quite able to gather from your comment is, what your thoughts are on qualitative measures of code such as maintainability and best practices? Taking Python as an example, the community considers certain patterns Pythonic and others not. Would you consider these as subjective opinions?
I personally think that the line around where subjective opinion starts or ends is quite blurry. And I personally think maintainability and - in the case of Python - writing idiomatic Python are important parts of a code review. However, I think the extend to which they factor into a code review is to be determined by the group of people carrying out the reviews between themselves.
And as such, I think developing guidelines that we as Mobifiers agree on as best practice will make it clearer what is considered objective or subjective. Which is the reason why I'd like to see us all work together on developing these guidelines and establishing them across our various code bases.
Overall, I state my position on subjective opinion on code and code style along the lines of the last section:
If you disagree with a guideline, open an issue on the guides repo rather than debating it within the code review. In the meantime, apply the guideline.
I think we should establish some guideline that we all agree on. Until they are established, we apply the ones that we have.
Does that make sense to you and how does that overlap with your position and thoughts? How do you think should we change this suggested document for you to be comfortable with it?
Also thanks for the references above, I'll definitely add them to my reading list. Especially the Peer Reviews in Software sounds pretty interesting.
There was a problem hiding this comment.
Hi Seb. That's a great response: thank you!
On "a code review should mainly be about its suitability to the problem it is trying to solve and the pros and cons of the chosen implementation over alternatives" - I 100% agree completely with this statement.
On "developing guidelines that we as Mobifiers agree on as best practice will make it clearer what is considered objective or subjective" - I also agree with this, with a small caveat. The more rules we add, the more complex the process of following those rules becomes. @dbader and I have been using PyLint and ESLint on the web-push code, and it drastically simplifies the process of having code syntactically compliant. I'd summarize it as if the linter complains, fix it, otherwise it doesn't matter.
Sometimes code reviews frustrate me. For instance, where the process of getting working code rolled out to production is blocked by an apparently endless series of requests for minor tweaks that demonstrably have no bearing on whether the code is a reasonable solution to the problem. Or at other times (as we've seen), the effort that should go into attempting to find actual issues is instead diverted into arguments about whether one style or another is more "readable", as though readability was not subjective. Reviews like that, at their worst, damage relations between developers, affect morale and reduce our productivity.
Lastly: "We should make it clear that reviewing a person's code means criticizing their work...". I don't agree with this, because it shouldn't be about criticizing. Reviewers help the author find defects in the code. That's not criticizing, it's assisting, providing an alternative point of view, asking for clarification. It's extremely difficult to criticize someone's else's work in a way that you can be sure won't have a negative effect. It takes time, care and effort to follow rules (e.g. phrase your observations as questions, not orders, start comments with "I feel", and so on). Better to adopt the position that reviewing is helping, not correcting.
|
Additional comment... taken from a StackOverflow answer: Some objective study (Fagan, etc.) and a lot of popular wisdom suggests that peer relationships facilitate code reviews aimed at reducing bugs and increasing productivity. Working managers may participate as workers, but not as managers. Points of discussion are noted, changes to satisfy reviewers are generally a good thing but not required. Hence the peer relationship. This (to me) echoes the point I was trying to make above: reviews are to help the author, not to try and control what they have written. If you have rules, you'll have foolishness, they are inextricably linked. Any rule's benefit should be worth the foolishness it costs, and that relationship should be checked at regular intervals. Here I was initially unclear what the writer meant by "foolishness" - I think it's referring to the mental costs of staying aware of the rules and following the rules. The more rules, the higher that cost. If the rules are about trivialities, then we're spending expensive developer time on those trivial matters. I don't think we should be doing that. Review is better, as the Agile Manifesto notes, relationships are more important than process or tools. And that, more or less, is why I'm rattling on about this stuff. The relationships between developers, as peers with mutual respect, are amongst the most important things for our team to develop and maintain. Bad review practice risks damaging those relationships. |
| them. If they are, use them with aplomb. | ||
| * Talk in person if there are too many "I didn't understand" or "Alternative | ||
| solution:" comments. Post a follow-up comment summarizing offline discussion. | ||
|
|
There was a problem hiding this comment.
Can I suggest also: Make it clear whether your comment is blocking (you think it needs to be addressed before you grant a +1) or non-blocking (it's a suggestion or observation that the author can choose to implement or not). Without this distinction, it's not clear to the author what she needs to do to gain a +1.
|
Just found this article and I'm thinking maybe we can incorporate some of it into the "code review mindset" section: https://medium.com/medium-eng/the-code-review-mindset-3280a4af0a89 |
@elbaschid found this fantastic code review guide by the folks over at Thoughtbot.
When we read through it we felt like it matched Mobify's idea of doing code reviews very closely. We'd like to propose this guide (potentially with minor adaptations) to be included in the mobify-code-style repo.
Thoughts?
To do