Don't Comment Your Code - Write Better Code

Update: Somehow, I managed to remove the middle section of this post and most of the code example when I was updating labels, or adding a link, or something. It read like nonsense because of that, but I've rewritten the middle section to the best of my recollection. If you were confused before, it should make more sense now.

As I've gained programming experience, I've noticed a significant change in how I write code. I tend to write less and less comments, and the nature of them has changed. Where I used to explain what the code did and how it worked, I now leave those explanations up to the code itself. I try to more directly express what the code is doing, and in the rare cases where that is not sufficient, I may put a few words in a comment to explain why the code is doing what it's doing. If I find comments answering 'what' or 'how', I take that as an indicator that the code is not written well enough. Then I refactor the code to make the comment redundant and eliminate it.

I do this refactoring primarily because I hardly ever read comments. When I'm trying to understand a block of code, I focus on the code because that is what gets executed. The comments are a distraction, and I don't trust them. They very easily get out of sync with the code, and then they lead you astray instead of aiding your understanding.

I've come to think of comments like a writing device that I find more than mildly annoying - footnotes and end notes. I understand their use for citing references, but when an author feels the need to add explanations and anecdotes in footnotes that should have been in the main text, all it does is break up the flow of the text. If the footnotes were so important that they had to be included, they should have been integrated better with the main text. If they don't fit in the main text, then they should have been cut out completely.

The same reasoning applies to comments. Footnotes should not be a substitute for better writing, and comments should not be a substitute for better code. And like writing, better coding involves making the code more directly express what the programmer intends. This is not an easy thing to do, and it can take many drafts to reach a version of the code that does an adequate job of expressing its purpose. This process is a form of optimization that improves readability instead of performance, yet it is just as important as performance optimization because confusing code is a minefield of potential bugs and performance losses.

An Example of Bad Code Badly Commented


To make my reasoning a bit more concrete, here is an example of one particularly messy method that I refactored recently that illustrates how I go about making code more clear. Keep in mind that some months prior I had already done a fair amount of work assigning better variable and method names, but the code still went through significant changes before reaching a clear and concise result. As the method name implies, it runs a filter over a set of samples:

void CFilter::Run(void) {
   // Update stage 0 after EDMA writes into the Stage 0 buffer
   _rgStageInfo[0].SetWriteIndex(_pWrFirstStage);

   // Run filter stages
   int i = 0;
   int rgixFirOutput[MAX_ADCS] = {0};
   for( i = 0; i < _cStages; i++ ) {
       // Determine if there are enough Samples to produce at least 2 results
       int cResultPairs = _rgStageInfo[i].NumResultPairs();

       if ( 0 == cResultPairs ) break;

       bool fIsInternalStage = i+1 < _cStages;
       int dixFirOutput = 1;
       if (!fIsInternalStage) {
           if (_fDownConverting) dixFirOutput = 2;
       }

       // process all available samples in this stage
       for( ; cResultPairs > 0; cResultPairs-- ) {
           if ((i == 0) && _fDownConverting) {
               _rgStageInfo[i].DownConvert(_centerFrequency, _cChans);
           )

           int *pixFirOutput = _rgixFirOutput;
           Sample *pFirOutput = _rgFirOutput;
           Sample *pFirOutputAlt = _rgFirOutput + 1;
           if (fIsInternalStage) {
               GetFirOutputIndexes(rgixFirOutput, i+1);
               pixFirOutput = rgixFirOutput;
               pFirOutput = _rgStageInfo[i+1].GetBuffer();
               pFirOutputAlt = _rgStageInfo[i+1].GetBufferAlt();
           }

           _rgStageInfo[i].CalculateResultPair(fIsInternalStage, _cChans, 
                                               pixFirOutput, dixFirOutput, 
                                               pFirOutput, pFirOutputAlt);
       } // for (cResultPairs)
    } // for (all Stages)

    // Is this the final stage?
    if ( i == _cStages ) { 
       // Reset EDMA src addr to simulate a 4-word linear buffer src
       // then trigger EDMA to move filtered results to vInput buffers
       if ( _csHoldoff ) {
           _csHoldoff--;
       } else if (_fDownConverting) {
           CFft::EdmaTriggerAll(_mid, _rgStageInfo[_cStages].GetBuffer(),
                                _rgixFirOutput, _fDownConverting);
       } else {
           // This should be unecessary since the parameter set will reload itself?
           EdmaSetSrc(_hEdmaFirOut, _rgStageInfo[_cStages].GetBuffer());
           EdmaSetChannel(_hEdmaFirOut);
       }
   }
}

