Subscribe to Stuck in an Infiniteloop        RSS Feed
*---- 1 Votes

This is Why You Comment Your Code

Icon 14 Comments
Earlier today I was working on an application that does a lot of specific byte manipulations. Each step depends on the correct output of the preceding step and thus if 0x00 is 0x01 you get the wrong answer. So, there's this one piece of data that comes from a file that is supposed to be written in a very specific way: it is plain ol' decimal in the file, but needs to be represented by 2's complement of an unsigned 32 bit integer in memory before a ton of operations happen on it.

So this is how the code reads:

/*   Subtract one and then subtract from ffff     */
unsigned int someUnhelpfulName = 0xffff - ( theInput - 1);

/* do other stuff, like shifting */



So I'm sitting there looking at this code going wtf (at the time I didn't know that 2's complement was the desired output). I then to proceed to spend the better part of an hour reading the spec to figure out what the hell was going on. All this guy had to do was put this:

//get two's complement of the number from the file 



Not tell me what math he's doing, which I can plainly see. It would have been better to have no comments at all!

Comment your code, meaningfully!

14 Comments On This Entry

Page 1 of 1

janne_panne Icon

12 April 2012 - 11:14 PM
Too often the comments just read what is happening, not why it's happening. Everyone can see from the code what is happening! But no one has a clue (without further investigation) why something is done if the commenting is bad.

So always answer the question WHY, not WHAT.

I'm still learning that, too often I just tell what already is clear from variable/method names. :P
3

cfoley Icon

13 April 2012 - 04:14 AM
Better yet, call the method getTwosComplement()
1

nick2price Icon

13 April 2012 - 06:56 AM
The classic comment I see way to much of

/*Method someMethod*/
public void someMethod(){}
1

nick2price Icon

13 April 2012 - 06:56 AM
The classic comment I see way to much of

/*Method someMethod*/
public void someMethod(){}
0

jon.kiparsky Icon

13 April 2012 - 07:16 AM
Recently I had a co-worker come to me with a problem. He was trying to deal with a row in the database that was labeled something like "expiration date" but the dates were all completely goofy, like some time in 2187 AD. He'd tracked it back to the original code that had created the data, and found a line that took a date, and if a certain string happened to be eight characters long, it multiplied the last character of that string by 32768, then it added that to another date and stored the result into a date field.

It was nice for me, because I got to look smart, but he'd never seen anyone packing two values into one field that way...
(It turned out that the "is this string eight characters long" part was checking for existence. Either that field held "00000001" or null.)

Needless to say, this would have been a fine opportunity for the original programmer to deploy a few well-made comments.
0

Jstall Icon

13 April 2012 - 09:24 AM
I for one see allot of value in descriptive comments.

I've heard of whole shops who make it a policy not to comment of any kind because "if people can't tell what you are doing by looking at your code then it is badly written code" .. that seems like a horrible practice to me.Just had a conversation with a co-worker about this today, so I digress :).

Yes, please comment your code in a useful manner!
0

Shane Hudson Icon

13 April 2012 - 12:01 PM
Nick, at university we get 10% of the marks deducted for not doing this:

/*
 * getName
 * This method gets the name
 * @return name The name
 */
public String getName() { }



Sure it makes it look neat.. but it drives me insane... things like that are so obvious!
1

skorned Icon

14 April 2012 - 03:15 AM
@Nick,

I'm sure an IDE such as Eclipse could be persuaded to automatically generate crap like that? And possibly even hidden/collapsed from view while actually writing productive code and comments.
0

Curtis Rutland Icon

14 April 2012 - 08:36 PM
We have a guy at work, a genius really, who doesn't believe in comments. It's not as crazy as it sounds, because he thinks that if you need to comment your code, then you should refactor it to be more readable. If your method is too long, factor it as other methods. If it's complicated, break it out into steps (clever is cool for your own code, but for other people, readability is king). Method names should be descriptive enough to know what it's supposed to do without spelling it out.

