3
\$\begingroup\$

Following is my attempt for a simple implementation for the std string class. Please review my changes and share your feedback.

string.h

class string
{
    char *pcString;
    int iCapacity;
    const static int npos = -1;
public:
    string(void);
    string(int);
    string(const char*);
    string(const string&);
    string& string::operator=(const string &strInstance);
    string& string::operator+(const string &strInstance);
    int find(const char*);
    bool empty() const;
    int size() const;
    char* data() const;
    ~string(void);
};

string.cpp

#include "string.h"
#define INITIAL_SIZE 15

/* Default Constructor */
string::string(void)
{
    pcString = new char[INITIAL_SIZE];
    memset(pcString, 0, INITIAL_SIZE);
    iCapacity = INITIAL_SIZE;
}

string::string(int iSize)
{
    pcString = new char[iSize];
    memset(pcString, 0, iSize);
    iCapacity = iSize;
}

string::string(const char *pcValue)
{
    if(pcValue)
    {
        int iSize = strlen(pcValue) + 1;
        pcString = new char[iSize];
        memset(pcString, 0, iSize);
        iCapacity = iSize;
        memcpy(pcString, pcValue, strlen(pcValue));
    }
}

/* Copy Constructor */
string::string(const string &strInstance)
{
    if(strInstance.empty() == false)
    {
        this->pcString = new char[strInstance.size() + 1];
        this->iCapacity = strInstance.size() + 1;
        memset(this->pcString, 0, this->iCapacity);
        memcpy(this->pcString, strInstance.data(), this->iCapacity);
    }
}

