Comments in Code: Necessary or Not?

Attention: The following article was published over 3 years ago, and the information provided may be aged or outdated. Please keep that in mind as you read the post.

I was trying to understand the logic in someone else’s old code the other day, and it caused me to wish for better code commenting. That took me down a rabbit hole of thought. Are comments in code necessary or not?

Comments in Code in Theory

The argument in favor of commenting is that good comments make the purpose behind the code understandable to developers who have never seen it. Essentially, comments make code readable. It might seem to people who feel this way that those who argue against comments are arguing against readable code, “No comments needed, the code is clear enough on its own.”

On the other hand, there are those who consider comments an anti-pattern because often people will update the code without updating the comment. I see the logic behind this opinion since I often do that myself. Instead of comments, they might use method names and variable names that are helpful.

That is reasonable sometimes, but other times, that isn’t enough. I’ve seen some of the unusually long method names in the Spring framework, and somehow, the code still ended up… interesting.

I’ve also seen it suggested that instead of writing a line-level comment, one could extract a private method with the same name as the comment you were about to write. Ummmm…OK?

It had never occurred to me to do small refactorings like that instead of writing comments. I certainly couldn’t understand how trying to avoid writing comments could put you in a position of “writing a comment is a sign of a design smell.”

My own commenting habits are intermittent at best. If I’m writing something that, to me, is glaringly confusing or that’s intent is not perfectly clear (or if I’m making concessions because I am incorporating someone else’s code that I cannot change and whose requirements for use are unclear), then I’ll go ahead and make a comment to clarify.

Anyway, stories and theories are all well and good, but how about we look at some examples?

A Live Example of Well-Used Comments in Code

Consider a method that creates a new hotel booking for a Spring Boot application.

@PostMapping("/bookings")
public Booking createBooking(@RequestBody Booking booking, RestTemplate restTemplate) {
    // In a "real" environment this would at the very least be a property/environment variable, but ideally something like Service Discovery like Eureka
    Hotel hotel = restTemplate.getForObject("http://localhost:8080/hotels/" + booking.getHotelId(), Hotel.class);

    if (hotel == null) {
        throw new HotelNotFoundException(booking.getHotelId());
    }

    if (hotel.capacity() < booking.getNumberOfGuests()) {
        throw new NoAvailableCapacityException("Number of guests exceeds available hotel capacity");
    }

    if (!hotel.openingDays().contains(hotel.getDate().getDayOfWeek())) {
        throw new HotelClosedException("Hotel is not open on: " + booking.getDate());
    }

    List allByHotelIdAndDate = repository.findAllByHotelIdAndDate(booking.getHotelId(), booking.getDate());
    int totalGuestsOnThisDay = allByHotelIdAndDate.stream()
                                                       .mapToInt(Booking::getNumberOfGuests).sum();
    if (totalGuestsOnThisDay + booking.getNumberOfGuests() > hotel.capacity()) {
        throw new NoAvailableCapacityException("Hotel all booked up!");
    }

    return repository.save(booking);
}

Fortunately, we have attempted to make our variable names readable, and we throw very specific custom exceptions. This (a) helps clients that are using the code to deal with each case individually, and (b) also helps us understand at this point in the code what the validation logic is.

However, there is something about this code that bothers me. That series of ifs smells a bit. Validation code does often looks like this, but you do have to read the code carefully to understand what is being done.

Worse than not understanding what is being done is not knowing why it’s being done. So, more comments need to be added in this case.

public Booking createBooking(@RequestBody Booking booking, RestTemplate restTemplate) {
    // In a "real" environment this would at the very least be a property/environment variable, but ideally something like Service Discovery like Eureka
    Hotel hotel = restTemplate.getForObject("http://localhost:8080/hotels/" + booking.getHotelId(), Hotel.class);

    // check if the hotel actually exists 
    if (hotel == null) {
        throw new HotelNotFoundException(booking.getHotelId());
    }

    // check if the number of guests in the booking is more than the number of rooms in the hotel
    if (hotel.capacity() < booking.getNumberOfGuests()) {
        throw new NoAvailableCapacityException("Number of guests exceeds available hotel capacity");
    }

    // check the hotel is open on the day of the booking
    if (!hotel.openingDays().contains(booking.getDate().getDayOfWeek())) {
        throw new HotelClosedException("Hotel is not open on: " + booking.getDate());
    }

    // find all the bookings for that day and check that with all the booked guests the hotel still has space for the new booking guests
    List allByHotelIdAndDate = repository.findAllByHotelIdAndDate(booking.getHotelId(), booking.getDate());
    int totalGuestsOnThisDay = allByHotelIdAndDate.stream().mapToInt(Booking::getNumberOfGuests).sum();
    if (totalGuestsOnThisDay + booking.getNumberOfGuests() > hotel.capacity()) {
        throw new NoAvailableCapacityException("Hotel all booked up!");
    }

    // if we got this far, the booking is valid and we can save it
    return repository.save(booking);
}

