When it comes to reviewing someone else's code, how should you approach comments?
Comments are typically seen as a good thing. They can improve readability, offer context as to what the code is trying to do, and help you remember parts of the code that should later be changed or refactored.
But is that all true? Are comments really that good, or can they be the telling sign of a bigger problem?
The hard truth is that, in many cases, comments can point you to inherent problems with the code that you're reviewing. There are also cases when comments are helpful, and you should know how to identify when and where this is the case.
But first, here are some examples of bad comments.
//FIXME
Just look at this piece of code:
$done = false;
$attempt = false;
while (!$done) {
$attempt++;
$done = true;
performSomeAction();
if (somethingWentWrong()) {
$done = false;
}
// FIXME Should we delay a second or two before retrying?
}
Who wrote that FIXME comment there? Was it the developer who initially built this feature or someone who worked on this piece of code later on?
If you're reviewing this code, you probably have the answer and are probably at the best point in time to stop this from going to the master branch.
There is no reason for that comment to be there. If the question, "Should we delay a second or two before retrying?" is an open one, then the developer should ask the project manager so that an actual answer can be found. If there is no project manager to be found, then, of course, we have a bigger organizational problem. Regardless, the comment here is indicative of something that might be wrong with the project, and that should not be tolerated. This is a classic example of the comment being a symptom of a bigger, in this case organizational, problem.
//TODO
TODO comments are often introduced by the same developer who built the new feature or piece of code that's being reviewed. That's because as she was working on the code, she realized that some improvements could be made, only to decide not to apply those improvements this time around. This could be due to any number of reasons, from time limitations all the way to laziness.
A TODO comment could indicate that some small refactoring may be helpful:
// TODO: do decompose conditional (https://refactoring.com/catalog/decomposeConditional.html) here
// by replacing this condition with a function
if user.isActive == true && user.Country == "Ireland" {
doWhatever()
}
Of course, it only takes a few minutes to replace that line of code with something like:
if isActiveUserFromIreland(user) { ... }
There is no reason to clutter your code with a TODO comment when all it does is reminding us of a small, quick refactoring that could help us improve the code. If it's a small enough change, it should just be done. There is no need to defer that change.
A TODO comment could also point to the need to add something very important, as in this case:
func addUser(name string) {
// TODO: save user to database
}
Here, this functionality simply doesn't work. That method is meant to save something to the database, and it's obviously not doing it.
While in the first example the change needed was a small one and it should have simply been done, now we're faced with a potentially more complex functionality. Who knows why that's not been done yet? Maybe we haven't chosen a database library yet, or we lack some vital information to proceed. Regardless, that TODO comment is there to clutter our code. It's not the best way to achieve what we want, namely, to remind us that the functionality that saves the user to the database needs to be implemented.
Whether the change indicated by the TODO comment is a small or a big one, it inevitably points us to the lack of a properly used ticketing system such as Jira, GitHub Issues, or YouTrack.
Unnecessary comments
Have you ever heard someone saying that you can never have too many comments in your code? Unfortunately, that's not true. Comments that are not strictly necessary can be instrumental in hiding badly designed code.
Look at this example:
type User struct { ... }
func (u *User) changeName(name string) {
// verify whether the user has a name already
hasName := false
if (u.Name != nil) {
hasName = true
}
// if user has a name already
if (hasName) {
u.PreviousNames = append(u.PreviousNames, u.Name)
u.NameChangesCount = u.NameChangesCount + 1
}
// save the new name
u.Name = name
u.FullName = u.Name + " " + u.LastName
// save initials
u.Initials = u.Name[0] + u.LastName[0]
}
On top of some obvious problems there, what those comments tell us is that we're dealing with bad design. There are a number of concerns with this function, such as:
- Breach of single responsibility principle: The function is doing too much.
- Too much procedural code: Those pieces of code could easily be moved into small, expressive functions.
There is more that we could say about this piece of code, but let's focus on the comments and why they're indicative of the above-mentioned issues.
First, if the function had complied with the single responsibility principle, we wouldn't have needed those comments at all. The function is called updateName, and as long as what's happening inside the function is the update of a name, then there is no need to add any comment. It's self-explanatory!
Second, it's clear in the example that the comments are enabling us to keep adding lines of code doing all sorts of things. A thoughtful programmer would feel unjustified in adding more and more lines of code to a single function. But, because there are nice little comments telling us what's happening line by line, then it feels like it's not a big a deal after all.
In the above example, virtually any piece of code that's preceded by a comment should be in its own function. There is no way around that. Comments are only a shortcut in this case, and they should be treated as such.
Commented pieces of code
This is an easy one to identify. A piece of code that used to be running and that gets commented out should have no place in your code. As a code reviewer, it's your responsibility to point this out.
The reasons for leaving a piece of code commented instead of removing it completely are typically related to either not being sure if that code will ever be needed again, or wanting to leave it there as a point of reference for everyone else.
These are not good enough reasons to clutter your code with unused code.
But then, how do you preserve a piece of code that could be needed for the future? The short answer to that is: You don't. The longer answer comes in two points:
- You should be working with a modern version-control system such as git. That will always allow you to go back to any prior version of a file and look at the code as it was.
- There is no reason for you to believe that the exact piece of code that you're commenting will still be working once it's needed again. You should rather have a place where you can document the way that piece of code used to work on a high-level basis. It should then be implemented afresh if you ever need to in light of the way the rest of the code works now.
What's so bad about comments?
We've looked at a number of specific examples that should help you identify bad comments when you see them as you perform a code review.
But comments should almost always make you stop and carefully consider whether they're needed or not. Here's why:
- Clutter. Bad comments add clutter and make the code less readable. It's important to only keep the comments that are strictly necessary.
- Bad design. As we've seen in the examples above, comments often tell us that the design could have been way better.
- Laziness. Comments can be used as a cheap shortcut to avoid writing proper code according to best practices, as in the "unnecessary comments" example.
- No compiler checking. Comments go unchecked by the compiler, which means that there is no way to ever tell us if they're correct or not. Code itself is the most reliable and self-documenting resource.
Good comments
With few exceptions, the only comments that can be considered good are the ones that give us a high-level description of a construct (such as a class, a type, an interface, or a function).
As in this example:
// Car is an interface that can be used to implement Car objects
// It's a generic car and it's not specified whether the gearbox
// is automatic or manual
type Car interface {
SwitchEngine()
Gear() Gear
Color() string
}
This is a perfectly acceptable comment that gives us context as to what the Car interface is about. We probably would have understood it without the comment as well, which is the whole point. But the comment is helpful and doesn't take anything away from the correctness of the code.
Code review de-coded
As we've seen, there are some cases (however limited) where comments can be useful for the person reading them. The key is to only add information that the reader could have deduced anyway. Comments should help provide context to a construct, but they should not be used to describe what the code is doing.
It's important to also make sure that comments aren't masking bigger issues, such as fundamental organizational inefficiencies or laziness on the part of the developer who wrote the code. Even though comments are often viewed as positive, it's always a good idea to carefully review code with too many comments mixed in.
Keep learning
Take a deep dive into the state of quality with TechBeacon's Guide. Plus: Download the free World Quality Report 2022-23.
Put performance engineering into practice with these top 10 performance engineering techniques that work.
Find to tools you need with TechBeacon's Buyer's Guide for Selecting Software Test Automation Tools.
Discover best practices for reducing software defects with TechBeacon's Guide.
- Take your testing career to the next level. TechBeacon's Careers Topic Center provides expert advice to prepare you for your next move.
