Don't "Namespace" Filenames

When I first started this blog, I wrote up a rant about directory names that mislead about what their contents are. At the same time, I complained about adding unnecessary morphemes to directory names, things like "db" or "model" or such-like.

I've recently become annoyed with the habit of -- for want of a better term -- "namespacing" directory names and file names. I'm not saying not to use good namespace discipline in source code. What I mean is having a directory structure like this:

  zoo/
    zoo_iface/
      zoo_cmds.hpp
      zoo_cmds.cpp
      ...
    zoo_model/
      zoo_cage.hpp
      zoo_animal.hpp
      ...

Why does everything have to start with "zoo"? Look at it this way: if that was how we named people, then everyone's last name would be part of their first name and also part of their middle name. Instead of calling someone Mary Jane Smith, we would know her as MarySmith JaneSmith Smith. Some people make it even worse by prepending the full directory name onto the file name -- like "zoo_model_cage.hpp". Poor Mary Jane would be MaryJaneSmith JaneSmith Smith.

Of course, a source file name may be dictated by its contents -- you're following our C++ standard, right? -- so my ire is directed at redundant directory names more than filenames.

This principle doesn't have to stop at source files.  It boggles my mind that raw bug testcases where I work start their life with names like this:

  /home/bugs/prod1_bugs/comp2_bugs/bug6003.tgz

For crying out loud, I know this is a directory full of bugs, can't we just call each bug something like:

  /home/bugs/prod1/comp2/6003.tgz

?

Don't Avoid No-ops

A form of spurious case analysis that seems to come up a lot in the code I have to work on right now falls under the category of "avoiding a no-op". Well, don't add cruft to your code just to avoid a no-op.

Here's the one that I see hundreds of:

  if (ptr != NULL) {
    delete ptr;
  }

Instead of:

  delete ptr;

But the line that caused me to post this today was this:

  if (!isalnum(str[i]) && (str[i] != '_')) str[i] = '_';

Why complicate the if condition just to make sure and not overwrite an underscore with an underscore? In reading through this code I paused to consider when an underscore could occur, and whether something special had to happen, only to read a little further and see that someone was guarding against a no-op. Good lord, just perform the no-op so I can read this more easily:

  if (!isalnum(str[i])) str[i] = '_';

C++ Static No Longer Deprecated

Wow, a long-held piece of geek trivia is no longer true.

This Stack Overflow article points out that in a recent draft of the upcoming C++0x revision to the C++ standard, file-scope static declarations are no longer deprecated. For, oh, 20 years, it's been one of those finer language points that you can whip out to show your sophistication and look down on the ignorant. But between the August 2010 draft (pdf) and the November 2010 draft (pdf), the section deprecating static has been struck!

If you're unfamiliar with the issue, it's that anonymous namespaces provide a more general solution to the multiply-defined symbol problem. Instead of:

  static int num = 0;
  static int read_num() { return num; }

the preferred C++ way to limit visibility of global objects to file scope was:

  namespace {

  int num = 0;
  int read_num() { return num; }

  } // anonymous namespace

The rationale is that you can put other symbols you wish to hide -- like class definitions -- in such a namespace, but static doesn't help you with those. As the most general solution to the problem, namespace wins and static loses. But I suppose that 20 years of failing to break people of the static habit led to a recent change of heart.

Suits me. I had been consistently using anonymous namespaces for quite a while, both to be modern and especially since it does frequently come up that I want a type that is only used in one file. But static declarations are better for self-documenting the code, so I am happy to welcome them back.

Only problem is, I need a new piece of C++ trivia to show off with.

Don't Give Variables Negative Names

Sometimes programmers get caught up in the absence of something. Don't let that leak over into the name of a variable or data member:

bool Job::acceptable(const JobCandidate &applicant) const
{
  bool noFelonyConvictions = findFelonyConvictions(applicant);
  return noFelonyConvictions && qualified(applicant);
}

The noFelonyConvictions variable documents the code in a readable way, but it gets silly if the code ever changes so that you have to initialize it, or if someone ever wants to know if there are felony convictions. Things become less readable:

  bool noFelonyConvictions = true;
  if (applicant.findRecords(FELONY)) {
    noFelonyConvictions = false;
  }
  return applicant.qualified(!noFelonyConvictions);

It all makes more sense if you name the variable felonyConvictions, which initializes naturally enough to false, can be set to true if one is found, and can be negated to show absence. Also, if you later become interested in the number of convictions, there will be fewer code changes if the variable changes from a bool to an int.

