Friday, January 10, 2014

(Code) Comments Lie

I have a young friend, who has entered the halls of academia to learn how to program. He's expressed frustration that the teachers spend what seems a disproportionate amount of time obsessing about code comments.

I come from the eXtreme Programming school which took a dim (and controversial) view on commenting for the sake of commenting, or often even, commenting for much of any reason at all. I stumbled on a great one just now. I'm still chuckling.

In a relatively small code base of some embedded code I have a function that looks like:

void setupPrintf(void)
{
    // set PORTC0 for UART
    PR.PRPC &= ~PR_USART0_bm; // enable it
    PORTC.PIN2CTRL = PORT_OPC_TOTEM_gc;
    PORTC.DIRCLR = PIN2_bm;
    PORTC.PIN3CTRL = PORT_OPC_TOTEM_gc;
    PORTC.DIRSET = PIN3_bm;
    PORTC.OUTSET = PIN3_bm; // Drive Pin3 High
    USARTC0.CTRLB |= USART_TXEN_bm;  // enable transmission
//  USARTC0.CTRLB |= USART_RXEN_bm; // no need for reception
//  USARTC0.CTRLA = USART_RXCINTLVL_LO_gc; // no need for reception
    USARTC0.CTRLC = USART_CHSIZE_8BIT_gc | USART_PMODE_DISABLED_gc;// 8N1
    USARTC0.BAUDCTRLA = 123; // 230400
    USARTC0.BAUDCTRLB = (-4 << USART_BSCALE0_bp);
    stdout = &UART_STDOUT;
    backgroundWorkAdd(&PrintTask);
}

Some gnarly C code filled with lots of lovely spec sheet abreviations. We're not a big team, just two of us. We've had some changes lately in our hardware and are doing some rewriting to adapt. Amongst others, we wrote the following code (in a separate function in another file):

    ....
    PR.PRPC &= ~PR_USART0_bm; // make sure it is enabled
    USARTC0.CTRLB |= USART_RXEN_bm; // no need for reception
    USARTC0.CTRLA |= USART_RXCINTLVL_LO_gc; // lo level interrupt
    USARTC0.CTRLC = USART_CHSIZE_8BIT_gc | USART_PMODE_DISABLED_gc;// 8N1
    USARTC0.BAUDCTRLA = 123; // 230400
    USARTC0.BAUDCTRLB = (-4 << USART_BSCALE0_bp);
    ....


Do you see it? I didn't either at first. Because I don't pay much to comments usually. The amusing line is the second line in the new code:

    USARTC0.CTRLB |= USART_RXEN_bm; // no need for reception

That line was copy/pasted from the original body. The comment "no need for reception now" was actually a reference to the fact that the whole line was commented out, not what the code does. Because the code does the exact opposite. So the new copy, no longer commented out, contains a single line of code where the code does the exact opposite of what the comment says it does.

Less is More. It would have been better if I had just removed the 2 lines in the first place many moons ago. And you should never copy/paste comments. Period.

No comments:

Post a Comment