Packetizer Logo
 

Writing Code for the Next Person

By: Paul E. Jones

Inline Comments

Inline documentation is also something I personally feel is very important. In the commenting code section, I provided an example that illustrates why it can be useful. However, there is always debate about how much inline commenting to use.

I admit that I do more inline commenting than most. That said, it takes minimal effort to insert a comment. In fact, I often write code by sketching an outline of what I want to do, then fill in the outline with actual code. That might explain why my code has more inline comments than most.

On the other extreme are those who feel that the "code should speak for itself" (having no comments) and those who just throw in random comments for seemingly no reason. Whatever your view is on inline comments, I can assure you that some programmers truly appreciate them. Getting back to the point of this document: we are all writing code for the next person. We need to consider choices that will facilitate making it easier for fellow team members today or those sustaining the code tomorrow.

Here's a good example of some code that is extremely useful, but makes little sense without commentary.

Source Code

struct RTPComparator
{
    static constexpr std::uint16_t midpoint_value =
        (std::numeric_limits<std::uint16_t>::max() >> 1) + 1;

    bool operator()(std::uint16_t lhs, std::uint16_t rhs)
    {
        return static_cast<std::uint16_t>(lhs - rhs) >= midpoint_value;
    }
};

What might help in understanding is the context within which this code would exist. In RTP, there are 16-bit sequence numbers assigned to packets. As they are received, those packets need to be ordered by those sequence numbers. The value of the sequence numbers roll over at 0, thus there is an ordered sequence: 65534 < 65535 < 0 < 1. This struct above (referred to as a "functor" in C++) can be used by various objects in C++ to provide ordering, akin to std::less that orders things from smallest to largest.

In any project, concepts like RTP sequence numbers would be understood if a substantial part of the project deals with RTP. Thus, some things do not necessarily need documenting in the code. However, looking at that example, it is not obvious even for people who work with RTP daily what it is doing. A little inline commentary helps. Consider the following.

Source Code

// Functor used to sort RTP packets by sequence numbers
struct RTPComparator
{
    // Divide the max packet sequence number by 2 and add 1 to get the midpoint
    static constexpr std::uint16_t midpoint_value =
        (std::numeric_limits<std::uint16_t>::max() >> 1) + 1;

    // Function to facilitate comparing RTP sequence numbers
    bool operator()(std::uint16_t lhs, std::uint16_t rhs)
    {
        // The following indicates lhs < rhs
        return static_cast<std::uint16_t>(lhs - rhs) >= midpoint_value;
    }
};

One could probably add even more comments to explain why the math works as it does, but at least there is an explanation of what it means now. Having this basic explanation at least gives a developer confidence that the functor will serve his or her needs.

In this example, you'll note that I did not use the larger function description block I described in the function description section. Given the limited scope of this struct, doing that would probably be overkill. However, these are the kinds of decisions a team should make.

The >> 1 syntax is something I often use (since I know for certain that it will be interpreted efficiently by the compiler or interpreter), but many people are not familiar with it. Adding that comment that it divides the number by two either educates those who did not know that >> 1 would divide by 2, or it will be a reminder. As you are writing code, think for a moment whether somebody on the team or a new college graduate might be confused. If the answer is yes, sprinkle in some comments. It really takes very little effort, and it also provides a clue as to what was intended in the event the code is wrong.

There is also another reason to have inline comments: they can help catch mistakes. The reality is that comments can be wrong, just as the code can be wrong. However, if a comment and the code do not agree, it can often point to a problem.

In this code example above, one might ask why not just hard-code the constant midpoint. That is a valid question. For C++, the constexpr is effectively a constant, but writing it this way allows one to create more generic code than can work with any size integer. Consider the following:

Source Code

// Functor used to sort sequence numbers that rollover
template<typename T
         typename =
             std::enable_if_t<std::is_unsigned_v<T> && std::is_integral_v<T>>>
struct SequenceNumberComparator
{
    // Divide the max sequence number by 2 and add 1 to get the midpoint
    static constexpr T midpoint_value =
        (std::numeric_limits<T>::max() >> 1) + 1;

    // Function to facilitate comparing sequence numbers
    bool operator()(T lhs, T rhs)
    {
        // The following indicates lhs < rhs
        return static_cast<T>(lhs - rhs) >= midpoint_value;
    }
};

// Define a comparator suitable for RTP packets
using RTPComparator = SequenceNumberComparator<std::uint16_t>;

Those familiar with the C++20 concept feature might note that the code above can be improved further using that feature. Here is an example of the previous example using concept:

Source Code

// Define a concept for unsigned integral types
template<typename T>
concept UnsignedIntegral = std::is_unsigned_v<T> && std::is_integral_v<T>;

// Functor used to sort sequence numbers that rollover
template<UnsignedIntegral T>
struct SequenceNumberComparator
{
    // Divide the max sequence number by 2 and add 1 to get the midpoint
    static constexpr T midpoint_value =
        (std::numeric_limits<T>::max() >> 1) + 1;

    // Function to facilitate comparing sequence numbers
    bool operator()(T lhs, T rhs)
    {
        // The following indicates lhs < rhs
        return static_cast<T>(lhs - rhs) >= midpoint_value;
    }
};

// Define a comparator suitable for RTP packets
using RTPComparator = SequenceNumberComparator<std::uint16_t>;

Whether one uses the older syntax or the newer syntax might depend on the target platform and capabilities. Generally, one should strive to use the latest syntax, especially if it makes the code clearer. Whether it does in this case is debatable, but I have seen code where using concept really made the code a lot more readable.

You will note that as I add more syntax, I introduce more comments. I understand that not everyone will be familiar with all syntax elements of a language, so a simple comment explaining what a line does can be very helpful to others.

As a closing comment to this section: be sure to keep comments updated as code changes. This is often overlooked, but it should just be a part of your normal routine.