C++ Constructor coding errors

Question

I just stumbled across this bug in some legacy code:

class MyAPIHandler
{
    private:
     int handle;
    public:
    void MyApiHandler()  // default constructor
    {
     handle = 42;
    };
};

It compiles fine, with no warnings - but the behaviour wasn't what I intended, because the constructor name is misspelt. This by itself would have produced a warning about "function does not return a value", but I guess I was on autopilot and added a "void" return type to 'fix' this.

Now, the bugfix was easy, but my question is this:-

What techniques could I use to prevent this type of bug recurring?

Some languages require an explicit "constructor" keyword, which should make this problem obvious. Unit testing, obviously should also have caught it. What else can I do?

Answer 1

Code review? Unit test, as you mention is good too. Code coverage. Many of the usual tools for finding bugs could find this.

Answer 2

Maybe you could do a

#define CONSTRUCTOR

And then in your code

class MyAPIHandler
{
   public:
       CONSTRUCTOR MyAPIHandler()
       {
           // Deep magic
       }
};

Now, by itself this will not do anything, but if you get used to writing it like this you should be able to spot the mistake more easily.

Although I honestly think that this is such a rare occurence that it isn't worth the trouble.

Answer 3

I'm pretty sure that PC Lint, a static code analysis tool, would have spotted this error. It isn't free but it is very, very good. Worth a look:

http://www.gimpel.com/

Answer 4

I guess following TDD will help, though not the solution you were looking for ...

as you said its legacy code, i suggest you read "working with legacy code- Kent Beck". That might help.

regression testing will pick issues due to bug fixes.

even i am looking forward for out of box suggestions :)

Answer 5

If you always use initialiser lists in your constructors:

MyApiHandler()  // default constructor
: handle(42)
{
}

the misnamed constructor bug would be even more unlikely, and it's better style anyway.

Edit: thanks to commenter for the link

Answer 6

Unit tests.

MyAPIHandler mah;
BOOST_CHECK_EQUAL(mah.handle, 42);

Answer 7

This shouldn't be much of a problem. Constructors don't have return values (not even void), so the compiler should be able to tell whether something is meant to be a constructor or a member function, and let you know if there's a problem. Obviously someone saw the compiler's error on that and chose the wrong solution (adding a return type instead of correcting the spelling). (EDIT: Sorry, didn't realize that you'd done that yourself.)

Personally, I always put the constructor(s) near the beginning of the the class's public section (before any member functions, but after public type definitions and constants), so a problem like that would be pretty obvious to me on re-reading the code. But conventions differ on that.

Answer 8

I believe you should not think too much about this issue

It happens for constructors, but also for any other method, or even function. For example:

class MyBase
{
   // etc.
   virtual void doSomething() ;
} ;

class MyDerived : public MyBase
{
   // etc.
   virtual void DoSomething() ;
} ;

Or:

bool isOk(int value) { /* etc. */ }
bool isOK(double value) { /* etc. */ }

void doSomething(int value)
{
   if(isOK(value)) // isOK(double) will be called
   {
      // Etc.
   }
}

And this is not only a problem of character case. This kind of error happens. IDE helpers like autocompletion can help somewhat, as could a good unit-testing code (something covering all methods of a class), but I don't believe the compiler alone could protect against this kind of developer mistyping even with additionnal keywords.

What about CONSTRUCTOR?

As for the CONSTRUCTOR define mentionned before me, this is a bad idea IMHO:

It will help as much as a comment (and so, why not use /* CONSTRUCTOR */ instead?), and if someone thought about using the word CONSTRUCTOR as a define symbol, then you can bet someone else will have the same idea in some header you include but don't own, and that it will break your code...

Answer 9

I have a set of broilerplates/templates (in Vim.. but I guess you can have them in any modern editor) which avoids typos of this nature.

Answer 10

Most IDEs allow for macros of some sort. Just use a macro to create a class definition. That's the way I have it setup. My macro prints outs:

class CHANGETHIS
{
    public: 
      CHANGETHIS();
      ~CHANGETHIS();
}
#error "Finish this class definition"

The #error line is just a safety net to make sure it doesn't build if I am interrupted in the middle of my work and forget about it when I get back.

What I like about this is that its portable across any editor, so I don't feel handicapped when I switch to a different IDE. Hope this helps.

In addition, use initialization lists. They're faster and lead to less bugs.

Answer 11

I'd say the simplest way of avoiding this problem is to lay down the law for naming in the form of a coding style guide. In this you set down how to name your entities and you stick to it.

Specifically at work we've got it mandatory that abbreviations are only capitalised on the first letter, therefore this would be caught more easily.

Answer 12

If the member variable is not going to change, declare it const and the compiler will force you to use an initialiser list, which in this case would force you to detect your error (because it won't compile).