I review a LOT of code, and honestly, I didn't find this article helpful. It's common sense on how to behave in a civil manner, but it doesn't really help with having effective code reviews.
Code reviews accomplish a few things:
- They are primarily a discussion about code changes. ("Why did you do this?"
- They help onboard new members of the team, both in terms of how to work most effectively with the code, and with style. (No style guide is 100% exhaustive, "We normally name variables for that fooBar, not barFoo.")
- They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!")
- They help enforce good engineering practice. ("I'm not merging that until you write tests.")
- They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.")
- They also quickly identify people who shouldn't be part of the team. Everyone sees that someone moves slowly in a pull request, or that someone resists cleaning up sloppy code.
The thing with people who resist cleaning up their sloppy code is that it shows to management. This happens when everyone else in the team gets their pull requests merged quickly, and one person's pull requests pile up with lots of minor comments.
> It’s common sense on how to behave in a civil manner
I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread.
[Conjecture/opinion] I believe that many people who enter software development and related fields are high on the autism scale- eg aspergers, etc., and that many such people are unaware of how badly that they’re treating others. It’s not “common sense” for many.
> I am frequently surprised by how lacking many technical professionals are in EQ/soft skills/civility -often I observe people who apparently believe that their skill is such that they don’t need to treat their peers with respect. This isn’t anomalous; it’s widespread.
I personally think that terseness is actually polite since it wastes a lot less time for both sides.
Code reviews accomplish a few things:
- They are primarily a discussion about code changes. ("Why did you do this?"
- They help onboard new members of the team, both in terms of how to work most effectively with the code, and with style. (No style guide is 100% exhaustive, "We normally name variables for that fooBar, not barFoo.")
- They help team members learn from each other. ("That's a cool new function in the new version of XYZ library!")
- They help enforce good engineering practice. ("I'm not merging that until you write tests.")
- They help make code more readable. ("Please add a comment here, and rename that method to DoSomething.")
- They also quickly identify people who shouldn't be part of the team. Everyone sees that someone moves slowly in a pull request, or that someone resists cleaning up sloppy code.
The thing with people who resist cleaning up their sloppy code is that it shows to management. This happens when everyone else in the team gets their pull requests merged quickly, and one person's pull requests pile up with lots of minor comments.