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';
}

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;

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.

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