Why didn't I initialize the private data member in class successfully?

Question

Now, I faced with an exercise problem: Deal with some data in a class CRect. But my results are different from what I expected.

My code is as followed:

#include<iostream>
#include<cmath>
using namespace std;
class CRect
{
public:
    void Diagonal(CRect one)
    {
        float l;
        l=sqrt( (one.right-one.left)*(one.right-one.left)+(one.bottom-one.top)*(one.bottom-one.top) );
        cout<<"The length is "<<l<<endl;
    }
    void Show(CRect one)
    {
        cout<<"("<<one.left<<","<<one.top<<")"<<"   ";
        cout<<"("<<one.right<<","<<one.bottom<<")"<<endl;
    }
    CRect(float left1,float top1,float right1,float bottom1)
    {
        left=left1;
        top=top1;
        right=right1;
        bottom=bottom1;
    }
    CRect(float left1,float top1)
    {
        left=left1;
        top=top1;
    }
    CRect(CRect &c)           
    {
        right=c.right;
        bottom=c.bottom;
    }
private:
    float left,top,right,bottom;
};

int main()
{
    CRect r1(10,10,20,20);
    CRect r2(0,0);
    r2=CRect(r1);
    r1.Show(r1);
    r1.Diagonal(r1);
    r2.Show(r2);
    r2.Diagonal(r2);
    return 0;
}

Then，the result is as the following picture: the result picture

I think that I have not initialized the left and top . But, I don't know how to correct it. I can't find the mistake in my code.

juanchopanza · Accepted Answer · 2013-03-21T12:44:30.263

Your copy constructor only "initializes"¹ right and bottom, top and left are left with garbage values:

CRect(CRect &c)           
{
    right=c.right;
    bottom=c.bottom;
}

Next, this two parameter constructor has similar flaws:

CRect(float left1,float top1)
{
    left=left1;
    top=top1;
}

You have to set all the data members to something. If you do so, you can completely remove the copy constructor and let the compiler synthesized one do its job.

¹why the inverted commas? Because you are actually assigning values to data members that, at least semantically, have already been initialized. To initialize data members to a value, use the constructor initialization list:CRect(float left1,float top1) : left(left1), top(top1), right(), bottom() {}

score 2 · Answer 2 · answered Mar 21 '13 at 12:41

2

Your copy constructor CRect(CRect &c) fails to initializer left and top. The copy constructor is called When you pass r1 and r2 to to the function Diagonal and Show.

Moreover:

You don't need to define a copy-constructor at all. A default one is generated if you don't define your own.
Show and Diagonal are member functions, they don't need to take a parameter.

answered Mar 21 '13 at 12:41

sbabbi

11,070
2
29
57

Your two points in the "Moreover" is useful. I have known it. Thanks. – Yingli Yan Mar 24 '13 at 05:24

score 1 · Answer 3 · answered Mar 21 '13 at 12:39

You have not initialized all members in CRect(float left1,float top1) constructor and your copy-constructor.

CRect(const CRect &c) 
{
    right=c.right;
    bottom=c.bottom;
    top=c.top;
    left=c.left;
}

Or even omit your copy-constructor and allow default copy-constructor do everything.

More

CRect(float left1,float top1)
{
    left=left1;
    top=top1;
    // How about right and bottom ?!
}

score 0 · Answer 4 · answered Mar 21 '13 at 12:42

0

Look at your copy constructor

CRect(CRect &c)           
{
    right=c.right;
    bottom=c.bottom;
}

should be

CRect(CRect &c)           
{
    right=c.right;
    bottom=c.bottom;
    top=c.top;
    left=c.left
}

answered Mar 21 '13 at 12:42

kassak

3,974
1
25
36

And the next step is to remove it, because it doesn't do anything that the compiler generated one wouldn't do. – juanchopanza Mar 21 '13 at 13:00

score 0 · Answer 5 · edited May 23 '17 at 10:25

My answer doesn't directly address your question, but you'll want to get in the habit of using initialization lists with your constructors to initialize your class member member variables. (I say this recognizing that initialization lists may not yet have been introduced in your class.)

For example, instead of assigning values to your member variables from your constructor like this:

CRect(float left1, float top1)
{
    left=left1;
    top=top1;
}

...you should instead initialize your member variables like this:

CRect(float left1, float top1)
    : left(left1), top(top1)
{
}

For why, see the C++ FAQ entry [10.6] Should my constructors use "initialization lists" or "assignment"? as well as C++ initialization lists on SO.

In your own programming, you may also want to consider adopting a naming convention for your member variables to more easily distinguish them from other variable types. For example, instead of:

private:
    float left;       // no decoration

...consider using one of two common conventions:

private:
    float left_;      // trailing underscore
    float m_left;     // leading 'm_'

For more about this, see Trailing underscores for member variables in C++ on SO.

OK. Thank you for your advice. I will go to see the FAQ. – Yingli Yan Mar 24 '13 at 06:02 — Yingli Yan, Mar 24 '13 at 06:02

Why didn't I initialize the private data member in class successfully?

5 Answers5