Partager via

Bad Names

  • Article

Over the past year and a half of working on the C# compiler we’ve refactored a lot of crufty old code. But a surprising number of the “refactorings” have actually simply been renaming a structure, class, or method to something more sensible. The compiler is showing its age; there are a lot of structures, classes and methods inside the compiler which no longer have sensible names (if, in fact, they ever did). Going through the whole thing all at once and renaming everything without changing the semantics of any of the code is a fairly cheap and easy way to massively improve code readability. After all, what is easier to understand, FUNCBREC or StatementBindingContext? (I think FUNCBREC at one point stood for “function binding record”.)

One of the things we’ve been looking for is names that “smell bad”. Sometimes the thing simply needs to be renamed and that’s the end of the story. Sometimes, though, the existence of a bad name is a red flag in the code that points to some chunk of functionality which could use some serious thought and additional code refactoring applied to it.

Functions with names like DoTheOtherThingToIt are obviously badly named. There’s no way to look at the call site and have the faintest idea what’s happening on the callee side.  But there are more subtle signs of badness. I’ve been compiling a list of these red flag particles, and here is what we’ve come up with so far:

Misc:

This is indicative that the Single Responsibility Principle has been violated.  Functions or structures which do “miscellaneous” work almost certainly do two or more things which are unrelated.

Base/Core/Worker/Helper:

These are often a sign of too much or too little abstraction. Suppose InferTypeSignature calls InferTypeSignatureWorker. If InferTypeSignatureWorker is doing all the work of inferring a type signature then move it back into InferTypeSignature. If it is not doing all the work, then describe what work it is doing, don’t just say that it’s working on something.

Fake/Real:

In the compiler we had a structure called FakeMethodSymbol, deriving from MethodSymbol. OK, it’s a symbol, it represents a method, got it. But in what sense is it a “fake” method? Is it a compiler-generated method as opposed to a user-generated method? Is it some kind of stub for a missing method? When is it legal to use as a MethodSymbol? Who knows? I sure didn’t. “Fake” doesn’t tell you anything about its purpose.

(It turned out that this is used when representing the method signature of an expanded method call that uses a C++ style “varargs” argument list. We’ve renamed it ExpandedVarargsMethodSymbol, and now it is extremely clear what that thing is for.)

Special/Normal:

Describe the property that makes the case special, rather than just calling it out as special. And try not to call things “normal”, just call them things. Rather than NormalFoo and SpecialFoo, have Foo and FrobbingFoo. Or have a NonFrobbingFoo and a FrobbingFoo both derived from abstract base class Foo.

Ex/2:

Oh, the pain. These should be used to solve COM interface versioning problems, and even then only as a last resort.

Those are the ones we've found in the compiler so far. What sort of bad-smelling name particles do you guys encounter in crufty code?

Comments

  • Anonymous
    June 12, 2007
    How about "NullableHelper_HACK"? :) Jon

  • Anonymous
    June 12, 2007
    "My" is a versatile particle. You can apply it to classes ("MyString") and functions/methods ("MyCopyFile"). All that you really know is that the class/function/method superficially resembles the non-"My" version, but differs in some subtle way that will be the source of a bug 2 months from now.

  • Anonymous
    June 12, 2007
    The comment has been removed

  • Anonymous
    June 12, 2007
    These are very nice examples; see also http://c2.com/cgi/wiki?BadVariableNames (like most c2 pages this is hoplessly disorganized, but some amusing examples nonetheless), and also all kinds of articles about how any comments in your code are another warning sign that the code needs cleanup.

  • Anonymous
    June 12, 2007
    I agree with Tom.  A class name suffix (Ex or number) is really the best choice when you need to add some random helper methods to a base class you can't change -- so you make a derived class called ClassEx.  If you mandate that such suffixes cannot be used you're just making work harder for everyone.  Either these helper methods will then appear in totally unrelated classes (with similarly uninspired names like ...Utility), or they cannot be written at all if they need to refer to protected members, or people have to think up some silly arbitrary name just so they don't use Ex/2. For the same reason, I disagree that Misc or Core/Worker are automatically bad.  These are often in fact the best names for the best division of code, quaint as they might sound.  Now Fake/Real and Special/Normal, that does sound bad!

  • Anonymous
    June 13, 2007
    Try using GhostDoc, it will cause you to use better names.

  • Anonymous
    June 13, 2007
    I didn't say that they were "automatically bad".  They are not. I said that they were "often a sign of too much or too little abstraction". Which they are.

  • Anonymous
    June 17, 2007
    The comment has been removed

  • Anonymous
    June 20, 2007
    Welcome to the XXVIII Community Convergence. In these posts I try to wrap up events that have occurred

  • Anonymous
    April 22, 2009
    Any class with the word "utilities" seems to be designed to hold a variety of unrelated functions that programmers couldn't find a better place for.