Implementing a string class in C++

I recently attended an interview for a C++ position in a very reputed bank.

The interviewer started off by asking me to implement a string class but was only interested in constructor, copy-constructor, assignment operator and destructor.

Basically, he wanted to know how I would implement Rule of Three and nothing else. He was not even interested in move operations.

So, I implemented the assignment operator in terms of copy constructor:

class String
{
  /*....*/
String& operator=(String other)
{
   std::swap(m_data, other.m_data);
   return *this;
}

private:
   char* m_data;
};

Since this is pretty common way to implement assignment operator, I thought he would like it. The problem started with that.

He questioned why I implement it that way. When I explained that this is exception safe way of implementing it, he asked what's wrong in implementing it:

String& operator=(const String& other)
{
   if (this != &other)
   {
     delete[] m_data;
     m_data = new char[strlen(other.m_data) + 1] {'\0'};
     strcpy(m_data, other.m_data);
     return *this;
   } 
}

His argument was:


We are about to overwrite the data anyways so what's wrong in deleting first even if it's not exception safe?

In the moment it did not occur to me that code will crash in the event of an exception but I did try to argue that internal data should retain original values in the event of an exception.

Had he argued on the basis that copy-and-swap will always do reallocation even if there is enough size, etc, I would have felt happy. But his argument was that he wants performance at any cost.

Can somebody help me with a comparative review of these two operator= implementations so that I am in a better position to argue my case / understand the interviewer's position if I encounter similar situations in the future?

Your version

String& operator=(String other)
{
   std::swap(m_data, other.m_data);
   return *this;
}

is

• Exception safe

• Correct (assuming that copy and move constructors are implemented correctly)

• Fairly efficient in C++11 if the move constructor is implemented

• Potentially slightly less efficient than implementing separate copy & move constructors

The interviewer's version

String& operator=(const String& other)
{
   if (this != &other)
   {
     delete[] m_data;
     m_data = new char[strlen(other.m_data) + 1] {'\0'};
     strcpy(m_data, other.m_data);
     return *this;
   } 
}

is terrible, for the following reasons:

• If allocation fails, m_data becomes a dangling pointer and destruction (or another assign) will lead to a double delete.

• If this == &other it doesn't return a value and therefore leads to undefined behavior (again).

• Will fail if other.m_data == 0 (this might be ok if null strings are disallowed, but that would make even weak exception safety complicated)

• Less seriously, it might lead to the content of m_data being initialized twice (depending on the optimizer and it's knowledge of strcpy)

• Cannot be extended to support C++11 by implementing only an additional move constructor.

Furthermore, the style could be improved as well:

• By using an early return instead of a long indented block

• By not using C-style strings (and having a length member instead, although your version is presumably implemented in the same way as only m_data is swapped)

• strlen and strcpy are not prefixed with std:: which they should be in C++ (having strlen and strcpy in the global namespace is allowed but not required by the standard.

A fixed version (still using a C-style representation) might look as follows:

String& operator=(const String& other)
{
   if(this == &other)
      return *this;

   delete[] m_data;
   m_data = 0; // weak exception safety, which might be sufficient, but only if null strings are allowed, complicating the following code

   if(!other.m_data) // strlen will fail if other.m_data == 0
      return *this;

   std::size_t length = std::strlen(other.m_data);
   m_data = new char[length + 1];
   std::memcpy(m_data, other.m_data, length + 1);

   return *this;
}

StackExchange Code Review Q#136170, answer score: 8