/* empty() */
bool string::empty() const
{
    if(this->pcString)
    {
        if(memcmp(this->pcString, "", strlen(this->pcString)) == 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
}

/* '=' operator */
string& string::operator=(const string &strInstance)
{
    if(strInstance.empty() == false)
    {        
        this->iCapacity = strInstance.size() + 1;
        delete this->pcString;
        this->pcString = new char[this->iCapacity];
        memset(this->pcString, 0, this->iCapacity);
        memcpy(this->pcString, strInstance.data(), this->iCapacity - 1);        
    }
    return *this;
}

/* '+' operator */
string& string::operator+(const string &strInstance)
{
    int iNewSize = strInstance.size() + this->size() + 1;
    char *pcTemp = new char[iNewSize];
    _snprintf(pcTemp, iNewSize, "%s%s", this->data(), strInstance.data());
    string *pStrResult = new string(pcTemp);
    return *pStrResult;
}

/* size() */
int string::size() const
{
    if(this->pcString)
    {
        return strlen(this->pcString);
    }
    else
    {
        return 0;
    }
}

/* data() */
char * string::data() const
{
    if(this->pcString)
    {
        return this->pcString;
    }
    else
    {
        return NULL;
    }
}

int string::find(const char *pcInputString)
{
    char *pcSubstrResult = strstr(this->pcString, pcInputString);
    if(pcSubstrResult)
    {
        return (this->size() - strlen(pcSubstrResult));
    }
    else
    {
        return string::npos;
    }
}

string::~string(void)
{
    delete this->pcString;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ your version can't store '\0', std::string does \$\endgroup\$
    – phuclv
    Nov 3, 2016 at 14:34

5 Answers 5

6
\$\begingroup\$

Size

The biggest thing that sticks out is that your string does not have a length member. So when testing for empty or size you have to scan the string to find the result (you don't actually need to do it for empty but you do).

Would it not be easier to keep track of the string length in a member and just return it?

Resetting Memory

    memset(this->pcString, 0, this->iCapacity);
    memcpy(this->pcString, strInstance.data(), this->iCapacity - 1);      

Setting memory to zero then copying over it with new values seems completely redundant. Just copy the data into the memory you want.

Because you don't have an explicit length setting memory to zero in other places is required (but do you need to set the whole array to zero? Why not jus the first element. In C-Strings the '\0' character is the string terminator. So if the first character is '\0' then the string is empty and has length zero. At this point what the other characters hold is irrelevant.

Also you are using C functions to do your copying. Use the much better C++ functions. These will use the fastest technique to copy the current type.

std::copy(strInstance.pcString, strInstance.pcString +  strInstance->iCapacity,
          pcString);

Default Parameters

Your first two constructors are identical. You should DRY up your code and use a single constructor. The difference is one has zero parameters and the other takes none and defaults the size:

string::string(void)   // By the this is correct. BUT not often used in C++
                       // The empty list has exactly the same meaning.
{
    pcString = new char[INITIAL_SIZE];
    memset(pcString, 0, INITIAL_SIZE);
    iCapacity = INITIAL_SIZE;
}

string::string(int iSize)
{
    pcString = new char[iSize];
    memset(pcString, 0, iSize);
    iCapacity = iSize;
}

By using a default parameter you can write this in one function:

class string
{
     public:
        string(int iSize = INITIAL_SIZE); // Now you only need to write it once
                                          // If used with zero parameters the
                                          // the compiler will call this
                                          // constructor and use the default value.
}

Using this

This is a sign of bad code design.

You only need to use this if you have shadowed variables. If you have shadowed variables you are writing code that is hard to read and understand because you are using the same name to represent different objects.

So stop using this and turn your compiler warnings up so that it warns you about shadowed variables and then use meaningful unique variable names so that it is obvious what you are doing.

Uninitialized Objects

If your input is not what you expect you don't initialize the object.

string::string(const char *pcValue)
{
    if(pcValue)
    {
         // INIT CODE
    }
}
string::string(const string &strInstance)
{
    if(strInstance.empty() == false)
    {
         // INIT CODE
    }
}

Notice in both these cases there is no else. So if you don't fit the condition you do no work. Note in C++ unless you explicitly set the value it has an indeterminate value. Attempting to read an indeterminate value is Undefined Behavior. In your constructors you should explicitly try and set all members (Note: Members that have constructors will be initialized but POD objects don't have constructors int/float/pointers etc...).

Initializer List

You should prefer to use the initializer list in your constructor to set up members. In your code it does not matter because all your members are POD but it is a good habit to get into for all members because when they are not POD (or if somebody changes the type of you member to non POD) this will cause a lot of extra code. This is because the members are initialized by their constructor (if they have one) before the constructor is entered from the initializer list. If you don't explicitly put them in the list the compiler will add them using their default constructor. Subsequent assignment in the constructor code will use the assignment operator for that object.

string::string(int iSize)
    : pcString(new char[iSize])    // Initializer list
    , iCapacity(iSize)
{
    pcString[0] = '\0'; // Only setting the first char
}

Boolean Tests

If a value or expression is boolean. You don't need to test it against true/false. That's its value already.

    if(strInstance.empty() == false)

    // I would just write

    if (!strInstance.empty())   // The whole point in giving it a reasonable
                                // name like empty() is to make the code more
                                // readable in situations like this.

Destructor

string::~string(void)
{
    delete this->pcString;
}

You used the wrong delete. Because you used new [] you must use delete [] to release the memory.

string::~string(void)
{
    delete [] pcString;
}

Assignment operator

string& string::operator=(const string &strInstance)
{
    if(strInstance.empty() == false)
    {        

So if the other string is empty() you are not going to do anything. I think that is a bug. If the other string is empty then this string should also become empty.

        delete this->pcString;  // missing [] see notes on destructor
        this->pcString = new char[this->iCapacity];

You correctly delete the pcString before re-use. BUT what happens if the call to new char[] fails? This can potentially throw an exception. If sombody catches that exception further down the road then you now have an object with pcString pointing at deallocated memory. Not a good idea.

When reallocating. You first allocate the new memory (make sure it works). if it does then you can delete your old memory. It should look like this:

string& string::operator=(const string &strInstance)
{
     // Step 1:
     // Do dangerous work into temporaries.
     // If this fails and throws an exception your object is 
     // still in a good state.
     char* newPcString = new char[strInstance.iCapacity];
     std::copy(strInstance.pCString, strInstance.pCString + strInstance.iCapacity,
               newPcString);

     // Step 2:
     // Swap the temporaries and your internal state in an exception safe way
     std::swap(newPcString, pcString);
     this->iCapacity  = strInstance.iCapacity;

     // Step 3:
     // Now release the old memory. In this case it is not dangerious.
     // But if the objects in the array had destructors it could be dangerious.
     // This is why we do it after the state of the object has been completely
     // changed.
     delete [] newPcString; // Note the swap in Step 2.

     return *this;
}

OK. So that is the long way around of doing it. But there is a technique called the copy and swap idiom. That automates Step 1 and Step 3. So all you have to do is write Step 2.

string& string::operator=(string strInstance)
{
     std::swap(strInstance.pcString, pcString);
     this->iCapacity  = strInstance.iCapacity;
     return *this;
}

Where did Step 1 and 3 go? If you look at the parameter strInstance. You will notice that we passed it by value. This causes a copy of the original to be created (this is step1) and when the function exits the destructor is called and the parameter is destroyed thus invoking Step 3.

Operator +

Operator + is not doing what I expect it to.

string& string::operator+(const string &strInstance)

I would expect operator + to return a completely new object with the strings concatenated. What you have implemented is what I would expect the function operator+= to implement (because you are returning a string reference). If you want to return a new object then you should return by value (drop the &). Yes it looks inefficient to do so but the compiler will see this and perform RVO optimization to elide the copy).

If you look at this in some code.

string    bred   = "bred";  // calls the correct constructor.
string    butter = "butter"

toast(bred + butter);

std::cout << bred.data() << "\n";  // This prints "bredbutter"
                                   // This is not what I expect because
                                   // I have made no assignment to bred.


toast(bred += butter);             // Here I see an assignment so I am
                                   // OK with bred changing.

So I would change the operator to +=. Note writing operator+ in terms of operator+= is trivial.

Having a look at your code for operator+. It looks like you were actually trying to do it correctly. Couple of gotchas.

{
    int iNewSize = strInstance.size() + this->size() + 1;
    char *pcTemp = new char[iNewSize];
    // This is leaked.
    // Also efficient as you are going to call new inside the constructor.


    _snprintf(pcTemp, iNewSize, "%s%s", this->data(), strInstance.data());
    string *pStrResult = new string(pcTemp);
    // You are dynmaiclly allocating a new object here.
    // But you return by refernece.
    // So there is no where to delete this object.

    return *pStrResult;
}

// This is how I would do it.

string string::operator+(string const& strInstance)
{
    // Create a new string object with enough space.
    string    result(size() + strInstance.size() + 1);

    std::copy(pcString, pcString + size(), result.pcString);
    std::copy(strInstance.pcString, strInstance.pcString + strInstance.size()
              result.pcString + size());
    result.pcString[size() + strInstance.size()] = '\0';

    // Don't worry about returning by value.
    // NRVO will kick in and elide the copy and build this object in place
    // at the destination.
    return result;

}

\$\endgroup\$
3
  • \$\begingroup\$ But to me the most glaring shortcoming is that it doesn't implement the std::string API even anywhere near the point of usable. For example at(), operator[], begin()/end()..., c_str(), substr, copy, swap, find etc are missing. \$\endgroup\$
    – Emily L.
    Jul 28, 2015 at 16:26
  • \$\begingroup\$ Yes I agree that is an issue. But there are so many basic problems that need to be solved first. Let the OP fix the current issues which are serious bugs then we can discuss interface enhancements in a follow up question. \$\endgroup\$ Jul 28, 2015 at 16:45
  • \$\begingroup\$ Downside to using default arguments is that it makes the defaults part of the interface (they have to be in the header, and the compiler will often inline them into code compiled using them). If the library decides to change a default, code that uses the recompiled library without being itself recompiled will still be passing the old default values. By contrast, constructor delegation can be used in the .cpp file without exposing it as part of the interface; this also saves a tiny amount of work (the defaults aren't repeated at every call site). \$\endgroup\$ Jul 20, 2018 at 19:40
2
\$\begingroup\$

Other answers have handled specific issues, mine will handle larger design issues. Some of my later advice is highly opinionated, but it is motivated by real design problems; be sure to understand the problem before dismissing it.


If you don't like nasty bugs, always use size_t for allocation sizes, not int. Yes, somebody will do it, this is a fatal flaw in the Qt libraries.

You're missing move constructors/assignment. Most likely they should be implemented by moving the ownership logic out to a separate class (if you don't take all of my advice, std::unique_ptr<char[]> would work) and then using the Rule of Zero.

There's no point in providing a find method (this is a flaw of std::string), you can just use std::find on the iterators (that you need to implement).

Most implementations use char *begin, *end, *end_cap; instead of char *begin; size_t size, cap;, or even just a single string_rep *rep; member which points directly to the string data. and stores size/cap information at offset -1. Newer implementations often use SSO (Short String Optimization) as well.

You're missing slicing operations. Slicing operations should return a string_view class though, instead of allocating a string.

You're missing operator[], which most people want from a string. Though I'm not convinced it's useful; I treat strings as opaque blobs rather than containers.

If you choose to have mutable strings (see below), you're missing insert, erase, and replace. In either case, you're missing clear().

You're missing operator==, operator<, etc. These should allow mixing string with string_view.


In my experience providing operator+ or operator+= on strings is a mistake. Instead, strings should be constructed via one of:

  • a join(separator, string_array, prefix, suffix, last_separator) method.
  • a ostringstream equivalent (which, signficantly, does not need to maintain all the string invariants such as contiguity in its intermediate form).
  • a printf-like string formatting operation.

Likewise, it is exceptionally rare that allowing mutation of a string after construction/assignment is actually useful. This makes the explicit string(size_t) constructor nearly completely useless.


When I implemented my own string classes, I ended up with 10 different classes:

  • XString for a full borrowed slice, equivalent to std::string_view. This is used for most strings in function arguments.
  • ZString for a tail slice, equivalent to XString except guaranteeing that there is a trailing '\0' byte so that it can have a .c_str() method for compatibility with C APIs (including system calls).
  • LString for string literals (via UDL). Acts much like ZString except it is guaranteed to be statically allocated, so nobody needs to own it.
  • VString<n>, which stores a string of fixed maximum size in its own body. sizeof(VString<n>) == n + 1. Uses a neat trick to store the size in a way that guarantees a '\0' terminator.
  • RString, which owns a string on the heap and uses reference-counting to make copies cheap. If constructed from an LString, detects that and just stores a plain pointer and avoids the allocation and refcount steps. This is the go-to class for long-lived strings.
  • AString, which is like RString but uses SSO. Commonly used for temporary strings to avoid heap allocations, but not often used inside classes since it would often waste spaaace. Since this is opt-in, I can use a large SSO threshold (255), which is useful e.g. for the return value of my snprintf wrapper.
  • SString, an owned full slice of an RString. I ended up not using this much,.
  • TString, an owned tail slice of an RString. Like SString as ZString is to XString.
  • MString, the only class that allows mutation, and that only at the end. Does not provide the standard API that all of the above use. Implemented as a wrapper for std::deque. Often not used in favor of the below.
  • FormatString, a string literal that contains a printf format. Contains lots of nasty hacks with constexpr to let gcc -Wformat work.

All of the above, except for the last two which are special, provide exactly the same public API (except c_str()) and can be implicitly constructed from each other if it makes sense.

\$\endgroup\$
1
\$\begingroup\$

Here are some things you should consider for a std::string-like class:

  • make it a regular object (implement ==, != and strict ordering operators)
  • add support for the compact form of the for loop and standard algorithms (implement begin and end operations returning iterators)
  • Ensure getting the size is a constant-time operation
  • use std::size_t for sizes, instead of relying on int
  • do not provide free access to the data pointer to client code; This is what made the old CString class in MFC be the monster that it was (and a major source for crashing MFC programs); If you want to offer access to the pointer, at least expose it as: const char* const instead of char*
\$\endgroup\$
1
\$\begingroup\$

I'll focus on the first steps to make it closer to a regular string class. There are many more things (optimizations, etc) to do.

General comments:

  • Given your implementation, the pointer pcString cannot be NULL. We can assume it is an invariant (which is rather safe, as the user can't modify it, and a bad alloc would generate an exception). So it is possible to remove all the if(this->pcString) checks.

  • Documentation is needed, as well as adding names to parameters in the .h. For example, only looking at the interface, string(int); is unclear: is the int the new size, or a number to convert to a string?

  • I recommend putting the size type (here, int) as a typedef, and then switching to unsigned int, or, better, to std::size_t [edit: it seems keeping it a signed int may be better, see the comment], and make npos public. Motivation: you'll enforce the fact that the size cannot be negative (npos being simply a special value). If you prefer to keep int as the size type, at least check that the size is >= 0 in string::find.

  • String can be seen conceptually as a vector<char>. Having an iterator would be nice (but it can take some time to implement it, and may be outside of your scope).

By Member:

  • I'd put npos as a public member, to allow if(whole.find(part) != string::npos)

  • About string(int): I'd mark this constructor as explicit, to avoid implicit conversion (and possible nasty surprises). Adding a default parameter (char default char = '\0') would be nice.

  • string(const char*); has a bug: if pcValue == NULL, then pcString is then left uninitialized, thus accessing it later on is Undefined Behavior. If pcValue == NULL, I suggest to make an empty string similar to string(void);

  • Still in string(const char*);: memset is useless, as you do a memcpy on the whole array right after.

  • Concerning find(const char*); It should take a string, not a const char*. With only const char*, you can't do:

    string whole("abcde");
    string part("bc");
    int index = whole.find(part); // failed, because find expects a const char*,
                                  // and we have a string.
    

    Also, it should be marked as const.

  • If you define operator+ and operator=, then operator+= should be present. Tip: it's usually easier to implement operator+=, then use it to implement operator+

  • operator+ : you should return a string, not a string&. pStrResultshouldn't be allocated on the heap.

  • operator= and operator< (and all the other comparisons) would be nice.

Note:

  • If you want to see another interface for a string than than the std one, I suggest you check Java's String one, or Qt's QString.

  • I don't know the C functions very well; I suggest switching to their C++ counterparts.

\$\endgroup\$
7
  • \$\begingroup\$ The standards committee (and Bjorne) has already said it was a mistake to use unsigned int as the parameter for size. But it is too late and they will not change it now. The problem is that it does not stop you using negative numbers (because of the language auto conversion rules the negative number is converted to unsigned number that is very large). But now it is a real unsigned number and impossible to detect if it was originally a negative number. Though your allocation will probably fail at this point). \$\endgroup\$ Jul 28, 2015 at 16:19
  • 1
    \$\begingroup\$ The standard rule of thumb is use unsigned numbers for bit fields otherwise use a signed number. \$\endgroup\$ Jul 28, 2015 at 16:20
  • \$\begingroup\$ @Loki: Very interesting! It's indeed the choice made by Qt. Do you have a link about the standard committee/Bjorne declarations? Also, is the loss of the allocation range (approx. 2 millions vs 4millions on 32bits systems) considered a non-issue? I agree on the failed allocation: the problem stands out (hopefully during the development, not the production) . \$\endgroup\$
    – Armaghast
    Jul 28, 2015 at 16:38
  • \$\begingroup\$ Yes. But it will take me a while to find. It is a quote at a conference and I only have a link to the video. I'll try and find it by the end of the day. \$\endgroup\$ Jul 28, 2015 at 16:42
  • 1
    \$\begingroup\$ In this video. channel9.msdn.com/Events/GoingNative/2013/… (The panel contained many members of the committee). At the 40 minutes and 19 second mark Bjarne talks about unsigned ints. At 44 minutes 30 second mark Herb Sutter says Quote: "Unfortunately is a mistake in the STL and standard that we use unsigned integers". \$\endgroup\$ Jul 28, 2015 at 19:54
0
\$\begingroup\$

When you do pcString = new char[iSize]; maybe you should delete it before. As I see it, you do lots of new but no delete (except in the destructor).

Maybe, like the std::string, data() should return const char * and you write accessors, so users don't directly access to the pcString.

Why creating the string with a size of 15 ? Wouldn't 1 (with only the character '\0') be more self-consistent ?

The rest seems good to me.

\$\endgroup\$

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.