Implementing a string class in C++

Question

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?

• forsvarir: If you are after a comparative review of the implementations, then this question is on topic, if you are seeking advice about how to deal with conflict in interview situations, I think it's probably better suited to programmers.stackexchange (where similar questions have probably been asked).
• forsvarir: I've attempted to reword your question so that it would be on-topic for this site. It's not exactly what you originally asked (which I still think is more suited to programmers), but it may be useful for you going forward. If you like the edit, I will try to get the question reopened. If not, feel free to roll it back.
• Jagannath: @forsvarir I am fine with the edit.
• user34073: The only potential issue I have here is that it could become a his code/my code situation, which we do not allow.
• Jagannath: No worries. I'm fine if it stays closed.As I mentioned in my question,his code has double delete issue in the event of an exception.it was my bad luck to face such an interview.

Answer 1

Your version

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

is

1. Exception safe
2. Correct (assuming that copy and move constructors are implemented correctly)
3. Fairly efficient in C++11 if the move constructor is implemented
4. 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:

1. If allocation fails, m_data becomes a dangling pointer and destruction (or another assign) will lead to a double delete.
2. If this == &other it doesn't return a value and therefore leads to undefined behavior (again).
3. 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)
4. Less seriously, it might lead to the content of m_data being initialized twice (depending on the optimizer and it's knowledge of strcpy)
5. Cannot be extended to support C++11 by implementing only an additional move constructor.

Furthermore, the style could be improved as well:

1. By using an early return instead of a long indented block
2. 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)
3. 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;
} 

• Jagannath: Thanks. The fixed version is good with the early return if other.m_data == 0.
• forsvarir: With regards to this other.m_data == 0, Empty string != null string, so can be represented by a 1 char string which consists of only a null terminator, so perhaps you mean null strings in your explanation? I'm not sure why checking for null on construction / assignment from char* would cause overly complicated code.
• Joe: @forsavir yes I meant null strings in point 3. if allocation fails after delete you might not even be able to allocate a 1 char string, so you'd have to work with a static allocation for the empty string (as you would otherwise lose even weak exception safety)
• Edward: I'd suggest using nullptr rather than 0 in the line m_data = 0;. Also, since you already have the length, you could use the somewhat faster std::memcpy instead of std::strcpy to copy both the string and the terminating NUL character.
• Joe: @Edward as the question mentions the rule of three I assumed C++98 and therefore didn't use nullptr. You are correct about the memcpy optimization.