String class in C++

Question

I was writing simple String class implementation (and it is quite ordinary), but I had found out that constructors, destructors and operator = are the most sensitive areas. So I wonder, if my implementation good in the sense of C++11/14 standard and are they efficient enough?

String::String()
{
 m_characters = new char[0];
 m_size = 0;
}
String::String( const int size )
{
 m_size = size;
 m_characters = new char[size];
}
String::String( const char* str )
{
 m_size = 0;
 int i = 0;
 while ( str[i] )
 {
 m_size++;
 i++;
 }
 m_characters = new char[m_size];
 for (int i = 0; i < m_size; i++)
 m_characters[i] = str[i];
}
String::String( const String& string )
{
 m_size = string.m_size;
 m_characters = new char[m_size];
 for ( int i = 0; i < m_size; i++ ) 
 m_characters[i] = string.m_characters[i];
}
String::~String()
{
 delete [] m_characters;
} 
String& String::operator=( const String& string )
{
 if ( this != &string )
 {
 m_size = string.m_size;
 m_characters = new char[m_size];
 for (int i = 0; i < m_size; i++ )
 m_characters[i] = string.m_characters[i];
 }
 return *this;
}

I omit the header file because I think that class interface is quite clear. If required, I can attach it.

UPD. SO does not allow me to put full code here, so I've put it on gist at github.

• Incomputable: The implementation is non conformant: it does not include null terminator automatically. The real string will be C compatible.
• Deduplicator: It would help if you actually posted your class, and not just the implementation of selected member-functions!
• Montreal: @Deduplicator, CW was complaining about my question being 95% of code, so I left things which were most interesting to me. I can add a link to full class on gist.
• Incomputable: @Montreal, links rot. SE would like to keep the stuff for eternity.

Answer 1

Well, in order to evaluate your code, I'll assume (Yes, assume makes an ass out of you and me, but what can I do...) that your class is of the form:

class String {
 char* m_characters;
 int m_size;
public:
 String();
 String( const int size );
 String( const char* str );
 String( const String& string );
 ~String();
 String& operator=( const String& string );
};

---

Now, there are various concerns with your implementation:

1. std::size_t is a far superior type for sizes, as it is actually dedicated for that task instead of just sometimes coincidentally having somewhat fitting limits.

2. You are missing any and all observer- and modifier-functions.
   At least that makes the lack of a separate member for the strings capacity less keenly felt.

3. You make no provision for the 0-terminator, meaning your strings cannot be used where c-strings are expected even after providing some observers.

4. You make no use of rvalue-references, and the optimizations knowing your source will be destroyed allows.

5. One of the main advantages of defining a separate default-constructor is the possibility of not letting it allocate any memory, meaning it can be constexpr and noexcept.

6. Any constructor which can be called with one argument must be marked explicit unless using it should be used for implicit type-conversions.
   The one expecting a size is not marked.

7. Even if you would not use std::strlen from the standard library, it is an utter waste to count a c-strings size twice in parallel in different variables.

8. The standard-library contains std::copy_n for copying a range of known length. And it is probably better-optimized than a simple hand-rolled loop.

9. Consider using the initializer-list where you can. Though it doesn't matter much with primitive types, it's a good habit to get into.

10. You should use the copy-and-swap idiom for assignment. Doing so fixes:

    • The self-assignment-check is an anti-pattern because it favors the rare self-assignment over the expected case, thus being a pessimization.
    • You fail to free the original memory in the assignment-operator.

As a bonus, my take on the additional members and free-functions in the full gist version:

