Hacker News Reader

Comment by zwegner | Hacker News Reader

At my last job I wrote a script to walk over git-log and add up all the line number differences (skipping merges). By the time I left, my net code contribution was something like -290kloc. I was the only person with a net negative, though it was a smallish team.

With the sheer amount of bad code people write, I expect to do a lot of deleting, refactoring, and rewriting, and I'd hope managers/fellow team members would be able to see the value in that. But sadly, they usually don't.

View on HN

People also leave dead code around instead of deleting it. That dead code is poison to the maintenance programmer because he's spend plenty of time trying to understand what role it plays in the system of tracing a complicated bug.

I'd say it is open season on commented-out code. If code has been commented-out since a few weeks it is good to delete it just as a matter of course.

This is actually one of my favorite things about version control. If I know it's been committed at one point, I can delete anything and breathe the sweet air of clean code.

But how does the next guy know it's been committed at one point?

You should never commit commented-out code. That's what version control is for. If you need that code back, just fetch it from the repo.

This is a bit of a tangent but I've been on teams that demanded you rebase everything so that there's a linear history.

Of course this leads to nasty conflict resolutions, so they solve that by squashing all of their commits before rebasing.

Now your code is no longer in the repo.

Well, I definitely prefer the rebasing route, but with the caveat that everyone's working on the same branch, so there's not much divergence. The only rebasing occurs when somebody commits before you do, and it's only your own local commits that need conflict resolution.

That seems like a git anti-pattern. On the other hand, one or two repos where I work have developed some, uh, interesting network graphs, as various branches have merged and evolved.

What do you mean by "your code is no longer in the repo"?

I suppose (s)he means that, if you squash commits, you lose the intermediate states containing code that was later deleted. It's not necessarily bad - squashing is a useful tool to clean up your changes before you merge them in.

If you squash together the commits that added and deleted a line, that line never existed.

But if it never appeared in the main branch then it doesn't really matter, does it? You don't want to clutter up the history with experiments and false starts.

Weirdly: yes, I do. It's handy, and those experiments show what I've done to tackle a problem. Numerous times those experiements that I've left in the repo somewhere have come in handy.

Hell, we keep our feature branches around here. *shrugs

I'm not sure that you don't. Having a history of how you got to the current version could help future maintainers understand why you made the decisions you did, and prevent them from making similar mistakes.

I tend to work out of a private branch, and commit early and often. So I end up with a lot of commits that say "fixed foo to be 1", then "oops, I meant it to be 2", then "No, it should be -7", etc. I'll drop a tag, then rebase a group of commits to re-order them (so all the fix, to a fix, to a typo mistake that shouldn't have been there) are together. Then sometimes I'll drop another tag, then squash, then rebase onto the main branch. Those temporary tags then get pushed only to a separate repo that only I write to (the one that everyone else uses gets the cleaned-up presentable code).

Agreed, but I have seen one time (and only one time) where deleting a piece of code instead of leaving the comment generated a compiler bug that generated some weird code. Put comment in - good code, delete comment - bad code. It was really, really odd. Add an ignored assignment (a=1), good code. So, we left a comment with a "DO NOT DELETE". Next version of the compiler fixed the bug and we deleted the comment. Weirdest damn thing I ever saw.

What language was it?

C compiler from a unix vendor in the late 90's.

Maybe the dead code was preventing the C compiler from carrying out a buggy "optimisation"?

Could have been. Although a comment or a printf kept the code working, and removing that line broke the line above it. Next compiler release fixed it. It was just odd and a lesson in compilers are not perfect.

Yes, but in my book, that's not commented out code, that's an (undocumented) compiler directive.

And I've done similar things with "undocumented compiler directives" for XSLT processors before. If you didn't leave the comments in then you got the dreaded "GregorSamsaException". I refer to it as "dreaded" because folks on our team dreaded it... the exception occurred at unpredictable times and who the hell was Gregor Samsa anyway, and what did that have to do with our Java application?

