2
\$\begingroup\$

I just recently got back into C++ and did a quick and simple exercise in the Rule of Three. The code for the copy constructor, overloaded assignment operator, and destructor follows below for my class (code is also below), and I was wondering if someone could take a quick look at it and point out anything that I overlooked.

class Coordinate
{
    private:
        int x;
        int y;
        string *name;

    public:
        Coordinate(int X, int Y, string Name)
        {
            this->x=X;
            this->y=Y;
            *(this->name)=Name;
        }

        //COPY CONSTRUCTOR
        Coordinate(const Coordinate& source)
        {
            this->x=source.x;
            this->y=source.y;
            this->name=new string(*(source->name));
        }

        //OVERLOADED ASSIGNMENT OPERATOR
        Coordinate& operator= (const Coordinate& source)
        {
            //check for self-assignment aka Coordinate1=Coordinate1
            if(this==&source)
            {
                return *this;
            }

            this->x=source.x;
            this->y=source.y;
            this->name=*(source->name);

            return *this;
        }

        //DESTRUCTOR
        ~Coordinate()
        {
            delete name;
        }

        void setX(int X)
        {
            this->x=X;
        }

        void setY(int Y)
        {
            this->y=Y;
        }

        void setName(string Name)
        {
            *(this->name)=Name;
        }

        const int getXValue()
        {
            return this->x;
        }

        const int getYValue()
        {
            return this->y;
        }

        const string getName()
        {
            return *(this->name);
        }
};

This post corrects some errors that led to my previous question to be rejected. I realize that I should have included all the code for my class from the beginning.

\$\endgroup\$

closed as off-topic by nhgrif, syb0rg, Ethan Bierlein, Emily L., RubberDuck May 13 '15 at 13:47

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – nhgrif, syb0rg, Ethan Bierlein, Emily L., RubberDuck
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ I'm not a C++ expert, but my IDE thinks that both instances of source->name should actually be source.name It won't compile your code as-is. \$\endgroup\$ – nhgrif May 12 '15 at 23:50
  • \$\begingroup\$ Do you really need to make name a pointer? string *name;. If you just make it a normal automatic variable all your problems go away. \$\endgroup\$ – Martin York May 13 '15 at 5:28
  • 1
    \$\begingroup\$ Does not compile under g++. Voted to close for off-topic. \$\endgroup\$ – Mast May 13 '15 at 10:56
6
\$\begingroup\$

This unfortunately shows a rather erroneous understanding of the rule of three. Yes, all three methods are present, but they do not serve their expected purposes.

  • Your copy constructor doesn't free the old name before assigning the new one. That's a memory leak.
  • Your copy constructor dereferences an unitialized pointer. That's undefined behavior (almost certainly a segfault in this case).
  • Your assignment operator makes a shallow copy. Not only is it unintuitive to consumers since objects may affect each other, the destructors would attempt to double free the same object likely resulting in a segfault.

Provided the other two were fixed, your destructor is actually correct.

This code may technically compile, and it may even execute, but I would suggest that you run it through something like valgrind before continuing much farther. In addition, it might help to read more about the Rule of 3, learn about the copy and swap idiom, and learn about the Rule of 0, or more generally RAII and idiomatic resource management.


From a design/style perspective:

  • This isn't a Coordinate. It's a coordinate with a name, perhaps a NamedCoordinate. I would expect the x,y to be encapsulated in some kind of struct in real code and then a NamedCoordinate to contain an instance of that.
  • Arguments that are not modified should be taken by const reference rather than value. For example, void setName(string Name) makes an unnecessary copy of Name. This gets a bit more complicated in C++11, I'm afraid as your taking parameters by value would be correct, but you would missing std::move calls. Anyway, for C++ < 11, you just need to know that non-modified parameters should be taken by const reference.
  • using namespace std; is a bad habit
  • It's customary to leave off this unless it must be included in function calls or member accesses.
  • snake_case and camelCase are the standards for argument/parameter/member/etc names with either snake_case or PascalCase for classes. Your class name is good, but some of your uppercased parameter names are rather unusual.
\$\endgroup\$
4
\$\begingroup\$

A couple of comments:

I would remove the "this->" from the methods. This is implied when you are in member functions so is not necessary to include.

I would also use initializer lists, both for the constructor and the copy constructor.

I'm also going to guess that we are using std::string. If that's the case, it is preferable to use the std::string copy and assignment semantics, and use it directly.

On the Getters, you probably would want to return the value, not a const version of the value. However, you'd probably want to mark the function as const.

(a further edit): Also, I would probably pass my strings by const reference on both the constructor and setter.

