NamedPoint class using unique_ptr for members

Question

After reading this old article from 2001 I have tried to implement the class from it using unique_pointer.

An author's claim is that C++ is not appropriate for large software projects because

My primary complaint against C++ is that the language is so complicated, and has enough booby-traps, that average and above-average programmers have a difficult time writing code without serious bugs.

This is the test he gave to candidates on his job interviews:

As part of my standard interview for C++ candidates I ask them to write me a small class with the intention of evaluating their command of the language. This also gives us a reasonable coding sample to discuss during the interview. I can ask about potential improvements, extensions and testing strategies.

The request of the author is:

Write a Named Point class with three members: two floating point values for the coordinates on an X-Y plane, and a name represented as a 'char *'. Assume that this class will be used for some sort of wargame or simulation program that treats the world as flat and that these named points will be used to represent things like cities, battlefields, etc.

This is the version of class implementation from the article:

class NamedPoint
{
private:
    float x;
    float y;
    char *name;

public:
    NamedPoint (float x, float y, char *name)
    {
        this->x    = x;
        this->y    = y;
        if (name == NULL)
            this->name = NULL;
        else
        {
            this->name = new char[strlen(name) + 1];
            strcpy (this->name, name);
        }
    }

    ~NamedPoint ()
    {
        if (name != NULL)
            delete name;             
    }

    // NOTE: Most interviewees start with a signature
    //       like this:
    //           NamedPoint (NamedPoint copy)
    //
    NamedPoint (const NamedPoint & copy)
    {
        this->x = copy.x;
        this->y = copy.y;

        if (copy.name != NULL)
        {
            this->name = new char[strlen (copy.name) + 1];
            strcpy (this->name, copy.name);
        }
    }

    NamedPoint & operator=(const NamedPoint & copy)
    {
        this->x = copy.x;
        this->y = copy.y;
        if (this->name != NULL)
            delete this->name;

        if (copy.name != NULL)
        {
            this->name = new char[strlen (copy.name) + 1];
            strcpy (this->name, copy.name);
        }

        // Note that we haven't nulled out this->name, so
        // we can get a double-delete problem...
    }

    float getX()          {return x;}
    float getY()          {return y;}
    const char *getName() {return name;}

    void  setX(float x)       {this->x = x;}
    void  setY(float y)       {this->y = y;}
    void  setName(char *name) {if (this->name != NULL)
                                   delete this->name;
                               if (name == NULL)
                                   this->name = NULL;
                               else
                               {
                                   this->name = new char[strlen(name) + 1];
                                   strcpy (this->name, name);
                               }}
};

There are a lot of problems with memory management because of raw pointers using. But it was before C++11 was invented. So now C++11 with smart pointers can help us to avoid mentioned problems? This is my version of this class (with smart pointers):

class NamedPoint
{
private:
    float x_;
    float y_;
    std::unique_ptr<char[]> name_;

    void  setName(const char* name) {
        if (name != nullptr)
        {
            auto len = strlen(name) + 1;
            name_ = std::make_unique<char[]>(len);
            strcpy(name_.get(), name);
        }
        else
        {
            name_.reset();
        }
    }

public:
    NamedPoint(float x, float y, std::unique_ptr<char[]> name) : x_(x), y_(y), name_(std::move(name)){}

    NamedPoint(const NamedPoint& copy) : x_(copy.getX()), y_(copy.getY())
    {
        setName(copy.getName());
    }

    NamedPoint& operator=(const NamedPoint& copy)
    {
        x_ = copy.getX();
        y_ = copy.getY();
        setName(copy.getName());

        return *this;
    }

    float getX()const { return x_; }
    float getY()const { return y_; }
    const char *getName()const { return name_.get(); }
    void  setX(float x)       { x_ = x; }
    void  setY(float y)       { y_ = y; }

    void  setName(const std::unique_ptr<char[]>& name) {
        setName(name.get());
    }

};

I've started with smart pointers recently so code review is needed.

UPDATE: Should I implement also destructor, move constructor and move assignment operator here?

Answer 1

My gut instinct is to agree with @Olzhas Zhumabek, that you should be using a std::string for your name_. With it a lot of the issues go away and you've already deviated from the original spec by not using a char*.

Looking at your code, your choice of unique_ptr has created a discrepancy in the interface for your class. You construct the object and set the value of name using std::unique_ptr<char[]> name, however you only return the name as a const char*. This feels odd.

Your copy constructors use the public interface to perform the copy semantics. Again, this feels odd. Part of the benefit of writing the copy constructor as a member of the class is that it can directly access the private class members of both sides of the assignment and avoid the overhead of the public interface.

Answer 2

First of all, the class has very limited usability. Lookup by name will be much longer than lookup by id or index. Second, class is not relevant here since it has setters and getters. I would just write this:

#include <string>

struct named_point
{
    float x;
    float y;
    std::string name;
}

Since the users need access to all members of the class, the members should be public. There is no real invariant to hold here. Using std::unique_ptr for this job is somewhat odd. Use std::string. It will cleanup your code dramatically, since it implements rule of 5 itself, a lot of burden will be lifted.