Archive for the ‘C++’ Category

In Defense of Hungarian Notation (with caveats)

Saturday, September 24th, 2005

Anyone who has programmed directly to the Windows API knows about the existence of Hungarian Notation. It is a way of making the name of a variable or procedure flow automatically from its data type. Like other conventions that have been rejected by the general programming community, it would be foolish to use it today on any public API or code example. Despite this, I do still borrow from some of the “spirit” of the notation when I code.

I’d like to explain why.

Some critics (and adherents) of Hungarian Notation think its goal is to encode useful type information into names. The truth is, coming up with useful names is not the point. It is much more about avoiding the encoding of useless information!

Names—like indentation, spacing, and comments—do not affect the executable code. For instance, look at this code:

void DestroyTheWindow(HWND TheWindowIWillDestroy)
   // This function should only be called on windows which
   // have no parents; if you would like to destroy a
   // window which has a parent, then you must destroy 
   // the window through the parent. Failure to do so
   // will skip essential window layer cleanup routines
   // and a later crash may ensue.
   {
   ...
   }

No matter how nice the prose, that programmer spent their finite allocation of time on earth unwisely. Given the 30 seconds (at least!) it took them to write that comment, I’d rather they had written:

void DestroyWindow(HWND hwnd)
   {
   assert(GetParent(hwnd) == NULL);
   ...
   }

…or even better, invested that time in creating subclasses of HWND (like HWND_TOPLEVEL and HWND_CHILD) so that the error could be caught at compile-time:

void DestroyToplevel(HWND_TOPLEVEL hwnd)
   {
   ...
   }

Even if HWND_TOPLEVEL and HWND_CHILD are merely typedefs for HWND, I think this is better documentation than a comment in the long run. It conveys all the same information and can easily grow into a compiler-checked solution, if you were to upgrade the definitions from typedefs into distinct classes.

Before anyone revokes my programming license, I’m not saying people shouldn’t comment. Yet a programmer only has twenty-four hours in a day (disregarding sleep, of course)… and therefore any time invested in comments is energy that was not put into fixing the code so that comment wasn’t necessary. Naming is the same way—a creative name is something a user will not be able to appreciate in terms of runtime features.

So to bring our discussion back to the core of Hungarian Notation’s value for your programming mind, let’s look at the situation of someone naming a function parameter:

void DestroyWindow(HWND /*[name goes here]*/)
   {
   ...
   }

I could sit around all day debating whether to call it Input or ToBeDestroyedWindow or TheWindowIWillDestroy or cryptically x. Yet what this variable represents is obvious from the context—after all, it is the sole parameter to a routine called DestroyWindow. It’s the window to destroy (Duh!)

One way of avoiding a meaningless name would be to give the variable some unique number:

void DestroyWindow(HWND noname1231 /* hope this is unique! */)
   {
   ...
   }

In the future, our integrated development environments might be able to manage such numeric identities for declarations “behind the scenes”. This would be a lot like how databases invisibly manage table relationships through primary keys. But so long as we’re directly modifying textual code, humans can’t really match up these numbers while reading. So this is a bad idea.

But what if you made all your type names in capital letters? Then you could turn the type into lowercase letters to produce an available symbol:

void DestroyWindow(HWND hwnd)
   {
   ...
   }

Assuming that turning your type into lowercase doesn’t produce a language keyword, then you’ll always get a legal identifier. Moreover, the name isn’t completely useless: anywhere you see a reference to this “nameless” variable you know at least one thing about it: its type.

Naturally, this method of producing a unique symbol breaks down once you have more than one variable of a specific type in a scope:

HWND hwnd; // topmost window in the Z order
HWND hwnd; // parent window (**ERROR, NAME ALREADY USED**)

Yet if there’s more than one variable of the same type in a scope, that means that context alone is by definition insufficient to explain your variable’s purpose. Hungarian Notation prescribes adding a disambiguating mixed-case phrase to the end of the name. It is especially efficient, because the disambiguation always tacks onto the end of names—which is easy to add and remove in the editor if you ever run up against a collision:

HWND hwndTopmostInZOrder;
HWND hwndParent;

This only works if you create lots of new types. If you have an integer value, and you prefix a variable with the letter “i”, context is probably not sufficient to know what the variable is for. So that begs the question: why are you using an integer and not a higher level abstraction like “line number” (LINE), “count of employees” (CEMP), or a “stack depth counter” (SDC)?