Turns out, Gregor is the main character of Kafka's "The Metamorphosis" and the exception was the brilliant idea of someone who wrote the XSLT transformer and probably thought it was cute. (It wasn't.) It was an internal exception in the transformer. (Get it? Metamorphosis? Transformer? Never mind.) It occurred when a certain buffer filled up, and the exact length of the XSLT input file affected that, so adding a few lines of commented-out content would make the error appear or disappear.

I wrote a lengthy essay explaining the above facts, and used commented-out excerpts of that essay as padding in the file. Yes, I was trying to be "cute" also, but I was too young to realize that was a bad idea.

Working on projects that have existed for years, and have been maintained by several different people, you don't always know what code there is that you could possibly get back out of the repo, without exploring it version by version.

I can imagine there being instances where leaving commented out code could be helpful -- including comments about why you thought it could be helpful, and why it is commented out!

I can almost guarantee that, if your code bases version history is too messy to find old code, then it will be even worse if you were to favor commenting out to deleting.

That said, I do see where you're coming from. Part of the issue is that we (well, most of us) don't have good ways of searching old code. There is Codeq ( https://github.com/Datomic/codeq ), which is prettydamncool™ ...hopefully we'll start to see more systems like it.

Yes, well -- and I suppose I should add that I'm not advocating for an environment in which keeping commented-out code around is a good idea. Just suggesting that there could exist situations in which it is a reasonable thing to do.

I've increasingly been noticing that a lot of really good development practices make sense if you're starting from scratch and can employ them right away, but sometimes if you've got years or decades of legacy code and legacy process (and code that was written as the result of legacy process) to deal with, the right thing to do isn't always so clear.

(I've unfortunately/fortunately been working some recently on a very large, very old code base that mostly doesn't need updating. Trying to unravel its mysteries enough to add a new feature has been an interesting experience.)

Ok, I can understand that a good version history will allow you to find what you are looking for.

But how will you know that you should look for it in the first place?

I don't agree. You should probably have a weak preference for storing unused code only in VCS, but if you think something will be needed soon I don't think it makes sense to delete it - c.f. "You should never keep values in cache. That's what disk is for."

OpenOffice.org went through contortions as it changed version control systems over the years. It got to the stage where devs would comment out code rather than deleting it because they didn't trust the VCS.

LibreOffice put the code into git and went mad with an axe deleting all the commented-out code.

Apache OpenOffice, on the other hand, still commits new commented-out code. http://mail-archives.apache.org/mod_mbox/openoffice-commits/... Possibly nostalgia for the good old days at StarDivision.

Occasionally I'll leave useful debugging log calls in there. It's a lot quicker to just uncomment the code when it's needed, rather than pulling that bit out from source code.

Also, even more occasionally, I'll leave some incomplete code commented out as an obnoxious reminder to complete it later. This is especially useful if the code wasn't ever committed before, so checking it out via source-control isn't an option. The very fact that it's not really supposed to be there is a good motivator to implement it!

Ideally you use log-levels and log-sources for this. Leave the logging calls in the production binary, but on DEBUG level. If there's a problem, bump the log-level up to DEBUG for just the subsystem you care about. Then you don't even need to recompile to get your logging information, you can just flip a flag - sometimes without even restarting the app.

I'll admit to just commenting and uncommenting log lines before, though - learning how the logging systems of major programming languages work takes some time, and there's an up-front cost to starting to use them.

The other thing is, you have a bunch of wasted calls to a logger if you don't want any logging to take place. Minor, but if you are working on embedded systems etc... you won't be using a logger, you'll more than likely just comment out debug statements and leave them in.

It all really depends on the application and how much performance is an issue.

Typically you wouldn't put a logging statement inside a hot inner loop anyway, as you'll get so much output when logging is turned on that it makes the logs worthless. For heavy algorithmic-intensive stuff, you'd often just record your loop counters, key invariants, and exit conditions, and then log them all at once when the algorithm exits.

I have little embedded systems experience, but what I gather from talking to folks who do is that they also use logging APIs, but they avoid logging from inside a hot inner loop (their log statements are usually around startup/initialization and when the system receives certain inputs), and they use custom log writers that write the logs to a host server or desktop system when plugged in for debugging rather than taking up storage space on the device itself.

> Typically you wouldn't put a logging statement inside a hot inner loop anyway, as you'll get so much output when logging is turned on that it makes the logs worthless. For heavy algorithmic-intensive stuff, you'd often just record your loop counters, key invariants, and exit conditions, and then log them all at once when the algorithm exits.

Debugging is a highly interactive art and sometimes operates inside a much faster feedback loop than that. Put those few log calls in the innermost loop, run it, scan the 20MB dump for a weird entry, fix, remove logger calls, and you're done in less time than it takes to consider which key invariants and exit conditions are worth tracking.

One of these days, I really need to post my C++ logger class. Don't know if you're doing embedded C++, but I've used this class in VxWorks, and if you're not logging anything (the default build with NDEBUG does nothing), it should optimize away.

For now, I can post a link to my inspiration: http://wordaligned.org/articles/cpp-streambufs

Extending from there is pretty straightforward, albeit you can hit some dark corners of C++ (I spent a few weeks tracking down a double link error caused by not templatizing an addition to the Logger that gave the capability to output to MSVS's debug window). This is also one of those very few cases in which I have justified using multiple inheritance, virtual inheritance, private inheritance, and templates.

Disagree wholly.

I don't think that a commit should be littered with commented out code, but there clearly are positive reasons to have some.

Besides, if the comment is not there, how can you be expected to know that there is old but still relevant code in the repo?

Out of curiosity: What are the positive reasons in having commented out code?

Sometimes just the presence of code triggers a bug because of dynamic dispatching / whatever, and a hotfix is necessary. You could remove, but then later programmers wouldn't have context to fix the bug properly.

    # TODO: Figure out why do_foo is triggering a bug
    # http://my-company.org/issues/42
    # def do_foo(self):
    #   ...

If the code is commented out, how can it be relevant?

"We cannot enable this yet, because foo isn't implemented yet, or because bar doesn't work correctly."

Or better yet, ban commented out code from getting into the source tree altogether.

If the code is in past commits, there isn't a reason to muddy the source tree with it.

Some people just don't understand that it's harder to maintain the code than to write new ones. They are probably not appreciating your refactorizations because they have not got to a point that it is almost impossible or super hard to write new code because the codebases are a large mess (they probably haven't got to this point because of your great refactorizations). But one day when they reach that point, I think they are probably going to miss and appreciate your past work.

I'd like to see that script.

Here is a sample implementation:

    git log --author=rav --numstat --no-merges --pretty=format: |
        awk -e '{ a += $1; b += $2; } END { print a; print b; print a-b; }'

produces this output (sum additions, sum deletions, net line additions):

    112529
    85383
    27146

Ha, nice, there's two options that definitely would've simplified my code (--numstat and --no-merges).

To count by author:

    git log --numstat --no-merges --pretty=format:%an
        | awk '
          author == "" { author = $0; next }
          /^$/ { author = ""; next }
          { added[author] += $1; removed[author] += $2 }
          END { for (author in added) {
            print author, "added", added[author], "removed", removed[author], "sum", added[author]-removed[author]
          } }'
        | sort -n -k 7

That is great! Thank you.

Unfortunately I forgot to grab it off my work laptop when I got laid off. I would've liked to have stuck it up on github...

It wasn't really all that tricky though, it took me a few hours to write. git-log has options for only displaying the status line of diff-stat for each commit, and then displaying the parents of each commit, and the author. You look to see that there's only one parent (so it's not a merge), parse out the X added, Y deleted numbers, and stick them in a dictionary keyed by name.

A lot of the script was just getting statistics like average/min/max/stddev line counts, and printing them nicely.

Reply on news.ycombinator.com