5
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Well, you are right that the better way is to use string instead if char*. Saved it just because of original request. Let's imagine it should be pointer of some PointProperties type. The another note about raw pointer as a return value: please see that \$\endgroup\$ – shtkuh Jul 20 '16 at 12:58
  • 2
    \$\begingroup\$ @shtkuh I don't have a problem with you returning raw pointers, I have a problem with the inconsistency of your interface. The goal of you using a unique_ptr is to simplify memory management within the class. Why should a client of the class be expected to facilitate that? It would be better to have your constructor and setName take in a const raw pointer and have the constructor create the unique_ptr that's used internally by the class. I should be able to call node.setName(othernode.getName()). \$\endgroup\$ – forsvarir Jul 20 '16 at 13:18
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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