This content originally appeared on HackerNoon and was authored by Maksim Zhelezniakov
How many times do you cross your fingers when opening a pull request in the hope that it’s not assigned to that one developer in your team who always leaves a handful of comments about everything and anything? I’m not talking about those very reasonable ones that reveal made mistakes or, please no, code smell.
\ These are the comments about your code style or some little subjective things that are not targeted at improving the code base in the end, but rather feel like “I would’ve done it another way because I just like it more.” As a responsible teammate, you are likely to address them.
\ No matter which option you decide to go with — just apply all the changes or push back with long explanations on why you don’t agree about this and that — you will probably end up feeling agitated, exhausted, and frustrated about the time spent on non-essentials. And was it really worth it? This question will pop up in your head every time you deal with such situations.
\ Or sometimes, you have another end of the stick — insta approves with no comments and no signs that a reviewer actually carefully checked your changes. Whether they did review it, and there are no objective inputs, or not, you are left questioning both yourself and this teammate.
\ Granted, even if everything is good, wouldn’t it be nice to include a quick shout-out to the author and ensure you are on the same page with no doubts left?
\ If both stories above sound too familiar, this article is for you. What your team misses is a pull request culture or, in other words, a set of guidelines on how to communicate in pull requests to support a friendly and highly efficient collaboration process. I’ll cover the essential pieces that reviewers would want to look for and also examples of questionable comments — simpler, nitpicks — that create friction or sometimes even problems beyond your personal feelings.
\ As an iOS developer myself, I will use Swift in my code samples, but generally, this topic is important for any developer, no matter the platform or language you use on a daily basis.
\ However, I believe that code review culture is even more relevant for Apple platforms because in my experience we tend to be more picky in a lot of ways. Probably it comes from a perfectionist mindset inherited from Steve’s vision back in the old days.
Lesson Learned
Pull request or merge request is a way to verify code changes with your colleagues and check it for errors, assess its readiness before going to the next stages, and, finally, to end users. It is also one of the main communication channels between developers. Sometimes, we might not even know a person outside of threads in PRs.
\ Quite often, especially in their earlier career stages, developers do not pay attention to their PR communication. Specifically, how their comments can be perceived by other people, whether the points made are clear and correctly written in English, and so on.
\ Believe me, I’ve been there and have also made mistakes in the past. One particular experience has stuck with me to this day. I bring it up almost every time during discussions about PR culture because it perfectly highlights the importance of this topic.
\ Years back, when I was looking for a new job, I was challenged with a mocked pull request that I needed to review. I didn’t think about everything thoroughly that time because I was excited about having received an easy task instead of another Leetcode torture. Of course, it wasn’t that easy. It was a test on pull request culture that I failed drastically.
\ I skimmed through the PR quickly and left a bunch of mainly pointless comments, for example:
\
import UIKit
import AVFoundation
// It would be nice to list your imports alphabetically, so that it’s easier to read.
\ Or another one:
\
func someMethod() {
// about 30 lines of code
}
// What do you think about splitting this method in half?
\ In other rare comments where I spotted a problem, I wasn’t clear enough about it and didn’t provide any reasonable solution. But the main failure of this exercise was that I missed a couple of performance problems because my focus wasn’t in the right place. In the end, I received the following feedback:
\
I felt you missed a lot of important details, like code correctness, performance and general improvements (with a few minor exceptions).
\ You focused a lot of the review on formatting which should be a team-discussion and/or make use of formatting tools. This also allows developers to focus on the right things in a review.
\ Since then, I realized my approach to code reviews was far from perfect and lacked many important things.
\ This example is quite primitive in a way, but it shows clearly how the lack of a PR culture can easily become a dangerous path leading right to bugs and problems in production.
\ Let’s dive into the details and start with something you would want to avoid as a code reviewer — writing nitpick comments.
What Is a Nitpick Comment?
There are a lot of ways you can express your logic and thoughts in code. Modern programming languages give you enough versatility to do so, and often, there is no right or wrong between multiple approaches. However, we don’t want our projects to look like Frankenstein monsters — every team should have a decent code style with guidelines.
\ At the same time, we cannot and we should not control every line of code of our teammates. I believe that finding a good balance between the two is a virtue that we ideally want to strive for.
\ A nitpick comment is a request to make a change, usually a small one, that does not have objective reasons to do so. Often, it is a projection of a personal preference of a code reviewer into the reviewed code. Let me show some examples of it:
\
// Original
guard let newValue, newValue != oldValue else { return }
// Change request
guard let newValue, newValue != oldValue else {
return
}
\ If you ask me, I’d always choose the second option because it’s easier in my opinion to debug such code and put a breakpoint right on the return line. But you can argue that the first variant saves 2 lines, plus you can see this styling from Apple engineers, as well.
\
// Original
func handleErrorIfNeeded(_ error: SomeError) {
if error == .failedRequest {
return
}
if error == .validationFailed {
state.errorMessage = “Data input is incorrect”
return
}
// … other cases
}
// Change request
func handleErrorIfNeeded(_ error: SomeError) {
guard error != .failedRequest else {
return
}
guard error != .validationFailed {
state.errorMessage = “Data input is incorrect”
return
}
// … other cases
}
\
In a lot of logic situations, guard and if in Swift can be used interchangeably. However, I’d argue that they don’t have the same language semantics. guard, as the name implies, wants to “protect” your code flow from an unwanted result, whereas if is neutral in its nature and allows a 50-50% chance of different code paths to happen. Both variants technically aren’t wrong and perfectly readable, though.
\ And the last one:
\
// Original, the constant value is not being repeated anywhere else
func calculateDuration(endDuration: Duration) -> Duration {
let startDuration = Duration.milliseconds(130)
// … calculation
return finalDuration
}
// Change request
func calculateDuration(endDuration: Duration) -> Duration {
let startDuration = Duration.milliseconds(Constant.startDuration)
// … calculation
return finalDuration
}
enum Constant {
static let startDuration = Double(130)
}
\ You can argue about the need to extract any constant into a separate namespace if it’s used only once, and when it’s not a magic constant. In the first variant, you clearly know what is this number of 130. However, someone will tell you to extract any constant like this, no matter what.
\ All the examples above are nitpick changes. Both variants are perfectly fine, readable by anyone, and do not break your code. They are merely alternatives with the same weight in usage.
\ So, what do you want to do with comments like these? From a reviewer perspective, some will say something like this:
\
Well, it’s just a suggestion. It’s not mandatory to apply it.
\ True, but at the same time it’s important to let the author know about it, for example:
\
It’s just a suggestion, feel free to discard it, but what do you think about this approach: … ?
\ However, my take on nitpicks is to not post them at all. Why?
\ Let’s say, you submitted a nitpick comment, even highlighted that it’s a low-priority suggestion. What happens next?
\
- 1st option: the author applies the change. What does it really mean behind the scenes?
- They need to save the current changes they are doing.
- They need to switch to the PR branch next.
- They need to apply the change.
- They might resolve conflicts.
- They need to push the change.
- Then it will trigger one more round of CI tests because of the new code pushed.
- They need to reply to the comment with text or emoji.
\ From one side, it’s a very simple set of tasks we do every day, as developers. But just imagine, it’s not one nitpick comment, but 10 instead. Or you have 5 pull requests open at the same time with nitpicks of different quantities. Was it really worth your time and the hassle? When we know what a nitpick is, the answer is no. I would rather deliver my changes faster and save time.
\
2nd option: the author does not apply the change.
In many situations, it’s just rude to disregard your teammate’s comment without responding at all. And it might look rude as well just to respond with:
\
Thank you for the suggestion, but I don’t like it. I won’t apply it, as I don’t think it’s necessary.
\
Instead, as the author, you will probably go deeper into explaining the details of why you won’t apply the changes. For example, your vision on
guardvsletas above.\
Then, it’d be great if the reviewer just accepts your take and moves on. But they also might feel the pushback and respond because they genuinely feel that their way is better.
\
Such threads can easily blow out of proportion with long comments back and forth which is not productive at all. And it’s not going anywhere in the end. You can both agree to disagree, there is no “winner”, but the final question is the same. Was it really worth your time and the hassle?
\
My practice is to filter out those nitpicks in code reviews as a reviewer even before posting anything by asking myself these questions:
\
Is it really important thing to change?
Are there any objective reasons for it? If so, name them.
Do I want to post this remark just because I would’ve done it differently? Or there is a real concern?
\ This way, you can remove the “noise” of nitpick comments from your code review and leave only important parts, the parts you all need to focus on scalability, robustness, thread safety, etc.
\ But why such comments do appear? Usually, it’s a sign that reviewers don’t have a clear understanding of what to focus on or look for in pull requests. In the next section, we’ll discuss exactly that.
How to Create a Code Review Culture
Let’s see how we can create a blueprint of a PR “gold standard.” I will list a set of values and guidelines that are essential in my opinion on the way of improving your code reviews:
\
- For all: Define a set of buckets or categories for your code review comments, and assign them different levels of severity. Always show examples to your colleagues, so that you are all on the same page. Example:
| Type | Severity | Expected from author | Expected from reviewer | Example | |----|----|----|----|----| | Personal style aka nitpick | 0 | Preferred is to do nothing | Try to avoid | return Object() vs return .init() | | Small improvement | 1 | Apply or dismiss with a comment | A brief explanation of why they think it’s better | Early return or else block |
\ …And so on. It can be different in your team, but the idea here is to have a structure and classification. Bringing on severity values allows us to prioritize it better and focus on the essentials.
\
Reviewer: Try to allocate a specific time slot for your code reviews. Do not rush things and allow yourself to dive deeper into the meaning of code changes, not the syntax. This ought to improve the quality and clarity of your comments.
\
Author: Help your peers. If you know there are some parts that can be unclear because of the lack of context or because your PR is only one part in a series with things to come, just leave a comment with an explanation about your own pull request in advance. It will save time for both sides!
\
Reviewer: re-read your comments before posting. Put yourself into the shoes of the other side. Is everything clear? Try to stick to the following structure:
State the concern you have.
Why it’s important.
What do you suggest to do instead?
\
For all: Check your grammar and spelling in English. Yes, for many of us, English is not our native language, and it’s sometimes a hard thing to do. But if not written well, it can lead to all sorts of confusion and misalignments you don’t want to have.
\
For all: How is your comment perceived? Does it sound friendly? This point is quite similar to the previous one, but the idea is to keep a healthy, friendly approach in the written form. For example:
\
You must extract this logic into a separate method.
\ Even though there is nothing wrong grammatically with this sentence, the word “must” is rarely used in this context and can be treated as a command to a reader. Government officials will tell you what you must do because there are laws. But it’s not the type of wording you want to use with your colleagues when you’re on the same team. Try this instead:
\
What do you think about extracting this logic into a separate method?
\
Reviewer: Balance negative with positive. If you left a handful of critical comments, find some good ones and highlight them, too. It’s a small thing, but improves overall perception in the eyes of the author.
\
Reviewer: If everything is good in code changes, don’t shy away from posting praise. Insta approves with no signs is not that good behavior and leaves a lot to question. Post a shout-out with the exact things that you found to be great in your peer’s work. Simple “LGTM” is too generic and can be perceived as dismissive.
\ I think these are good points to start with in your team to work towards better and healthier communication in your pull requests. The harder part after that is to keep following these guidelines. Sometimes, it can be two steps forward and one step behind.
Conclusion
In the end, coding is not always the hardest part. Communication and collaboration can be more challenging, especially when you all come from various countries and backgrounds with your own experiences and thoughts.
\ I hope this insight into code review culture has been helpful, and maybe you will take something useful out of it.
\ Good luck, and friendly code reviews to you all!
This content originally appeared on HackerNoon and was authored by Maksim Zhelezniakov
Maksim Zhelezniakov | Sciencx (2024-07-24T00:23:55+00:00) Code Review Culture: Why You Need to Have One. Retrieved from https://www.scien.cx/2024/07/24/code-review-culture-why-you-need-to-have-one/
Please log in to upload a file.
There are no updates yet.
Click the Upload button above to add an update.