If the Y2K problem taught us anything, it’s that you should be very liberal in creating new types which capture your ideas—even if it’s just a measly preprocessor macro. Compare:

BYTE todays_date_in_MM_DD_YY[3];

with:

#define DATE BYTE[3]
DATE date;

Even though the implementations are isomorphic, the second pattern is far better. There is no automatic way to find all the dates in the first example…you have to do manual inspection of all the names of BYTE arrays to figure out which are dates and which are not. This is why I don’t hesitate to create new types while I program, even wrappers for basic types like int or long. Once you’ve done that, you can feel good about variable declarations like LINE lineFirst; or CEMP cempHiredLastMonth; or just SDC sdc;.

If you aren’t making tons of types, and just sticking “i” or “l” in front of everything, forget about Hungarian. It will be useless, and makes what Linus Torvalds says absolutely true:

“Encoding the type of a function into the name (so-called Hungarian notation) is brain damaged - the compiler knows the types anyway and can check those, and it only confuses the programmer.”

I’m not surprised Hungarian Notation has a terrible reputation, because I’ve never seen published code that used it right. Microsoft even screwed it up in their most public APIs! Just look at the definition for window procedures:

long FAR PASCAL WndProc(
   HWND hwnd,
   UINT msg,
   UINT wParam,
   LONG lParam);

The only part they did right is the HWND. The rest is a complete mess. A more genuine attempt would probably look like:

typedef UINT WPARAM;
typedef LONG LPARAM;
typedef LONG LRESULT;
typedef UINT WM; // (W)indows (M)essage
 
LRESULT FAR PASCAL LresultWndproc(
   HWND hwnd,
   WM wm,
   WPARAM wparam,
   LPARAM lparam);

Some very reasonable people suggest that since so few programmers have grasped the “true” spirit of Hungarian Notation, something must be inherently confusing about it. Therefore nearly everyone should avoid it.

I mostly agree.

Yet I simply can’t stand being forced to give a meaningless name to something which is obvious from its context. That’s why I came up with a compromise. I simply make a mental association of a shorthand for each data type that I’m using (such as “WND” for System.Window) without changing the definition. This means code starts to look like:

void DestroyWindow(System.Window wnd)
   {
   System.Window wndParent = wnd.GetParent();
   System.Window wndTemp;
   foreach (wndTemp in wndParent.Children())
      {
      if (wndTemp == wnd)
         ...
      }
   }

This gives me what I desire, and avoids the taboo of data types that are in all capital letters. (I don’t know why, but people have reserved all caps for naming constants. It’s a convention that never made sense to me—mixed case constants are more readable. Oh well.)

One quirk I have adopted is to use “is” as the prefix for booleans. I still think it’s in the spirit of Hungarian, and boolean isTheUserOnline reads a bit better than boolean boolUserOnline. Here’s another example demonstrating my naming technique:

void KickUserOffline(UserObject userKick, UserObject userAdministrator)
   {
   assert(userAdministrator.HasPrivilege(PRIVILEGE_KICK));
   if (userKick.isTheUserOnline)
      {
      userKick.Message("You've been kicked off.");
      userKick.Logout();
      }
   }

I hope I’ve presented this clearly. The code examples that appear on HostileFork.com will use this technique where possible, and I welcome feedback if someone has a better way.

Cleaner API Design Using Ignorable “Hints”

Friday, August 12th, 2005

Sometimes API authors expose additional entry points to their code which exist only because of performance. For instance, look at why MakeManyWidgets() exists in the sample below:

class WidgetFactory
   {
private:
   // a really, really slow routine
   void StopProcessesSoItsSafeToMakeWidgets();
 
   // another really, really slow routine
   void RestartProcesses();
 
public:
   Widget* MakeAWidget()
      {
      StopProcessesSoItsSafeToMakeWidgets();
      Widget* w = new Widget();
      RestartProcesses();
      return w;
      }
 
   std::vector<Widget*> MakeManyWidgets(int HowMany)
      {
      std::vector<Widget*> ret;
      StopProcessesSoItsSafeToMakeWidgets();
      for (int temp = 0; temp < HowMany; temp++)
          ret.push_back(new Widget());
      RestartProcesses();
      return ret;
      }
 
   void DoSomethingElse()
      {
      assert(ProcessesRunning());
      return;
      }
   };