The way in which the validation logic is written is still a bit confusing. Let’s break each distinct validation into its own method.

public Booking createBooking(@RequestBody Booking booking, RestTemplate restTemplate) {
    // In a "real" environment this would at the very least be a property/environment variable, but ideally something like Service Discovery like Eureka
    Hotel hotel = restTemplate.getForObject("http://localhost:8080/hotels/" + booking.getHotelId(), Hotel.class);

    // check if the hotel actually exists 
    checkIfHotelExists(booking, hotel);

    // ... rest of the method as before....

    // if we got this far, the booking is valid and we can save it
    return repository.save(booking);
}

private void checkIfHotelExists(Booking booking, Hotel hotel) {
    if (hotel == null) {
        throw new HotelNotFoundException(booking.getHotelId());
    }
}

Note that now the method is called more or less the same thing as the comment. It should be clear from the call site (the place in our code where we’re calling the method) what this method does.

I’m going to take a bit of a detour here because I don’t really like the shape of the method. Why does checkIfHotelExists need to take the booking as well? Well, it needs the hotel ID from the booking to create a helpful error message.

Since just the ID is needed and not the whole booking, I’m going to refactor to pass only what is needed to the method.

public Booking createBooking(@RequestBody Booking booking, RestTemplate restTemplate) {
    // In a "real" environment this would at the very least be a property/environment variable, but ideally something like Service Discovery like Eureka
    Hotel hotel= restTemplate.getForObject("http://localhost:8080/hotels/" + booking.getHotelId(), Hotel.class);

    // check if the hotel actually exists 
    checkIfHotelExists(hotel, booking.getHotelId());

    // ... rest of the method as before....
}

private void checkIfHotelExists(Hotel hotel, String hotelId) {
    if (hotel == null) {
        throw new HotelNotFoundException(hotelId);
    }
}

This looks better, but I’m actually still a bit concerned about the name of the method. checkIfHotelExists tells me what it’s doing, but it doesn’t tell me what the outcome is.

It sort of feels like maybe it should return something, like a boolean. Instead, however, what it does is throw a runtime exception if the hotel is null. Let’s document this method to show the contract.

/**
 * Checks if the hotel is null, and throws an Exception if it is * 
 * @param hotel the hotel to check 
 * @param hotelId the ID of the hotel, used for error reporting if the hotel is null. 
 * @throws HotelNotFoundException if the hotel is null */ 
private void checkIfHotelExists(@Nullable Hotel hotel, String hotelId) { 
     if (hotel== null) { throw new HotelNotFoundException(hotelId); } 
}

Ok, hold on. This accomplished what we were going for, but all of a sudden, it’s starting to seem like a bit much. There are really three things that tell us this. In this case, they are…

The comment is longer than the method,
The description of what the method does (throws an Exception if the hotelis null) is repeated twice in the documentation and, of course, is “readable” from the method contents itself, and
Javadoc on a private method is not encouraged.

Because of point 2, as soon as the functionality changes (even slightly), this whole comment needs to be completely re-written as it’s very closely coupled to the implementation. This is what people mean when they say documentation and comments “rot.” They can easily get age out of sync with the code.

I do like adding @Nullable to the hotelparameter, though. That feels like a helpful piece of “documentation.”

What if instead, we changed the name of the method to be even more descriptive but still general enough to encompass other types of validation if they come along later?

public Booking createBooking(@RequestBody Booking booking, RestTemplate restTemplate) {
    // In a "real" environment this would at the very least be a property/environment variable, but ideally something like Service Discovery like Eureka
    Hotel hotel = restTemplate.getForObject("http://localhost:8080/hotels/" + booking.getHotelId(), Hotel.class);

    // check if the hotel actually exists 
    throwExceptionIfHotelNotValid(hotel, booking.getHotelId());

    // ... rest of the method as before....
}