class Coordinate
{
    private:
        int x;
        int y;
        std::string name;

    public:
        Coordinate(int X, int Y, const std::string& Name) : x(X), y(Y), name(Name)
        {
        }

        //COPY CONSTRUCTOR
        Coordinate(const Coordinate& source) : x(source.x), y(source.y), name(source.name)
        {
        }

        //OVERLOADED ASSIGNMENT OPERATOR
        Coordinate& operator= (const Coordinate& source)
        {
            //check for self-assignment aka Coordinate1=Coordinate1
            if(this==&source)
            {
                return *this;
            }

            x=source.x;
            y=source.y;
            name=source.name;

            return *this;
        }

        //DESTRUCTOR
        ~Coordinate()
        {
        }

        void setX(int X)
        {
            x=X;
        }

        void setY(int Y)
        {
            y=Y;
        }

        void setName(const std::string& Name)
        {
            name=Name;
        }

        int getXValue() const
        {
            return x;
        }

        int getYValue() const
        {
            return y;
        }

        string getName() const
        {
            return name;
        }
};
\$\endgroup\$
  • 1
    \$\begingroup\$ The copy-constructor, assignment operator, and destructor can be removed, as the compiler generated version will work. \$\endgroup\$ – Sjoerd May 13 '15 at 0:03
  • \$\begingroup\$ @Sjoerd: Thanks for the heads up! Also, are my copy-constructor, assignment operator, and destructor written correctly as they are? \$\endgroup\$ – xXAnointedXx May 13 '15 at 0:36
  • 2
    \$\begingroup\$ All good except use the copy and swap idium for assignment operator. Well don't actually implement it in this case as the compiler generated version works (but if you need to implement assignment operator use copy and swap). \$\endgroup\$ – Martin York May 13 '15 at 5:33
2
\$\begingroup\$

Design

The main point is that you are not following Separation of Concerns Pattern. Your class shold either:

  1. Perform resource management.
  2. Perform business logic.

In your case this is a business logic class and thus there should be no resource management. If there is no resource management then you don't need to implement the rule of three (you delegate that to classes that do resource management).

The only member that needs resource management is name. The class std::string already implements the rule of three for you and so you do not actually need to do any work.

class Coordinate
{
    private:
        int          x;
        int          y;
        std::string  name;  // Note normal member not a pointer.
                            // The std::string class is a resource
                            // management class let it handle all
                            // the tricky memory management.
public:
    Coordinate(int x, int y, string name)
        : x(x)
        , y(y)
        , name(name)
    {}

    // Copy constructor.
    // The compiler implemented version works fine
    // as long as your class is not doing resource management.

    // Assignment operator
    // The compiler implemented version works fine
    // as long as your class is not doing resource management.

    //Destructor
    // The compiler implemented version works fine
    // as long as your class is not doing resource management.

Geters and Setters break encapsulation.

By using getters and setters you are exposing implementatin details about your class that will tightly couple your class to the code that uses it.

    void setX(int X)
    {
        this->x=X;
    }

    void setY(int Y)
    {
        this->y=Y;
    }

    void setName(string Name)
    {
        *(this->name)=Name;
    }

    const int getXValue()
    {
        return this->x;
    }

    const int getYValue()
    {
        return this->y;
    }

    const string getName()
    {
        return *(this->name);
    }

There are usually three scenarios where geters and setters are not needed:

Normally if you pull the data out of a class do some calculation then put the data back in the class it is better to make that calculation part of the class (ie a method call on the class).

Another thing people use getters incorrectly for is to serialize or display the object. Better to get the object to display itself. For nameal serialization use operator<<

If you class is just a bag of data that is being passed around. Then treat it as a bag and make the members public.

If your object falls into another class then maybe (but only maybe) getter and setters can be justified.

\$\endgroup\$
1
\$\begingroup\$

Your use of const in these accessors is incorrect:

    const int getXValue()
    {
        return this->x;
    }

    const int getYValue()
    {
        return this->y;
    }

There is no point in stating that the returned int is constant. Those numbers are returned by value, so why should the Coordinate class care what the caller does with the number?

Perhaps you meant to write int getXValue() const, to indicate that the method does not cause the Coordinate object to mutate.

Also, why are setX(…) and getXValue() named asymmetrically? Why not getX()?


The same type of const error occurs with getName():

    const string getName()
    {
        return *(this->name);
    }

But here, there is a complication. Since the return type is string, you're actually going to return a copy of the string held by the Coordinate. You probably want to return a reference to the string rather than a copy. In that case, you should write const std::string& getName() const { … }, and there, the const does make a difference.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.