gtg-contributors team mailing list archive

[Bryce Harrington <bryce@xxxxxxxxxxxxx>]

Hi all,

I've noticed all over the codebase there are lines of code commented
out, but without an explanation why they're commented out.

I don't think this is a good practice.  It means that one of these
happened:

 a.  I was unsure of my fix
 b.  I wasn't sure what the original code was supposed to do
 c.  I needed to disable it to work around some problem
 d.  I removed or broke other code that this code depends on
 e.  I started implementing something but haven't finished it yet
 f.  I need this for debugging problems that might still exist

Obviously none of these are great situations to be in, but it happens.

Ideally, commenting out a line of code should be a signal to yourself
that one of these things has happened, and that you probably should ask
for help before merging it to trunk, and it should stay in a branch for
now.

But that may not always be possible.  So more practically, when
commenting out code please ALWAYS explain why you commented it out.
This enables other developers (who may know the code better) to figure
out and solve the problem.

The comments needn't be long.  Imagine it as a privmsg to a fellow dev,
rather than as a personal reminder note which can be too terse for
others to comprehend.  A 'FIXME' or 'TODO' label might be appropriate.

Also, if the issue is related to a bug, include the bug report number.
If there isn't a bug report, consider filing one if you have time.


A special case I see a lot are commented-out print statements for
debugging.  In most cases something like this:

  #print "Testing: ", 1, 2, 3

can be better replaced with this:

  Log.debug("Testing %d %d %d", 1, 2, 3)

And if you don't think it's worth using Log.debug(), then it's probably
not worth keeping the print statement at all!  :-)


Going forward, I think we should turn a stern eye towards commented-out
code.  Existing commented-out code should be reviewed and either removed
or justified.  This will help make the codebase easier to understand and
will help resolve the uncertainties / bugs / workarounds / incomplete
works that the commented-out code implies.  It will also help tidy up
the code.  

What do you guys think?

Bryce