1. The indexing-operators should be constexpr, noexcept, both return a reference and be defined in-class for better inlining.
2. You are missing == and != from your relational operators. Those are the easiest and most used ones, which makes their absence especially puzzling. Define != in terms of ==.
3. You should define all the other relational operators in terms of < to avoid duplicate code. And that one should use std::lexicographical_compare.
4. All relational operators should be noexcept and constexpr.
5. The member-function void Print( std::ostream& os ) const should be the free (or at least friend, you need more observers at this point) function std::ostream& operator<<(std::ostream& os, const String& s).
6. GetSize() should be size(). Following interface-conventions is not only for ease of use by humans but also for templates.
   Also, mark it noexcept and `constexpr ́.
7. Your ToUpper causes UB if called with a negative char. And even if that is avoided, the adjustment only works for ASCII a-z. You know that islower is locale-aware? And what's wrong with `toupper` that you are trying to correct?

Answer 2

There is no point in having this constructor.

String::String()

You can simulate its affects by putting a default parameter in this constructor.

class String
{
 public:
 explicit String(int size = 0); // Have a default parameter and
 // Covers both your first two constructors.

For the next constructor:

String::String( const int size )
 ^^^^^ I see little point in this.

Use initializer list when you can.

This:

{
 m_size = size;
 m_characters = new char[size];
}

Should be:

 : m_size(size)
 , m_characters(new char[size])
{}

In this case it makes no difference (I will admit). But not doing it will make you classes more brittle in for future modifications. So it is always a good habit to get into.

I would argue that this constructor should test for nullptr

String::String( const char* str )

Now the standard does not. But there have been many/many arguments over this. I think you will reduce errors be detecting this and shutting them down.

Don't try and get string size yourself.

 m_size = 0;
 int i = 0;
 while ( str[i] )
 {
 m_size++;
 i++;
 }

There is a standard library function for this strlen(). On some systems it is even highly optimized to be quicker than stuff you can write using standard C. So use the system version it will potentially be quicker than stuff you write.

There is a standard library function for doing a copy:

 for (int i = 0; i < m_size; i++)
 m_characters[i] = str[i];

Prefer:

std::copy(m_characters, m_characters + m_size, str);

It will not be slower than your code but could be quicker. It also expresses intent much more clearly.

Same comments as above:

String::String( const String& string )
// Prefer initializer list.
{
 m_size = string.m_size;
 m_characters = new char[m_size];
 // Use standard algorithms.
 for ( int i = 0; i < m_size; i++ ) 
 m_characters[i] = string.m_characters[i];
}

Destructor is very standard and will work.

String::~String()
{
 delete [] m_characters;
}

The standard pattern for the assignment operator is Copy and Swap Idiom (please look it up).

String& String::operator=( const String& string )
{
 // Suspect check for self assignment. 
 // Does it really make things quicker?
 if ( this != &string )
 {
 m_size = string.m_size;
 // This leaks the old value.
 m_characters = new char[m_size];
 for (int i = 0; i < m_size; i++ )
 m_characters[i] = string.m_characters[i];
 }
 return *this;
}

The check for self assignment. Yes it looks like it speeds the code up when you do self assignment. It does. But conversely it slows the code down when you don't have self assignment (not very much but a tiny bit). The problem is that actual self assignment is very exceptionally rare.

So the question becomes: Is the size of number of non self assignment that happen per self assignment multiplied by a tiny cost smaller than 1 * cost of making a copy an average string. People have tried this and found the most efficient version is not to test for self assignment but always make a copy.

String& String::operator=(String string) // Pass by value to get a copy.
{
 string.swap(*this);
 return *this;
}

I would also note that you missed the move operators. Move semantics can improve the performance a bit as they avoid copying in a couple of situations).

• Mathieu Guindon: Comments are not for extended discussion; this conversation has been moved to chat.

Answer 3

So I wonder, if my implementation good in the sense of C++11/14 standard and are they efficient enough?

By means of c++ standards (c++11 and any earlier) you simply won't write such class at all.

You simply use std::string and be done with it.

---

More points:

m_characters = new char[0];

Doesn't makes sense. Rather use

m_characters = nullptr;

String::String( const String& string )

string is already used as a type in the c++ standard library, this may confuse your compiler in certain circumstances.

• Montreal: I am aware that std::string exist, I am exploring some basic pointer-related C++ stuff. Concerning string, I do not use using namespace std; stuff.
• πάντα ῥεῖ: @Montreal "I am exploring some basic pointer-related C++ stuff." You shouldn't. Rather learn about the idiomatic stuff. There's not much value in doing so when working with c++. You may want to read my blog entry about that topic.
• Loki Astari: I disagree with not using new char[0]. Sometime you want to distinguish between an uninitialized string and one that is initialized.
• Loki Astari: How can you tell the difference between an unallocated string and an empty string?
• Lightness Races in Orbit: @πάνταῥεῖ: There is nothing invalid about new char[0] (though it is arguably wasteful when nullptr can signal the same high-level "state")

Answer 4

It can be good excercise to write such thing but in production code I would not like to see "yet-another-string-class".

That being said here is my review:

String::String()

You can omit the implementation of the default constructor if you initialize the members where they are declared. This also reduces a source of error as you can't forget to initialize members when you add a new constructor.

class String
{
 std::size_t size = 0;
 char* m_characters = nullptr;
public:
 String() = default; // use compiler-generated default constructor
};

String::String( const int size )
{
 m_size = size;
 m_characters = new char[size];
}

It makes no sense for size to be of a signed type. I suggest replacing int by std::size_t which is unsigned and will be large enough to store the biggest possible object size. On some 64-bit platforms such as Microsoft Windows int is only 32-bit, whereas std::size_t will be 64-bit.

for (int i = 0; i < m_size; i++)
 m_characters[i] = str[i];

Instead of copying the string char-by-char use memcpy() which is pretty well optimized. It typically uses special CPU instructions that copy multiple bytes in parallel:

memcpy( m_characters, str, m_size );

• Incomputable: The defaulted constructor will explode. It will not make size and pointer zero, which is a bomb going to explode. Also if type is POD even VC++ will optimize it into memcpy, so calling copy is usually better.
• Deduplicator: @Incomputable Ehm, you did see the in-class-initializers?
• Incomputable: @Deduplicator, no, thank you. Though the point about copy still holds. I apologize for misunderstanding.
• Montreal: @zett42, in production I will just use std::string.
• zett42: @Incomputable What is the advantage of std::copy() over directly calling memcpy() in this case?