The extra routine was added as a mere performance convenience, since making N widgets has identical semantics to simply N successive calls to MakeAWidget(). This is presumably safer than publishing the private routines for starting and stopping processes so it’s safe to make widgets—not only might that be an implementation detail we don’t want to expose, it could be deemed too serious a problem if the client improperly matches up stop/start calls.

I’ve seen this pattern countless times, and never liked it. Routines like MakeManyWidgets() lead the clients of your API to start disrupting the control flow in their programs to try and get a performance payoff that may turn out to be irrelevant in the future. It also gives the misleading impression that there might be a semantic significance to creating a set of widgets as a batch, and will make source code written to the API a lot harder to absorb.

If I face a situation like this, I completely decouple the performance “hint” from the routines that do the work. As a rule, I also make sure that if the hint gives blatantly incorrect information, the worst that can happen is that your program is a bit slower than it would have been without the hint. To give an example of how this might work, look at HINT_MakingManyWidgets() below:

class WidgetFactory
   {
private:
   mutable int widgets_hint;
 
private:
   // a really, really slow routine
   void StopProcessesSoItsSafeToMakeWidgets();
 
   // another really, really slow routine
   void RestartProcesses();
 
public:
   void HINT_MakingManyWidgets(int HowMany) const
      {
      widgets_hint += HowMany;
      }
 
   Widget* MakeAWidget()
      {
      if (ProcessesRunning())
         StopProcessesSoItsSafeToMakeWidgets();
      Widget* w = new Widget();
      if (widgets_hint > 0)
         {
         widgets_hint--;
         RestartProcesses();
         }
      return w;
      }
 
   void DoSomethingElse()
      {
      if (!ProcessesRunning())
         {
         widgets_hint = 0;
         RestartProcesses();
         }
      }
   };

This way, developers aren’t encouraged to complicate their code up front. No one will use these HINT_ functions unless they need to—only those clients who are dissatisfied with performance will bother. You can add as many as you like, adapted specifically to suit the real use cases of important clients. And if you ever want to stop supporting a hint you merely make the function have no effect.

The worst you’ll do to your clients is slow them down, and your API and code using it will be purer and more elegant!

Assertions parameterized by location

Sunday, May 1st, 2005

Many assert macros label the line where the assertion occurred. If the assertion is fired during a program run, this causes an alert to be raised mentioning the exact source line of the assert statement. Usually, this is exactly what should happen, but sometimes a subroutine would like to establish a contract with its caller that if a certain condition is not met, it is the caller that should be identified in the assertion!

For example, imagine if you are writing a routine whose job it is to hang up the phone, with the precondition that it is currently not already hung up:

void Phone::HangUp()
   {
   assert(this->status == Phone::statusConnected);
   ...
   }

There may be many places where this method is called. Yet no particular call site will be indicated in the assertion message. A manual inspection of the call stack with the debugger is the only way to uncover the guilty caller who is hanging up an already-hung-up phone. Using the default assertion libraries, there is no simple way to log the actual origin of the problem.

As a mechanism for addressing this problem, I use my own Assert() routine and an abstraction for encapsulating the idea of a source location in a file. I call it SLOC (for Source LOCation), and it may be freely passed as a parameter to any routine you like. For example, if you wanted Phone::HangUp()’s assertion to be tied to a caller’s location, you’d write:

void Phone::HangUp(SLOC sloc)
   {
   Assert(sloc, this->status == Phone::statusConnected);
   ...
   }

Then in the caller, you can write:

phone.HangUp(SLOC (__FILE__, __LINE__));

Because I like brevity, I use the abbreviated macro “_d_”:

phone.HangUp(_d_); // _d_ means source location "D"efinition

This way, when the assertion is reported by the program, it will make reference to the passed-in location, rather than the location of the Assert() in the code that’s reporting the error. Source locations can be applied in many debugging scenarios, for instance if we wanted to track the previous disconnection call we could do so:

void Phone::HangUp(SLOC sloc)
   {
   // this->slocLastHangupCaller contains the last
   // caller's location.  assert message could include it,
   // or just inspect from debugger
   Assert(sloc, this->status == Phone::statusConnected);
   ...
   this->slocLastHangupCaller = sloc;
   }

By checking slocLastHangupCaller, you will be able to find the source of the previous (unexpected) hang-up!