And the language we use (C#) allows for this in a really cool way: extension methods. You can "attach" methods to classes that you don't the source to. It's actually just a static method in a static class, and the first parameter is an instance of the class its attached to, but using them makes your code so much more left-to-right rather than inside-out. It's easier to read this:

instance.Method1().Method2();


than this:

Method2(Method1(instance));


Handy little language feature.

I still prefer some comments explaining why, but it's very easy to read his code.
2

KYA Icon

15 April 2012 - 01:41 PM
The worst thing ever:

//increment x
x++;



THANKS GUY!
0

jon.kiparsky Icon

15 April 2012 - 11:39 PM
if you need to comment your code, then you should refactor it to be more readable

Nothing to add to that, I just thought it should be said a little louder.
0

SwiftStriker00 Icon

16 April 2012 - 12:14 PM
This is why i comment code. Because i do things like this:
Quick background, i was working on a raytracer. Can you explain what I did? without reading the comment?
RayColor ShaderTool::gridProceduralShade( Shape * plane, Vector3 intersectionPoint )
{
        RayColor red = RED, yellow = YELLOW, white = WHITE;
        RayColor returnColor = YELLOW;

        double TILE_W = 20, TILE_H = 20;
        double Iy = intersectionPoint.getY();
        double Iz = intersectionPoint.getZ();

        int y_tiles =  (int)( Iy / TILE_W );
        int z_tiles =  (int)( Iz / TILE_H );

        int row = ( y_tiles % 2 == 0 ) ? 0 : 1;
        int col = ( z_tiles % 2 == 0 ) ? 0 : 1;

        //comment normally goes here
        returnColor =(Iz > 0 || Iy > 0 ) ? (( col ^ row ) ? YELLOW : RED) :  (( col ^ row ) ? RED : YELLOW);
        if( Iz > 0 && Iy > 0 ){
                if( returnColor == red ){
                        returnColor = yellow;
                }else{
                        returnColor = red;
                }
        }

        return returnColor;
}

Spoiler


I haven't looked in this code in a while ( almost a year now ) so it could be better, but my prof and my partner had to know what was going on. I comment generally in a few key spots.
1. Method headers, explain the general purpose of the method
2. Logical chunks, usually beginning of loops, basically explaining the goal of a group of code
3. Any hack-y, or mathematically intense line ( aka KYA's 2s-complement step )
1

KYA Icon

16 April 2012 - 05:12 PM

jon.kiparsky, on 16 April 2012 - 12:39 AM, said:

if you need to comment your code, then you should refactor it to be more readable

Nothing to add to that, I just thought it should be said a little louder.



I would say this is an excellent principle, but there are a few exceptions, most involving some sort of math/math trick.
1

Skydiver Icon

10 May 2012 - 06:10 PM

jon.kiparsky, on 13 April 2012 - 07:16 AM, said:

Recently I had a co-worker come to me with a problem. He was trying to deal with a row in the database that was labeled something like "expiration date" but the dates were all completely goofy, like some time in 2187 AD. He'd tracked it back to the original code that had created the data, and found a line that took a date, and if a certain string happened to be eight characters long, it multiplied the last character of that string by 32768, then it added that to another date and stored the result into a date field.

It was nice for me, because I got to look smart, but he'd never seen anyone packing two values into one field that way...
(It turned out that the "is this string eight characters long" part was checking for existence. Either that field held "00000001" or null.)

Needless to say, this would have been a fine opportunity for the original programmer to deploy a few well-made comments.


Although, I agree that something tricky like that should have been commented, there are somethings that are common idioms that involve math tricks and/or conventions that we as programmers just accept as common. What's bad is when they fall out of common use, and then we have to go look at the old code and pray that somebody put in a self explanatory comment. By common idioms, I mean stuff like:

divideBy2 = x >> 1;
multiplyBy2 = x << 1;
isEven = !(x % 2);
next4KBoundary = (bytePointer & ~0xFFF) + 0x1000;
indexOfLeftChildNodeInABinaryTreeStoredInAnArray = parentIndex * 2;
indexOfRightChildNodeInABinaryTreeStoredInAnArray = parentIndex * 2 + 1;



To some programmers, it's old hat for them to count the number of 1 bits in an integer just using a series of XORs and a comment is not needed. I unfortunately never recognize that pattern and would need to see a comment over it to see what is happening if they didn't use meaningful variable names.
0
Page 1 of 1

November 2014

S M T W T F S
      1
2345678
9101112131415
1617181920 21 22
23242526272829
30      

Tags

    Recent Entries

    Recent Comments

    Search My Blog

    1 user(s) viewing

    1 Guests
    0 member(s)
    0 anonymous member(s)