Are the constructors defined and initialized correctly in C++ code?

Question

I saw this code on the Internet and I didn't understand it. Could anyone help me to translate it? Are the constructors defined and initialized correctly?

struct account {

string login;
string surname;
string name;
string passwd;
bool connecte;

/** accessInt :
int accessInt;

/** acl  :   acl */
acl* accessListe; 

//constructers
account() 
{
}

account(
    const string & login, const string & surname,
    const string & name, const string & passwd, const bool connecte , const int & accessListe
    ) : login(login), surname(surname), name(name), passwd(passwd), connecte(connecte), accessInt(accessListe){
    accessListe = new acl(accessInt);
}

account(
    const char * login, const char * surname,
    const char * name, const char * passwd, const bool connecte , const int & accessListe
    ) : login(login), surname(surname), name(name), passwd(passwd), connecte(connecte), accessInt(accessListe){
    accessListe = new acl(accessInt);
}

friend std::ostream &operator<<(std::ostream &c, const account& ac) {
    c << "a:" << ac.login << ":" << ac.surname << ":" << ac.name << ":" << ac.passwd << ":" << ac.accessListe->toInt();
    return c;
}
};

Besides that the code, as shown, will give you compiler errors it's correct. :) — Some programmer dude, May 15 '15 at 21:32
There are no `name`, `surname`, or `accessInt` members, so this shouldn't even compile. — WhozCraig, May 15 '15 at 21:34

score 0 · Answer 1 · answered May 16 '15 at 01:07

The code has an odd embedded comment:

/** accessInt :
int accessInt;

/** acl  :   acl */
acl* accessListe;

Since there is code that attempts to initialize accessInt, I assume the line above its declaration is just missing token to close the comment.

The code lacks a definition of acl.

The bodies of the latter two constructors attempt to initialize accessListe, but there is also a constructor parameter named accessListe which will shadow the object member. Either rename the parameter, or use this->accessListe in the constructor body.

score 0 · Answer 2 · answered May 16 '15 at 01:28

jxh's answers several if not most things wrong with the code, but I'd like to extend that and talk about some others. The default constructor in particular is bad as it makes no attempt to construct any object with values. This is fine for all your std::string which are guaranteed to be constructed as an empty string; however, for your int, bool and pointer members they will all be constructed with any values. This means if the default constructor this class is called, these values will not be initialised to anything and can cause problems! Let's assume account destructor cleans up its allocated memory:

account::~account() { delete accessListe; }

If the class is constructed with its default constructor, undefined behaviour will ensue when it gets cleaned up (by default, accessListe will just point to any bit of memory). You should ensure your default constructor (and all constructors) initialise all values with normal values:

account::account() : connecte(false), accessInt(0), accessListe(nullptr /* or 0 if you don't have C++11*/) {}

Regarding the latter two constructors, both could (read: should) initialise the pointer accessListe in the constructor initialisation list, rather than assigning a value afterwards:

account::account(arguments...) : accessInt(someValue), accessListe(new acl(accessInt))

This will save an unnecessary copy.

Are the constructors defined and initialized correctly in C++ code?

2 Answers2