Review of three constructors for a String class

Question

I have the following declaration in my String.h file:

private:
 char* nstring;
 int nlength;
};

The following are three constructors I've implemented in String.cpp. I would like these to be reviewed.

// Default constructor for this class. Initializes an empty string.
string::string() {
 nlength = 1; // for "0円" terminating character at the end of the char array
 nstring = new char[nlength];
 nstring[nlength - 1] = '0円';
}
// Constructor for String class. Initializes a string based on the given C string.
string::string(const char* input) {
 nlength = strlen(input) + 1;
 nstring = new char[nlength];
 for (int i = 0; i < (nlength - 1); i++) {
 nstring[i] = input[i];
 }
 nstring[(nlength - 1)] = '0円';
}
// Copy constructor for String class. 
// Initializes a string from an already existing string. 
// The contents of the existing string should be copied over to this string.
string::string(const string& S) {
 nlength = S.nlength;
 nstring = new char[nlength];
 for (int i = 0; i < (nlength - 1); i++) {
 nstring[i] = S.nstring[i];
 }
 nstring[(nlength - 1)] = '0円';
}

• Loki Astari: Its not the constructor that it is hard. But the interaction of the compiler generated methods. Constructor (default - copy)/Destructor/Assignment (and now Move in C++11). These all have to work together to make memory management work correctly.

Answer 1

Your lengths are off by 1 everywhere. An empty string has length 0 not 1, "Hello" has length 5 not 6, etc.

Your constructor from the const char* can just assign every character in the loop (since we know the nlengthth character of input will be 0円, right? Even better would be to use memcpy:

nlength = strlen(input); 
nstring = new char[nlength + 1];
memcpy(nstring, input, nlength + 1);

Also you have to be careful on the copy constructor. What happens in this code?

string hello("hello");
hello = hello;

• letter Q: Thank you! I was wondering: why is memcpy better than the for loop?
• letter Q: I'm using c++11 and I can use the memcpy method. Is there a header I need to include?
• microtherion: I disagree with @Barry: There’s nothing wrong with the lengths as written, as long as nlength is not returned from the size() method of the string.
• Loki Astari: Your example "What happens in this code?" is not copy construction; but rather assignment. Which is easy to implement using the copy and swap idiom.
• Loki Astari: @Barry: cppreference is actually terrible resource with lots of errors. But it is OK for quickly checking things like this.

Answer 2

The big question here is why exactly you’re writing a string class.

• If you’re doing this for practical use, you’d probably be better off with std::string, or writing a subclass thereof.
• If you really need some specialized functionality, you might still be better off with making nstring a std::vector<char> and leaving the memory management to the library.
• If you want to manage your own memory, you need to be concerned about exception safety. (削除) Right now, if any of the calls to new char[] throws an exception, you’re leaving your string instances in an inconsistent state. (削除ここまで) ETA: While the general point stands, @LokiAstari in comments correctly points out that this is not a concern in this particular class, as there is only a single allocation in the constructor.

• Jamal: +1. If you go with the vector, the third point is unneeded. I do hope the OP is okay with a vector, knowing the exception-handling.
• Loki Astari: If new throws an exception (and it is not caught like the code) in the constructor then the object is never created and does not exist so there is not state to be inconsistent with.
• microtherion: @LokiAstari, you’re right; I’ve edited my response.

Answer 3

(削除) When dealing with length, prefer std::size_t. This is an unsigned integer type that is also the return type of the sizeof operator. It is not good to use int because you cannot guarantee that any length will fit. Your code will break if the user constructs an object that is too large. There's also this issue. Accordingly, your loop counter type throughout the class should be std::size_t. (削除ここまで)

CORRECTION: @LokiAstari has pointed out that this is now wrong, according to Bjarne Stroustrup (the creator of C++) and other top C++ experts.

The main consensuses here are that:

• mismatching signed/unsigned is a source of bugs
• prefer signed unless the extra bit is needed for larger values
• the STL was wrong about this all along

More information can be found here:

• Going Native
• Interactive Panel: Ask Us Anything

• Loki Astari: Until very recently I also applied the above rule in my code (because I got it from the behavior of the standard library). But while at "Going Native" all the big guys in the C++ community said that it was a bad idea. You should use signed types for everything (apart for ints that you are using to hold bit flags). They basically said that the got it wrong when designing the standard library.
• Loki Astari: See: Bjorn describe the issue and then Herb apologize about the standard: See: channel9.msdn.com/Events/GoingNative/2013/…
• Jamal: @LokiAstari: So this must be yet another reason being my computer architecture professor's words...
• Loki Astari: @microtherion: The general discussion on ints tarts at 41:05 The specific bit about unsigned starts at 42:54 Herb starts his apology at 44:25 and wraps up at 46:00. The basic argument is that mixing signed/unsigned introduces errors. But the two problems it (using unsigned) tries to solve don't really exist. Example: setSize(unsigned int s) Now call it like this: setSize(-1); Works perfectly well but probably does not do what you want.
• Jamal: (and I think we should take this to chat)