You might theorize that run-time access to the stack is the ultimate API for dealing with this sort of thing. Imagine if PHONE::HangUp() could somehow obtain an object representing the call stack, and then extract whatever information it wanted about the callers. That’s a little heavy-handed, and I believe that the odds of the API being abused are so high that a “narrower” protocol agreement between callers and subroutines would need to be established for the common scenarios.

At the end of the article, I’ve given sample code for those interested. This includes an additional template class called TR<[type]> (for TRacker), which lets you audit the places in your code where certain assignments are made. If the value of a variable isn’t what you expect, it automatically identifies the location of the previous assignment! So for instance:

class Phone 
  { 
private:
  TR<bool> connected;
public:
  Phone::Phone () :
       connected (_d_, false, "connected")
    {
    // set up phone
    ...
    }
  void Phone::Call(SLOC sloc, PhoneNumber number)
    {
    // assert that we are not already connected
    // (automatically reports location of last assignment if we are)
    connected.AssertValue(sloc, false, "false");
 
    // call the number
    ...
 
    // set to true and record that we are doing so
    // on behalf of the caller
    connected.Set(sloc, true);
    }
  void Phone::HangUp(SLOC sloc)
    {
    // automatically reports location of last assignment to false
    // *if* connected is not true.
    connected.AssertValue(sloc, true, "true");
 
    // hang up the phone
    ...
 
    // records the caller as the location of false assigment
    connected.Set(sloc, false);
    }
  };

It should be apparent why why this is useful. These mechanisms are extremely helpful in finding bugs in systems, and are very addictive once you start using them. To further simplify matters, I’ve also created a stock type for TR as TR_bool, to reduce typing. Bear in mind that if you want the default behavior of an assertion reporting its own line, it’s as easy as:

Assert(_d_, "report the error here.");

Here’s the small amount of code, which you can adapt to your needs. Please report any problems:

  1. location_parameterized_assert.cpp : view CPP

Object Lifetime as Protocol in OOP

Thursday, April 21st, 2005

Object-oriented programming is a powerful paradigm for improving software engineering. However, it is important to realize why OOP is powerful.

Some people think the power comes from inheritance, and this means that instead of having to write new code you can just “inherit” behaviors from code that has already been written. This is not patently false, but if your sole goal is to write fewer bytes of code you can attack that lots of ways without OOP. You can find repeated program structures and put them in functions, or just use shorter variable names. :)

What real OOP is about is creating entities which saliently capture specific programmer concerns (objects). Thanks to the built-in creation management of objects through constructors and destructors, you can make the lifetime of the object map to the lifetime of the concern. It’s more about “everthing in its right place” and sane APIs than it is about reducing lines of code.

So given that, what’s wrong with this picture?

{
Employee worker;
worker.DoSomeStuffAndMaybeInitializeEmployee();
if (worker.IsValid())
   {
   // since worker is valid, it's ok to call GetName()
   printf("The employee is %s\n", worker.GetName());
   worker.DoOtherStuffMaybeUninitializeEmployee();
   }
else
   {
   // calling GetName() will fail, so do something else
   worker.DoOtherStuffDefinitelyInitializeEmployee();
   }
 
// before destructor runs, worker must be uninitialized
if (worker.IsValid())
   worker.Uninitialize();
 
// should be safe to run employee destructor now!
}

In short, this is not real C++. The abstract concept of an employee is getting tangled up with the mechanics of initializing an employee, after the constructor has already run. The programmer is worried about cleaning up the object before the destructor runs, which is terrible since destructors should be safe to run at any moment. Especially if you believe in exception handling!

There are grievous examples of this in many class libraries. For example, in the Microsoft Foundation Classes (MFC) the CView class tries to inherit much of its functionality from CWindow. (A view is-a window, and hence it “inherits” much of its functionality). Yet there are copious comments in the source warning you against putting “too much code” in the constructor for a class inheriting from view. Instead, you’re supposed to put it in the OnInitialUpdate() or OnInitialize() method.

Why do these Initialize methods exist? When the view constructor runs, the window it needs should have been already created, so that relevant initialization code can be put in the constructor–however much it takes. It doesn’t do this because MFC is too caught up in the mechanics of inheritance and dismissing the importance of separation of concerns. A view constructor should be the place for initialization code, not an obligatory “post-constructor” function.

