Cons of putting huge chunks of code in the constructor - C++

resetreset · 01-04-2011, 01:44 AM

Not related to the thread- I just wanted to ask Anisha - you write device drivers for Linux right? What's all this about XML?

Have you switched paths?
Happy new year!

Aquarius_Girl · 01-05-2011, 05:13 AM

Quote:

Originally Posted by dugan

The pattern in C++ is for the constructor to acquire any resources necessary (including file handles), and for the destructor to release them....More information is here:http://en.wikipedia.org/wiki/RAII

Dugan, thanks to you for telling me about RAII, I hadn't come across it before! but I couldn't understand the crux from the wiki link so I found this to be more helpful:
http://stackoverflow.com/questions/3...-pointers-in-c

Quote:

Originally Posted by dugan

It follows that the actual parsing should be done by a third method...
BTW, if your XmlFileReader class is also writing data, then you should either rename it, or move the parts that write to another class.

RAII is AFA I have understood all about acquiring and releasing the resources in a "proper manner", but why do you say that the parsing code should be somewhere else? How's that related to RAII?

and thanks for the design you presented in post 13, I already knew about virtual functions, but our design doesn't need that currently.

Aquarius_Girl · 01-05-2011, 05:39 AM

Quote:

Originally Posted by paulsm4

Again, I think the best criteria for "what goes into a constructor" is to ask yourself "what is the invariant of your class"? What is the BARE MINIMUM needed to make your class useful? If your class contains "information" (information that happened to come from an XML file), then your constructor should arguably "get the information". It should NOT leave the file open if it doesn't need to. Nor should any USER of your class need to know - or care - if the information happened to come from XML, from a database or from a web service. Your class just gives the user "information". And should be ready to give that information the moment after it's constructed

Thanks for posting, Paul,
I also thought the same thing, in my case the only main task the class does is to parse the XML file and put the data in the vectors. There is no need for any other function as the input is supposed to be the file name, nothing else, and we often make different functions to support code resuability (spelling is wrong I think), but my code because it parses only two PARTICULAR files couldn't be reusable in any other way!!

The only two valid reasons for NOT putting the code in the constructor, I have understood till now are:

The case in which we have to create multiple objects of the class and initial code for all the objects is NOT bound to be the same.
The case in which the static methods can do the job for you (we might not need the constructor itself in that case)

Is there any other reason for not putting the code in the constructor? ()

Thanks for the help on copy constructors. and all I could understand from the class invariant is "the main thing which your class does"! (Is that right?)

orgcandman · 01-05-2011, 07:31 AM

A lot of what you put in your constructor depends on what you're doing with the class (as paulsm4 notes). One very important fact of life, for those of us who can't use exceptions in our C++, is that constructors can't fail. They can't return status codes. They can't indicate errors in an intuitive manner.

Personally, I'd split the XML interface you have into two classes - one which is a generic interface for reading data, and the other which is a specific interface for parsing into xml. Pass a specific instance of the data reading interface to the xml class on construction (ex: file reading[std::], socket reading, basically you can use the std::stream class). XML class can then set itself up using an abstract interface that is more clear to the developer, and more flexible for you.

dugan · 01-05-2011, 08:19 AM

Quote:

Originally Posted by Anisha Kaul

The only two valid reasons for NOT putting the code in the constructor, I have understood till now are:

The case in which we have to create multiple objects of the class and initial code for all the objects is NOT bound to be the same.
The case in which the static methods can do the job for you (we might not need the constructor itself in that case)

Is there any other reason for not putting the code in the constructor?

I've already mentioned another reason: the case where you, or the testers in your QA department, want to test the XML parsing code and the rest of the class independently. You want to do this if you're unit testing. And you should be unit testing. This is what happens when you're on a team and you don't unit test.

Quote:

RAII is AFA I have understood all about acquiring and releasing the resources in a "proper manner", but why do you say that the parsing code should be somewhere else? How's that related to RAII?

It's not. It's related to the principles of good software architecture. I predicted that if your class is both holding persistent data and reading XML, it's probably doing too much and not specialized enough. Paulsm4 disagrees; he's said that my opinion on this is "absolute nonsense." Remember that neither of us have actually seen your class design, so you'll have to weigh the considerations we brought up and decide for yourself.

BTW, the recommendation in orgcandman's post is very good one. It has the same goal as my post in #13: decoupling the Xml parser from the program state (not using virtual function for their own sake). Only, his goes a step further.

Aquarius_Girl · 01-12-2011, 12:54 AM

Quote:

Originally Posted by orgcandman

One very important fact of life, for those of us who can't use exceptions in our C++, is that constructors can't fail. They can't return status codes.

Thanks for the useful reply,

void functions do not have a return value as well, if we are not using exceptions in any of our code then,
in the below code is situation not same for the constructor as well as the void function? and secondly you have said that "constructors can't fail", what did you mean by that?

Code:

// constructor
constructor :: constructor (int x) {
   if (x != 0) {
       // do something
   }
   else {
      // failure
   }
}

// normal function of the class
void constructor :: function (int x) {
   if (x != 0) {
       // do something
   }
   else {
      // failure
   }
}

Aquarius_Girl · 01-12-2011, 01:26 AM

Quote:

Originally Posted by dugan

I've already mentioned another reason: the case where you, or the testers in your QA department, want to test the XML parsing code and the rest of the class independently. You want to do this if you're unit testing. And you should be unit testing. This is what happens when you're on a team and you don't unit test.

Thank you for following up, Dugan, Of course I know the unit testing is extremely important when we are working in a team and otherwise also. There was no need to cross link that thread here and this thread there! I always do follow up my threads (though not the same day) and was going to address that issue without your prompting.

The second issue in that thread is solved now, the problem was not with the repository but with a variable which hadn't been initialized, so the value of that variable was being set as desired by the compiler, that's why sometimes it was working and other times not. The second issue(code) in that thread was not written by me, that's why I didn't do the unit testing.

Thanks again for useful inputs.

orgcandman · 01-12-2011, 01:08 PM

Quote:

Originally Posted by Anisha Kaul

Thanks for the useful reply,

void functions do not have a return value as well, if we are not using exceptions in any of our code then,
in the below code is situation not same for the constructor as well as the void function? and secondly you have said that "constructors can't fail", what did you mean by that?

Code:

// constructor
constructor :: constructor (int x) {
   if (x != 0) {
       // do something
   }
   else {
      // failure
   }
}

// normal function of the class
void constructor :: function (int x) {
   if (x != 0) {
       // do something
   }
   else {
      // failure
   }
}

How do you express failure in a constructor? It's meant to put the object into a "known good" state. As such, if there's a failure while putting the object into it's "known good" state, how do you tell the callers that the object needs to be deleted and a possible error propagated up the line? Typically, it's done with exceptions, but some of us aren't allowed the luxury.