private void throwExceptionIfHotelNotValid(@Nullable Hotel hotel, String hotelId) {
    if (hotel == null) {
        throw new HotelNotFoundException(hotelId);
    }
}

This method name is even more helpful, and it removes the need for the comment altogether.

One possible side-effect of this refactoring is that I could test just this specific piece of logic, the throwExceptionIfHotelNotValid method, with a unit test. (Yes, it would either need to be made package-private or one would have to use a test framework that supported testing private methods.)

There’s probably no need for it at this stage, but as soon as the logic becomes more than this simple check, I might want a series of unit tests to document exactly what is expected of the method – what valid and invalid inputs are, what the expected behavior is, ect.

These unit tests are yet another good way to “document” the code, they should state the expectations of how it behaves and maybe even why. Actually, I wrote an article a while back about this, The Importance of Unit Testing. It’s dated, but you might still find some helpful info!

Let’s apply this sort of refactoring to all the sections of the method.

public Booking createBooking(@RequestBody Booking booking, RestTemplate restTemplate) {
    // In a "real" environment this would at the very least be a property/environment variable, but ideally something like Service Discovery like Eureka
    Hotel hotel = restTemplate.getForObject("http://localhost:8080/hotels/" + booking.getHotelId(), Hotel.class);

    throwExceptionIfHotelNotValid(hotel, booking.getHotelId());
    throwExceptionIfBookingHasMoreGuestsThanHotelHasRooms(booking, hotel);
    throwExceptionIfHotelIsClosedOnBookingDate(booking, hotel);
    throwExceptionIfHotelDoesNotHaveEnoughSpaceOnThatDate(booking, hotel);

    // if we got this far, the booking is valid and we can save it
    return repository.save(booking);
}

private void throwExceptionIfHotelNotValid(@Nullable Hotel hotel, String hotelId) {
    if (hotel== null) {
        throw new HotelNotFoundException(hotelId);
    }
}

private void throwExceptionIfBookingHasMoreGuestsThanHotelHasRooms(Booking booking, Hotel hotel) {
    if (hotel.capacity() < booking.getNumberOfGuests()) {
        throw new NoAvailableCapacityException("Number of guests exceeds available hotel capacity");
    }
}

private void throwExceptionIfHotelIsClosedOnBookingDate(Booking booking, Hotel hotel) {
    if (!hotel.openingDays().contains(booking.getDate().getDayOfWeek())) {
        throw new HotelClosedException("Hotel is not open on: " + booking.getDate());
    }
}

private void throwExceptionIfHotelDoesNotHaveEnoughSpaceOnThatDate(Booking booking, Hotel hotel) {
    // find all the bookings for that day and check that with all the booked guests the hotel still has space for the new booking guests
    List allByHotelIdAndDate = repository.findAllByHotelIdAndDate(booking.getHotelId(), booking.getDate());
    int totalGuestsOnThisDay = allByHotelIdAndDate.stream().mapToInt(Booking::getNumberOfGuests).sum();
    if (totalGuestsOnThisDay + booking.getNumberOfGuests() > hotel.capacity()) {
        throw new NoAvailableCapacityException("Hotel all booked up!");
    }
}

It still feels pretty procedural to me, but there are advantages like…

The createBooking method now “documents” the validation steps taken before the booking is accepted, via the calls to the methods.
The createBooking method is shorter.
Each check is separated clearly.
Each throwException... method can be tested individually if required.

Even though the original topic seems to have gotten sidetracked a bit, what I hope I’ve shown is that it’s possible to reduce the number of comments in your code by extracting functionality that needs to be documented into its own method with a descriptive method name.

In Summary

No matter what your opinion is on comments, one thing is universally true: developers should create code that the next developer can understand.

Sometimes, comments are a safe, simple way to leave the code better than how you found it. Sometimes, extracting sections of code into a well-named method can be an alternative way of doing this, with the added benefit that (a) you can test this method individually and (b) it may help you to spot smells in your code or see places for further refactoring or simplification.

Each situation you encounter will require different treatment, but I hope this post has given you a starting place for creating clean, readable code in your own life! I’d love to hear your questions or opinions in the comments below.