Did your eyes glaze over? It's fairly confusing, and frankly, ugly code. There is so much going on here that doesn't have much to do with running the filter stages, and the comments are not at all helpful. But before getting into that, I should briefly explain the Hungarian notation being used. I use a variation of Apps Hungarian Notation to prefix variable names with information about the variables in shorthand. I was skeptical of Hungarian notation at first, but I've found that using prefixes that are actually meaningful in the context of the application are quite helpful for naming and understanding variables. Once you get used to the prefixes used in a given app, you no longer have to waste much time thinking of good variable names because, most of the time, they quickly come to mind. A lot of these conventions are the same across applications, but some are specific. Here are the ones that are relevant to this code:

'_' = member of a class
'c' = a count of something
'd' = a difference between two things (i.e. an offset)
's' = a data sample, specific to this DSP app
'f' = a boolean flag
'p' = a pointer
'h' = a handle to a system resource
'rg' = a range (i.e. an array)
'ix' = an index

These prefixes can be stacked, so 'cs' is a count of samples and 'rgix' is a range of indexes. Not all variables will lend themselves to this notation, but those variables normally have a specific purpose that's easily named. For example, _centerFrequency doesn't have a prefix except for the member designator, but it doesn't need one because it's clearly the center frequency that the sample stream is being down converted to.

An Intermediate Step on the Way to Something Better


Getting back to the problems with the code, the first line of the method shows the main problem with this whole piece of code. It is too detailed for the level of meaning that this code should convey. The method is running a filter, so it should clearly show how it runs the filter, not muck around with setting the write index of the first stage. The write index was optimized out, which I'll get to later, so this line and its accompanying comment were removed.

The next improvement comes from knowing that this code is only called when a pair of samples are available for processing, and only one pair of samples is ever available when the method is called. These constraints are not apparent in the comments. In fact, the comments lead the programmer to believe that any number of samples could be ready, even zero, but that is not the case. Moving the test for available samples to the end of the for loop and checking if enough samples have been accumulated to run the next stage makes more sense. Oh, and that redundant comment? Gone.

Let's move on to the inner for loop. As I said, this method is only called when two results will be generated for the first stage. Additionally, each subsequent stage can only generate a maximum of two results as well. That means the inner for loop will only run once. It's useless! And so is the redundant comment preceding it. They'll get axed.

Finally, there is a lot going on with a couple of FIR output buffers that is mucking up the inner for loop. All of those xxFirOutput variables are rather confusing. They are used to keep track of the stage buffers and the channel indexes within the stage buffers so that filter results get moved to the right place after each stage is processed. Instead of having this method keep track of all of this stuff, the stages themselves should keep track of it, and they should each have a pointer to the next stage so that they can coordinate the movement of their filter results. Moving the buffer handling code into the stage class simplifies the moved code, and reduces this Run() method to its fundamental operations:

