1
\$\begingroup\$

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';
}
\$\endgroup\$
  • 4
    \$\begingroup\$ 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. \$\endgroup\$ – Martin York Oct 31 '13 at 23:02
4
\$\begingroup\$

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;
\$\endgroup\$
  • \$\begingroup\$ Thank you! I was wondering: why is memcpy better than the for loop? \$\endgroup\$ – Q Liu Oct 31 '13 at 0:20
  • \$\begingroup\$ I'm using c++11 and I can use the memcpy method. Is there a header I need to include? \$\endgroup\$ – Q Liu Oct 31 '13 at 0:29
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – microtherion Oct 31 '13 at 0:44
  • \$\begingroup\$ 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. \$\endgroup\$ – Martin York Oct 31 '13 at 22:52
  • \$\begingroup\$ @Barry: cppreference is actually terrible resource with lots of errors. But it is OK for quickly checking things like this. \$\endgroup\$ – Martin York Oct 31 '13 at 22:53
2
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ +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. \$\endgroup\$ – Jamal Oct 31 '13 at 1:41
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Martin York Oct 31 '13 at 22:55
  • \$\begingroup\$ @LokiAstari, you’re right; I’ve edited my response. \$\endgroup\$ – microtherion Nov 2 '13 at 21:19
1
\$\begingroup\$

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:

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Martin York Oct 31 '13 at 23:00
  • 1
    \$\begingroup\$ See: Bjorn describe the issue and then Herb apologize about the standard: See: channel9.msdn.com/Events/GoingNative/2013/… \$\endgroup\$ – Martin York Oct 31 '13 at 23:00
  • \$\begingroup\$ @LokiAstari: So this must be yet another reason being my computer architecture professor's words... \$\endgroup\$ – Jamal Oct 31 '13 at 23:08
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – Martin York Nov 3 '13 at 3:05
  • 1
    \$\begingroup\$ (and I think we should take this to chat) \$\endgroup\$ – Jamal Nov 3 '13 at 3:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.