Hi, I 100% agree with Bryce on this.
I'm probably one of the most responsible for the commented print statements. The reason is simple : I don't use debug.log because I generally want only informations about a very precise point. Then, when I have too many, I quickly comment some of them out which means that I sometimes forget to remove them before merging. Sorry for that ! Lionel On Tue, 3 Aug 2010 15:17:52 -0700, Bryce Harrington <[email protected]> wrote: > 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 > > _______________________________________________ > Mailing list: https://launchpad.net/~gtg-contributors > Post to : [email protected] > Unsubscribe : https://launchpad.net/~gtg-contributors > More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~gtg-contributors Post to : [email protected] Unsubscribe : https://launchpad.net/~gtg-contributors More help : https://help.launchpad.net/ListHelp