void CFilter::Run(void) {
   // Run filter stages
   int i = 0;
   for( i = 0; i < _cStages; i++ ) {
       if ((i == 0) && _fDownConverting) {
           _rgStageInfo[i].DownConvert(_centerFrequency, _cChans);
       }


       _rgStageInfo[i].CalculateResultPair(_cChans);


       // TODO integrate this better and possibly use a while loop
       if ((i+1 < _cStages) && _rgStageInfo[i+1].DecSamplesUntilResult()) break;
   } // for (all Stages)


   // Is this the final stage?
   if ( i == _cStages ) {
       // Reset EDMA src addr to simulate a 4-word linear buffer src
       // then trigger EDMA to move filtered results to vInput buffers
       if ( _csHoldoff ) {
           _csHoldoff--;
       } else if (_fDownConverting) {
           CFft::EdmaTriggerAll(_mid, _rgStageInfo[_cStages].GetBuffer(),
                                _rgixFirOutput, _fDownConverting);
       } else {
           // This should be unecessary since the parameter set will reload itself?
           EdmaSetSrc(_hEdmaFirOut, _rgStageInfo[_cStages].GetBuffer());
           EdmaSetChannel(_hEdmaFirOut);
       }
   }
}


That already looks much better. Now it's much more clear what the method is doing. For every filter stage, it does an optional down conversion if it's the first stage, it calculates a result pair, and it checks if enough samples have accumulated in the next stage to run it as well. If not, it breaks out of the for loop. If all stages have run, then the results are passed on to the next step of the process. But wait, look back at that down conversion code. It only runs in the first stage and the first stage is guaranteed to run, so it can be moved above the for loop. Also, that first comment isn't saying anything useful so we can get rid of it.

Next, notice that TODO comment? That is one kind of comment I don't hesitate to put in my code. It's there to remind me to go back to something that I might forget, but it is temporary. As soon as I finish the TODO task, I remove the comment. In this case the DecSamplesUntilResult() call can be moved inside the CalculateResultPair() call so that the latter call returns true if the next stage has enough samples to run. Then to convert the for loop to a while loop, we can use a pointer to the current filter stage instead of array accesses and put the CalculateResultPair() call inside the while loop condition. Then all we have to do inside the while loop is increment the stage info pointer to the next stage.

Finally, did you notice the question in the last comment? That's been there for quite a while, and I finally got around to answering it. The answer was actually right there in the previous comment, but it's not very clear. The reason why the DMA source needs to be reset is because the destination size is bigger than the source size, and if it wasn't reset, the DMA source pointer would keep incrementing until the destination was full - right past the end of the buffer. The relevant comment was moved and reworded. The redundant comment before the test for the final stage was removed.

Better Code With a Single Relevant Comment


Now look at how much cleaner the final code is:

void CFilter::Run(void) {
   SStageInfo *pStageInfo = _rgStageInfo;

   if (_fDownConverting) pStageInfo->DownConvert(_centerFrequency, _cChans);

   while ( (pStageInfo != &_rgStageInfo[_cStages]) &&
           pStageInfo->CalculateResultPairForNextStage(_cChans) ) {
       ++pStageInfo;
   }

   if ( pStageInfo == &_rgStageInfo[_cStages] ) {
       if ( _csHoldoff ) {
           _csHoldoff--;
       } else if (_fDownConverting) {
           CFft::EdmaTriggerAll(_mid, pStageInfo->GetBuffer(),
                                _rgixFirOutput, _fDownConverting);
       } else {
           // Reset EDMA source address to simulate
           // a FIFO source to a larger sink buffer.
           EdmaSetSrc(_hEdmaFirOut, pStageInfo->GetBuffer());
           EdmaSetChannel(_hEdmaFirOut);
       }
   }
}

The code pretty much stands on its own now and clearly shows its intentions. The only surviving comment explains why the DMA source is getting reset because that's normally an odd thing to do. When I have to come back to this code six months from now, I'll be able to easily see what it does. I can read the code without having to slog through irrelevant or redundant comments or tedious details that should be handled at a lower level. Instead of becoming mired in byzantine logic, I can get on with the task at hand because the code's intent will be obvious. That is the goal of well-written code.


Follow Up: A Taxonomy of Code and Comments
at

No comments:

Post a Comment