Issue with C++ constructor

Question

EDIT: This question came up and I think I aced it! Go StackOverflow!! :D

I have exams coming up, and one of the questions on last year's exams was to spot the problem with implementation of the following constructor and to write a corrected one.

Rectangle::Rectangle(string col, int len, int br)
{
    setColour(col);
    length =len;
    breadth=br;
}

The class definitions are as follows:

class Polygon
{
public:
    Polygon(string col="red");
    void printDetails(); // prints colour only
    virtual double getArea()=0;
    void setColour(string col);
private:
    string colour;
};


class Rectangle : public Polygon
{
public:
    Rectangle(string, int, int);
    void printDetails(); // prints colour and area
    // for part 3, delete the line below
    double getArea();
private:
    int length;
    int breadth;
};

I've written the code into the compiler and it runs fine. I'm guessing the question is relating to inheritance, since string colour; is private, but setColour is public so I cant figure it out. Unless its Rectangle::Rectangle(string col, int len, int br):length(len), breadth(br) and then set the colour inside the construcor or something.

Its only worth 3 marks so its not that big a deal if nobody wants to answer, but I figure if I'm going to have a career as a programmer, its in my interest to know as much as possible. ;)

Thanks for any help.

PS, see getArea() in Rectangle. When I remove that it tells me it "cannot instantiate the abstract class". What does that mean?

EDIT: Here's the main:

void main (void)
{
    Rectangle rect1 ("blue",5,6);
    Rectangle *prect2 = new Rectangle("red",5,6);
    rect1.setColour("red");
    rect1.printDetails();
    prect2->printDetails();
}

Answer 1

I don't see anything wrong, though you could make it more efficient by using an initialization list (otherwise your private members of both classes get initialized twice):

Rectangle::Rectangle(string col, int len, int br) 
: Polygon(col), length(len), breadth(br)
{

}

Notice that the initialization list can call the constructor of Polygon as well.

See Why should I prefer to use member initialization list? for a complete description of the advantages of using initialization lists.

Answer 2

If it's about best C++ practices, then:

1. Pass string parameters by const reference;
2. Use initializer list and initialize colour by passing it to parent constructor, not setColour.

Answer 3

The only thing I see off the bat is there is two printDetails() but the base class one is not virtual so you would not get the polymorphic behavior expected.

Answer 4

The main "issue" I see (and it is kinda minor) is that the derived constructor lets the parent class use its default colo(u)r value ("red"), and then supplies its own. That's kinda wasteful, when you could have given it the correct one from the get-go.

Rectangle::Rectangle(string col, int len, int br) : Polygon(col) {
    length =len;
    breadth=br;
};

Now, having done the above, you might as well intialize all the members that way:

Rectangle::Rectangle(string col, int len, int br) 
    : Polygon(col), length(len), breadth(br) {};

Hmmm. Now that I look at this, there's another thing wrong with it. Your constructors are passing in std::string objects by copy, and not modifying them. That's a waste too. All the constructor string parameters ought to be changed to string const & parameters. This potentially avoids an extra copy construction of a string at runtime, and notifies the compiler and the users that you aren't actually modifying the input strings (which is good practice when you in fact aren't).

So the final version would look more like:

Rectangle::Rectangle(string const & col, int len, int br) 
    : Polygon(col), length(len), breadth(br) {};

This formulation takes you from 4 std::string constructions (and 3 destructions) for every Rectangle constructor called down to 2. It can be further taken down to one by making the same change to the Polygon constructor.

Answer 5

You should call the base constructor with the col parameter:

Rectangle::Rectangle(string col, int len, int br) : Polygon(col)
{
    //setColour(col);
    length =len;
    breadth=br;
}

Concerning the getArea():
The reason it doesn't compile when you remove it is because that function is marked as pure virtual in your Polygon class virtual double getArea()=0; using the =0;

Answer 6

For your PS regarding Rectangle::getArea(): the declaration in Polygon of virtual double getArea()=0; means that the function is a pure virtual function. You can think of this conceptually: "All polygons have an area, so I should be able to ask what it is, but unless the polygon has a particular type (square, circle), it won't know what its area is".

What this means is that your Polygon class is an abstract class. By not defining getArea() in the Rectangle class, your rectangle class is also an abstract class. You can't instantiate a Rectangle because the compiler doesn't know about any Rectangle::getArea() function definition.

Answer 7

You can also add call to the base class constructor in your initializer list:

Rectangle::Rectangle(string col, int len, int br)
    : Polygon(col), length(len), breadth(br)

That uses the base class' constructor, so is a bit neater.

Answer 8

I can think of a number of possible problems here:

1. Use initializer lists to assign the values.

2. Call the base class constructor to set the color.

3. A string might not be the best type to represent a color. Maybe an enum or a separate color class would be better here. This also prevents passing invalid colors, if properly done.

4. Speaking of invalid values: length and breadth should be validated in the constructor (you don't want to end up with negative areas, do you?). At least use an assertion. It has no effect on release builds, but prevents development errors. For a more public interface, exceptions may also be an option (this is personal taste to some degree).

5. If you really want to use a string for the color, pass it by const reference (and probably test for edge cases like the empty string).

6. printDetails should probably be virtual, so you can call it with a base class pointer. The current implementation might not behave as intended.

7. The class seems to be designed for polymorphism. A virtual destructor has to be defined, if deletion from a base class pointer is required. Since there already is a virtual method, it probably won't hurt either.

8. getArea and printDetails should be declared const, so that they can be called on const objects. They shouldn't modify the object.

This is just a list of possibilities. Many of them depend on the intended usage of the class and might not be needed, but it doesn't hurt to consider them carefully.

Answer 9

As mentioned printDetails won't behave as expected.
I also think that just declaring getArea() within Rectangle class is kinda cheating because you do not provide implementation for it, and if you happen to call it within you code you would get a linker error.

Answer 10

An initialization order issue is possible. Polygon::setColour could call the pure virtual Polygon::getArea. (There is no indication that it would need to, but the possibility exists.) The implementation in Rectangle would presumably need length and breadth to compute the area, but they are not initialized yet.

The minimal fix is to initialize length and breadth before calling setColour:

Rectangle::Rectangle(string col, int len, int br)
{
    length =len;
    breadth=br;
    setColour(col);
}

It would be best, of course, to drop the pure virtual getArea() declaration from Polygon because it doesn't appear to be needed by any Polygon methods. But that is outside of the scope of the question.