CODEWITHSUNDEEP
c++constructor
Roddy
Roddy

Reputation: 68033

C++ Constructor coding errors

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?

Upvotes: 2

Views: 818

Answers (13)

dcw
dcw

Reputation: 3555

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).

Upvotes: 1

Dominik Grabiec
Dominik Grabiec

Reputation: 10655

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.

Upvotes: 0

Head Geek
Head Geek

Reputation: 39858

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.

Upvotes: 2

Carl
Carl

Reputation: 44448

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.

Upvotes: 0

paercebal
paercebal

Reputation: 83309

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...

Upvotes: 1

Sridhar Iyer
Sridhar Iyer

Reputation: 2840

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.

Upvotes: 0

Andreas Magnusson
Andreas Magnusson

Reputation: 7434

Unit tests.

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

Upvotes: 0

James Hopkin
James Hopkin

Reputation: 13973

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

Upvotes: 19

Rob
Rob

Reputation: 78638

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/

Upvotes: 1

Joris Timmermans
Joris Timmermans

Reputation: 10978

Besides being careful or having code reviews, there's not a lot you can do.

You can make a checklist for writing new classes with topics like:

  • Copy-paste the class name to be used as the first constructor.
  • If you implement the copy constructor, implement the destructor and the copy assignment operator (the Rule of Three.
  • Take care with single-argument constructors or constructors with default arguments - consider making them explicit

These are constructor-related points, the whole checklist could be longer. After a while you'll start doing things like this automatically.

Personally, I think Unit Testing is the best answer to avoid your specific problem, as you already mentioned.

EDIT: Added idea from comments:
In some development environments you could use code templates or macros to generate a correct class skeleton for you. This is a real "programmer's solution" - automating everything that can be automated to avoid errors.

Upvotes: 2

FL4SOF
FL4SOF

Reputation: 4281

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 :)

Upvotes: 0

Vilx-
Vilx-

Reputation: 106912

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.

Upvotes: 0

Brian
Brian

Reputation: 118865

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

Upvotes: 2

Related Questions