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;
}
'\0',std::stringdoes \$\endgroup\$