Whenever you notice yourself putting a "no" into a variable name, get rid of it.

Open Your Eyes!

Here's a sweet piece of code (names have been changed to protect the guilty):

if (calculated_condition == some_test()) {
  // Ancient comment.
  // T intermediate = partial_value(val);
  // calculated_val = more_calculations(intermediate);
  // Merely elderly comment.
  // calculated_val = better_calculations(val);
  calculated_val = val;
}
else {
  calculated_val = val;
}

Good lord! Maybe if there wasn't a compulsion to comment out old code, it would have been clearer what was happening here, and this would have been replaced with the one-liner. Instead, a few years later I have to spend a couple minutes reading the whole thing, maybe pausing right at the beginning to figure out when the condition is met, only to find that none of it matters.

It's even more fun when several lines of code are commented with a single "/*", or the hated "#if 0". Then as you're grepping to figure out where something decrepit like more_calculations() is used, it looks like it's used in hundreds of places.

Delete dead code!

Obfuscated Directory Names

I have to rant about some confusing directory/file/object names in a project I inherited at work. The guy who did it is a competent, experienced engineer. This isn't an attack on him or his work habits. If there's a moral to this rant, it's that good developers can make this kind of mistake, so the rest of us have to be extra careful not to make it.

Now, on to my rant. This C++ project -- for purposes of anonymity let's call it the "zoo" project -- interfaces with a Tcl interpreter. One confusing thing about the project organization is that it doesn't follow Rules 1.1 and 1.2 of my C++ coding standard: each source file might define many different classes. That compounds some other class naming issues, like the presence of classes with ridiculously similar names, like "ZooObj" and "ZooObject", or naming files after an abstract base class, but changing the capitalization: zooObject.hpp.

But the real problem has to do with directory names that don't describe their contents well, and that all sound the same. Here's the directory structure:

 ...
   zoo/
     tclApi/
     zooDb/
       zooDb/
       zooDbTcl/

The morpheme "db" here just means "I am defining a set of classes that model the zoo data". Well, no kidding, it's the zoo project. Things that go without saying should just not be said. In object-oriented design, responsibility for every function belongs to some class or another. You should never need special directories called "db" or "model" or "classes" or anything like that -- they don't add any information.

The next indication that something has gone wrong is that we have a directory called "zooDb/zooDb". That's one duplication that we could surely live without; you also have to wonder why there are two directories with "tcl" in the name. It turns out that one rationale for the directory structure is that there are a set of classes which will be compiled into a library with no dependencies on the larger project, so that it can provide a Tcl package to any interpreter. So maybe there's a method to this madness, we need the zoo/zooDb/zooDbTcl directory for that separate Tcl package code. Oops, turns out that's not the case, it's zoo/zooDb/zooDb that holds that. Arrrggggh!

What can we learn from this bad example? First, don't add directory structure where it's not needed. You probably need one directory for each object library you're building; any more than that just makes it confusing when someone is trying to walk through your code. Second, directory names and class names should not contain superfluous strings like "db", "model", or "class". Finally, don't add misleading strings -- like "tcl" in the example above -- that lead away from the directories or files that should be described with them.

If we apply all that, we end up with a much cleaner directory structure:

 ...
   zoo/
   zooTclPackage/

With that structure, when a new developer is looking for the classes that are in the dependency-free package, he knows just where to look. When he's looking for other zoo-project files, he knows where to look. It doesn't matter that some code interfacing with Tcl is mingled in a directory with stuff that knows nothing about Tcl -- like the food on your plate, it all gets mixed together when you eat it anyway. As an added benefit, the use of that flat hierarchy and putting every class in a separate header file will hopefully cut down on the temptation to create evil twins like zooObj and zooObject.

Eschew Case Analysis

It ticks me off when I see code like this:

if (str != NULL && *str != '\0') {
exists = true;
}
else {
exists = false;
}

This is a pretty simplistic example, and most -- not all! -- programmers would get rid of the exists variable and the accompanying code, and replace it all with (str && *str). But it's all too common to find more subtle versions of this same kind of logic bloat. And when it's even more complicated and nested... can you say "cyclomatic complexity"?

This is a special case of what Dijkstra very cogently described as "spurious case analysis". That link is well worth reading, by the way.

Dijkstra was certainly on the science side of the engineering-science frontier. I once heard him say that the world would be a better place if all computer programs had to be chiseled in marble. I don't agree with that, but his commandment to avoid case analysis resonates with good software engineering practice: be concise, self-document, code for testability.

If you can avoid case analysis, do so. Otherwise....