Another generally troubling aspect about the example above is that there are “modes” under which certain methods are not safe to call at run-time. This indicates the design is not protecting the programmer from a concern—just giving them a headache! (However, see my article on usage of const to see how modes on objects at compile time can actually be quite helpful.)

In short: don’t lose focus on the idea that objects in a class library are there to reduce the concerns of the programmer. That reduces client lines of code and bugs. If you instead think of how to use C++ to reduce the concerns of the class library author by saving code through inheritance, you’re probably missing the point!

Pseudo-functional programming tricks in C++

Tuesday, March 15th, 2005

Wouldn’t you be upset if you discovered that sqrt(9) was 3 on weekends and 3.7 every other day of the week? I certainly would.

Yet in languages like C++, there is nothing fundamental about the language which keeps a misguided library author from writing things like:

float sqrt(float x)
   {
   if (x == 9)
      if (today() == Saturday || today() == Sunday))
         return 3;
      else
         return 3.7;
   ...
   }

Of course, you’d hope that the authors of the standard math libraries wouldn’t do something so blatantly crazy. But you can’t really protect a module from a dependency you don’t want to arise in C++. So any function in any library can—theoretically—examine any information it wants. That information might live in a global variable, somewhere in the file system, in the video buffer, or even on the Internet.

To address this issue, computer science academics often advocate ditching traditional OOP entirely and moving to functional programming. This lets you contractually define operations which are guaranteed to depend solely on the parameters you give to them. This is great, but it requires a certain “stylized” way of thinking…and it doesn’t trust programmers to “do the right thing”. So functional programming often makes seemingly-simple procedures a hassle to write.

Yet it’s possible to abuse C++ a little, in order to be relatively sure certain methods aren’t depending on certain things. For instance, let’s imagine that you have a Region object type which is going to receive some number of Enter() and Exit() calls, followed by a call to RunAction():

class Region
   {
   ...
   virtual void Enter() = 0;
   virtual void Exit() = 0;
 
   // supply action code here, by overriding "RunAction"
   // please do not make behavior depend on how
   // many times Exit or Enter was called, only
   // depend on what the last call was!
   virtual void RunAction() = 0;
   ...
   };

Notice that you would very much like RunAction()’s effects to NOT depend on how many Enter()s or Exit()s happened. Yet the comments do little to enforce this: if any of Region’s members are modified by the Enter() or Exit() calls, that might introduce a dependency! If you want to discourage the RunAction() method from depending on those modifications to Region, you can introduce a cooperating class:

class Action
   {
   ...
   virtual void Run(bool inside) = 0;
   ...
   };
 
class Region
   {
   ...
   virtual void Enter() = 0;
   virtual void Exit() = 0;
   virtual std::auto_ptr<Action> AllocateAction() = 0 const;
   ...
   };

I’ve used a smart pointer here (std::auto_ptr), to indicate a transfer of ownership. The Action that Region::GetAction returns is owned by the caller and can be freed by it. That’s important!

Before any Enter() or Exit() calls are made, the caller asks the Region object to construct an Action object which predetermines the final action. The Action object does not have access to the state which may be accumulated by the Region object, and vice-versa. Yet Region still drives the logic—it was able to give us a class which dictates what will happen when a Action::Run() is finally invoked…but without being able to make that execution conditional on the state it might gather.

On the surface this looks good, but it can be undermined (unintentionally or otherwise) in several ways. For instance, the Region object could poke a this pointer into the Action it returns, thus giving the Action access to its state to the later call to Action::Run(). However, as long as we require (by convention) that Region::AllocateAction() produce an isomorphic object on each invocation, the caller can:

  • Construct an object region1 of type Region
  • Call region1::AllocateAction() to produce an object action of type Action
  • Construct a new object region2 of type Region (same constructor as step 1)
  • make the Enter() and Exit() calls on region2
  • optional: make a few spurious Enter() and Exit() calls on region1
  • Destroy region1
  • Call action::Run(inside) where inside indicates whether we are inside region2

This obviously hampers the ability of Region developers to build a coupling between the Region object and the Action object it hands you. It’s not exactly functional programming, and of course a programmer could subvert any hard-coded strategy you might come up with (it’s even better if you do something more random). Yet it should catch most major violations of the model, and alert a programmer who is simply unaware of the dependency that they were doing something wrong.


Creative Commons Attribution-NonCommercial-ShareAlike 3.0 Unported
Creative Commons Attribution-NonCommercial-ShareAlike 3.0 Unported