std::string implementation

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

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& str

StackExchange Code Review Q#98329, answer score: 6