Don’t keep track of information you don’t need


This is sort of an extreme corollary to Don't save anything you can recalculate. Sure, it sounds like such an obvious principle, but many people fail to understand its consequences.

Let's look at the principle again. Don't keep track of information you don't need. I remember being asked to look at a customer's program, and one thing that struck me was that the program had a bazillion different flag variables that it spent a lot of time setting and clearing. Here's an oversimplified example:

void CConfiguration::ShowDialog(HWND hwnd)
{
  m_fShowingDialog = true;
  DoModal(hwnd);
  m_fShowingDialog = false;
  m_fDialogHasBeenShown = true;
}

A naïve reading of the code would lead one to believe that the main purpose of the program was to set and clear flags! Upon closer inspection, I found that most of the flag variables were write-only. Although the code went to great pains to update the flags as it ran, there was no code that actually cared whether the flag was set or not.

I asked the customer liaison about this, thinking that maybe I missed something. Maybe the variables were being used in a subtle way that I failed to appreciate. Maybe they were used during debugging.

The customer liaison got back to me. "When I asked them about it, they just said, 'Ah, yes.' Apparently that code was written by one of their guys named Bob. I guess he likes to set and clear flags a lot."

But is there any code that actually checks the values of those flags?

"Probably not. He just set and cleared those flags because he figured maybe someday he might need them."

Indeed, once in a while, he actually tested one of the dozens of flags he spent most of his time setting and resetting. But the vast majority of the flags were write-only. (And since most of the flags went unused, I suspect that many of them weren't even accurate.)

Obviously, this is an extreme case of write-only variables, but it illustrates the point. Next time, we'll look at one of the consequences of the principle of not keeping track of information you don't need.

Comments (19)
  1. Chriso says:

    Maybe Bob has

    m_fHasToGoToSourceControl = false;

    somewhere, which he is checking? :-D

  2. John says:

    Isn’t it obvious?  This is about job security.  You throw a few hundred random lines of code in that do nothing and anyone else who takes a look at it will be confused.  Also, this is why you should always throw a few for loops in that do some irrelevant calculation a couple million times.  You can remove them later and call it a performance improvement.

  3. Greg says:

    At least this was only bools – I knew one "Bob" who loved to store information as delimited strings. He was familiar with the concept of arrays, as shown when he used arrays of delimited strings, but the light bulb never lit up.

    Job security like that only works if you can convince management that a new guy can’t come in, delete everything, and have a fresh version done in a week. My Bob managed it.

  4. Tim Smith says:

    I know it isn’t the same thing, but I once ran into a class that had seven flags and only one of them could be set to true at a given time.  In other words, a enum would have worked just fine.

    Sure, there isn’t anything technically wrong with it, but when looking at the state machine, if you didn’t know that the flags were mutually exclusive, then the state machine had 128 possible states instead of just seven.

  5. Duke of New York says:

    I’ve had to clean up after a Bob, but instead of "deleting everything," I just kept chiseling away at the Bob code bit by bit (lol) until it was gone.

  6. Kaenneth says:

    I’m currently annoyed by complier warnings-treated-as-errors for this problem…

    If I temporarally comment out a line of code that uses a variable, I need to also comment out everywhere it’s set, and it’s declaration and then remember to uncomment them all later.

    I could clutter the source with ifdefs….

  7. ton says:

    Hmmm… I really do wonder that if Bob is sitting there writing reams of unused code and letting it get into production should he really be coding? Or should his check-ins be automatically redirected to the TheDailyWTF? That is if he is *even* using source control I somehow doubt it.

  8. Merus says:

    I’ve never gotten "write terrible code" as job security, to be honest. A good employer will find or invent another project for you to work on because they want to keep you around, and a bad employer will let you go, giving you motivation to go find a good employer.

  9. Vijay says:

    Maybe, the title of the post should have been -"Don’t be a Bob" :)

  10. Duke of New York says:

    ‘I’ve never gotten "write terrible code" as job security’

    That’s probably because you’re competent.

  11. niyazpk says:

    I thought most of the compilers can detect and remove the unused variables and code that has no consequence.

    Am I wrong?

  12. niyazpk: For variables local to a procedure, certainly.

    For variables local to a source file, perhaps (but it depends on the language when a variables is local to a source file. In C++, private fields can appear unused but still be unsafe to remove).

    For global variables and public fields, no chance with ordinary compilers.

  13. Jonathan says:

    / me verify that my RSS reader didn’t confuse The Old New Thing with TheDailyWTF

    Tim Smith – It can happen gradually:

    1. Code only has 2 options, bool is great for that. So we have IsOption1.
    2. Need to add Option2. The person writing Option2 doesn’t know about Option1, or doesn’t think about whether they’re mutually-exclusive or not. So we get IsOption2 instead of {none, Option1, Option2}.

    3. Repeat as (not) needed.

    Also, I’ve found that some people can’t quite grasp the concept of enum.

    niyazpk: The CPU time wasted on the flags is the least of worries here. The amount of brain-time to grok the code and understand that it does nothing is the point (at least until next time).

  14. MadQ says:

    I’m surprised that not a single Functional Programming fanatic has chimed in yet with an obligatory "State machine? I don’t need no stinking state machine!!1!†" statement.

    †The 1 was intentional.

  15. wtroost says:

    It’s very common to see code written with the intention that it might be useful some day.  In my experience it’s just too hard to predict the future.  But then, I get labelled short-sighted for not writing everything as a library. :)

  16. Peter says:

    I actually see this as something different: it’s a pattern that’s occasionally useful as a hack, and which Bob has put into his muscle memory for dialog boxes.

    For example, some time in his past, he had to deal with Dialogs "A" and "B" that both called setup_widget().  Then a requirement came in that setup_widget() do different things based on the caller.  The correct thing to do is refactor, because setup_widget() now has to be two different functions.  The hack is to set the calling/is_called flags.

    Similarly, I can imagine a "file save" dialog function that should only be called if the underlying data has been changed.  Clearly you need some sort of flag — and Bobs system will always provide that kind of flag.

    Not to say it’s good code, mind; but it is understandable.  It’s not job security, it’s just unfortunate training.

  17. Jim says:

    I like to leave space for code that might be written one day as much as the next guy does. But that’s not the same as writing the code in advance.

    With write-only flags, there’s an absolute certainty (for certain values of ‘absolute’) that at least one of them will not be updated correctly, and because its never used, you won’t know about that bug. So one day when you write a small bit of code that needs to rely on the flag, you’ll have to search the entire codebase for places it should be set, anyway.

    So, you should probably replace all reads of m_fShowingDialog with dialog.IsVisible() – or whatever suits the dialog API in use – and thus not be caching data that you don’t need.   ;)

    Also, in the sequence "m_fShowingDialog = true; DoModal(hwnd); m_fShowingDialog = false;" I didn’t see any locking, so one day in the future you know that someone’s going to add a thread that assumes m_fShowingDialog is accurate and complain that its not always working right. Of course, even then there will be problems with the typical single-threaded GUI library, but that issue isn’t fixed by an inaccurate flag. (And there’s plenty of cases where the flag introduces a race condition even if your resource happens to be thread safe, so caching without knowing what you’re doing is still wrong.)

  18. Cheong says:

    Maybe it’s written by staffs of one of the companies that sets target amount of LOC for employees.

    Setting/clearing flags is a good way to get away a casual code review when counting LOC(consider even experts like Raymond have to ask before confirming they’re not needed, their uselessness is not readily detectable by whoever sitting above them). This is a useful way to make sure everyone in team meet the target.

  19. Because there is no need for it to keep track of that information.

Comments are closed.

Skip to main content