Click here to Skip to main content
15,881,204 members
Please Sign up or sign in to vote.
3.00/5 (2 votes)
See more:
I've designed my own string class and I would appreciate it if the experts would give a look at it. It took me most of yesterday afternoon to write this from scratch and an hour this morning removing memory leaks. If it resembles the Standard Libray CString class in any way, I guess it means I'm on the right track.

Header file
C++
#pragma once

class cString
{
private:
	// Variables
	char * string;
	int cStringLength;
	static const int STR_MAX = 25500;
	static bool stringsJoined;

	// Encapsulated Methods
	int GetStringLength(char*) const;
	void CopyToLocalString(char*);
	void DeleteLocalString();
	bool CompareStrings(char*) const;
	char * JoinStrings(char*);

public:
	// Constructors & Deconstructor
	cString();
	cString(char*);
	virtual ~cString();

	// Operator Overloads
	void operator = (cString&);
	void operator = (char*);
	operator char*();
	bool operator == (cString&) const;
	bool operator == (char*) const;
	char * operator + (cString&);
	char * operator + (char*);
	void operator += (cString&);
	void operator += (char*);

	// Accessor methods
	char * GetString() const { return this->string; }
	int Size() const { return this->cStringLength; }
};

Source Code
C++
#include "cString.h"

bool cString::stringsJoined = false;

cString::cString()
{
	this->string = 0;
}

cString::cString(char* String)
{
	this->string = 0;
	CopyToLocalString(String);
}

cString::~cString()
{
	DeleteLocalString();
}

int cString::GetStringLength(char * String) const
{
	// local variable
	int i;

	// Loop through the string until a null is found or
	// the max limit is reached
	for ( i = 0 ; i < STR_MAX ; i++ )
		if ( String[i] == 0 )
			break;

	// Return a -1 if the operation fails
	if ( i >= STR_MAX )
		return -1;
	// Return value of i if operation succeeds
	else
		return i;
}

void cString::CopyToLocalString(char * String)
{
	// Local variables
	int sourceLength;

	// Delete local string
	this->DeleteLocalString();

	// Once local string is cleared, begin copy operations.
	// -> Determine the length of the source string
	sourceLength = this->GetStringLength(String);
	// --> Ensure operation succeeded
	if ( sourceLength < 0 )
	{
		// If length of the source string failed the size check
		// create an empty string
		this->string = new char[1];
		this->string[0] = '\0';
		return;
	}

	// If length operation returned a valid value, continue
	// -> Set the cStringLength field to sourceLength + 1
	this->cStringLength = sourceLength + 1;
	// -> Create a new char array
	this->string = new char[this->cStringLength];
	// -> Copy the source string into the new array
	for ( int i = 0 ; i < this->cStringLength ; i++ )
		this->string[i] = String[i];
	// -> Null terminate the new string.
	this->string[this->cStringLength - 1] = '\0';
}

/*  cString Method : DeleteLocalString()
	Checks to see if data is stored behind the string pointer. If data
	is found, the char array is deleted and the pointer nullified.
*/
void cString::DeleteLocalString()
{
	if (this->string != 0)
	{
		delete this->string;
		this->string = 0;
	}
}

/*  cString Method : CompareStrings(char*)
	Compares the string in the char* pointer to the string
	currently held in the local string variable
*/
bool cString::CompareStrings(char * String) const
{
	// Local variables
	int compareStringLength = GetStringLength(String) + 1;
	int comparisonTotal;

	// Compare lengths
	comparisonTotal = ( cStringLength - compareStringLength );
	if ( comparisonTotal != 0 )
		return false;

	// If strings are equal lengths, begin character by
	// character comparison
	for ( int i = 0 ; i < cStringLength ; i++ )
	{
		comparisonTotal = ( this->string[i] - String[i] );
		// If the strings are different, total will != 0;
		if ( comparisonTotal != 0 )
			return false;
	}

	return true;
}

/*  cString Method : JoinStrings(char*)
	Concaternates the characters int char* parameter with the
	characters currently stored in the local string
*/
char * cString::JoinStrings(char* String)
{
	// Local variables
	int totalLength = this->cStringLength + ( GetStringLength(String) );
	int placeHolder = 0;

	// Create a new char array that will fit both strings + null
	char * strJoin = new char[totalLength];

	// Copy first string into array
	for ( placeHolder ; placeHolder < this->cStringLength ; placeHolder++ )
		strJoin[placeHolder] = this->string[placeHolder];

	placeHolder--;

	// Copy second string into array
	for (int i = 0 ; placeHolder < totalLength ; i++, placeHolder++ )
		strJoin[placeHolder] = String[i];

	// Add null terminator
	strJoin[totalLength - 1] = '\0';

	// Return the finished string
	return strJoin;
}

/* cString Operator Assignment Overloads */
// cString to cString copy
void cString::operator = (cString& cstring)
{
	if ( this == &cstring)
		return;
	else
		CopyToLocalString(cstring.string);
}
// char array to cString copy
void cString::operator = (char * string)
{
	if ( stringsJoined == true)
	{
		CopyToLocalString(string);
		delete string;	// Removes memory leak in joining operations
		string = 0;
		stringsJoined = false;
	}
	else
		CopyToLocalString(string);
}
cString::operator char*()
{
	return this->string;
}
/* cString comparison overloads */
// compare cString to cString
bool cString::operator == (cString & cstring) const
{
	if ( this == &cstring)
		return true;
	else
		return this->CompareStrings(cstring.GetString());
}
// compare cString to char string
bool cString::operator == (char * string) const
{
	return this->CompareStrings(string);
}
/* cString addition overloads */
// return a joined string, cString to cString
char * cString::operator + (cString & cstring)
{
	stringsJoined = true;
	return JoinStrings(cstring.GetString());
}
// return a joined string, cString to char string
char * cString::operator + (char * string)
{
	stringsJoined = true;
	return JoinStrings(string);
}
/* cString add then replace overloads */
// join cString to another cString
void cString::operator += (cString & cstring)
{
	char * tempString = this->JoinStrings(cstring.GetString());
	this->CopyToLocalString(tempString);
	delete tempString;
	tempString = 0;
}
// join cString to char string
void cString::operator += (char * string)
{
	char * tempString = this->JoinStrings(string);
	this->CopyToLocalString(tempString);
	delete tempString;
	tempString = 0;
}

My own testing shows that this class acts as I intended. I'm just looking for input for stability or improvements. Also, is the commenting decent or did I overdo it?
Posted

There is a lot of problems with that code.

My first recommandation if you want to write good solid code, would be to read good books like:

- Effective C++
- More Effective C++
- Effective STL
- Exceptionnal C++

In those book, you will learn a lot and have explaination more most common mistake like those in your code.

Having said that, Here is some comment for the code. I might split that in a few solution as otherwise it could be very long...

C++
class cString
{
private:
    // Variables
    char * string;
    int cStringLength;


Well variable name string is discutable as it is widely used as a class name as in std::string so it be might be a bit confusing for people.


C++
static const int STR_MAX = 25500;


Do you really want to put a maximum length to your string class? For a general purpose class it should be able to handle anything the system can handle.


C++
static bool stringsJoined;


Using a static field won't works properly if you have many cString object instance and call the operator + on many objects.

C++
// Encapsulated Methods
int GetStringLength(char*) const;


Either that method should be static (it does not access instance data) or the argument should be removed and the function works on the instance data. Given the use of that function, the first option would be the correct one in your case.

Also the parameter should be const: int GetStringLength(const char*) const;

Also, it is recopmmanded that you give name to your parameters even in declaration. It will document your code better and some editor will also be able display the parameter name while editing code.

C++
void CopyToLocalString(char*);

Parameter should be constant

C++
void DeleteLocalString();

No comment

C++
bool CompareStrings(char*) const;
char * JoinStrings(char*);

Parameters should be constant. The second function should also be constant as the object should not be modified by this method.

C++
public:
    // Constructors & Deconstructor
    cString();
    cString(char*);
    virtual ~cString();

    // Operator will be discuted in a subsequent solution...
};

The parameter for the second constructor should be constant. That constructor should also be made explicit to avoid silent convertion: explicit cString(const char *source);

Generally for a class like this one, the destructor would not be virtual as normally we should not derive from such a class and by not being virtual, it will help performance a bit and uses might use a bit less memory (no vtable to keep).

Copy constructor is missing. Default generated copy constructor is not appropriate in this case. If an object is copied, then 2 object would reference the same string which will lead to corruption.

Assuming that the original string has already been validated, the implementation might look like:

C++
cString::cString(const cString &other) :
    cStringLength(other.cStringLength)
{
    // Since the original string has been validated and contains
    // a final '\0', this function can easily be optimized a bit...

    string = new char[cStringLength];

    // Copy could be done with memset which is more efficient
    // but for educationnal purpose, I use a loop
    for (int i = 0; i != cStringLength; ++i)
    {
        string[i] = other.string[i];
    }
}
 
Share this answer
 
v2
Comments
Foothill 24-Sep-11 20:27pm    
Just for clarification, what benifit does making the parameter 'char *' a const provide.
Philippe Mori 24-Sep-11 20:47pm    
If you have a declaration like const char *test = "hello"; and const is missing from the declaration, the compiler would complain if you try to call the function. Particulary for a class like this one, it is desirable to have const correctness.

Also, it help ensure that your code is right if by error you would try to modify the string.

Also litteral string are constant.

In the end, by being disciplined, you can avoid some subtle bugs and have more solid code.... which was your question after all.
Sergey Alexandrovich Kryukov 25-Sep-11 3:24am    
Number of great points, excellent effort and clearly explained reasons; my 5.
--SA
Now for operators...

C++
// Operator Overloads
void operator = (cString&);
void operator = (char*);
operator char*();
bool operator == (cString&) const;
bool operator == (char*) const;
char * operator + (cString&);
char * operator + (char*);
void operator += (cString&);
void operator += (char*);


Your operators should follow "standard" prototypes. The main reason is for consistency with built-in types. For example, built-in type allows chaining of =. That is:

C++
int a, b, c;
a = b = c = 5;


Thus you should have:

C++
// Operator Overloads
cString & operator = (const cString&);
cString & operator = (const char*);

bool operator == (const cString&) const;
bool operator == (const char*) const;

cString operator + (const cString&);
cString operator + (const char*);

cString& operator += (const cString&);
cString& operator += (const char*);


Operator == and operator + should be global (friend) functions and there should be another function for the case where the object on the left is a const char * and the one on the right is a cString.

operator char*();
You should avoid having a conversion operator for such class. Although is seem to make your class easier to use, it come with many pitfalls. This is the reason that class like std::string do not have it and instead provide an explicit function c_str() to get the internal string when required.

Your GetString function just do that. But it is also preferable to return a const char * to avoid external modifications. For example, you don't want your length to be wrong by someone writing a '\0' before its current end.
 
Share this answer
 
v2
Comments
Simon Bang Terkildsen 24-Sep-11 19:38pm    
+10 :) taught me something aswell
Foothill 24-Sep-11 20:23pm    
I ran into a little snag with your suggestion before I changed most of them to void. Whenever I had cString& return type and returned 'this', the program called the destructor and destroyed the cString object at the completion of the fuction, thereby deleting the string. I'll go back and take another look.
Philippe Mori 24-Sep-11 20:41pm    
Well, you first need to have a copy constructor to avoid destruction problem. And in a more optimal version (if you are using a recent compiler like VC++ 2010), you could even have a move constructor but then it is a much more advanced topic...

Anyway, if it does not work, it because there is something wrong in the code.
Philippe Mori 25-Sep-11 8:25am    
By the way, that code could be improved a lot using STL. You migth uses a std::vector<char> for the text... and STL would do memory managment and even string length computation for some operations.

That way, you could have a far more robust class with not so much effort.
Sergey Alexandrovich Kryukov 25-Sep-11 3:25am    
Same as about the other answer; my 5; adding a second answer on the same topic perfectly reasonable.
--SA
For the implementation, there is also a lot to be said... I will try to be brief and simply explain main problems.


C++
cString::cString()


That constructor set string to 0 but most of the rest of the code does not check for that case.

Also cStringLength is not initialized in that case.

C++
int cString::GetStringLength(char * String) const


The problem with this function is that it returns -1 on error but the client code does not always check it. For example JoinStrings does not properly handle that case.

C++
void cString::CopyToLocalString(char * String)


This function is not exception safe. If an exception is thrown (out of memory), then the string member would point to null.

C++
bool cString::CompareStrings(char * String) const


Since this function has 2 loops, it might be less efficient than it could be.

C++
void cString::operator = (char * string)


This function is plainly wrong... It should be somthing like:

C++
cString& operator=(const char *source)
{
    CopyToLocalString(source);  // Assume that the function is first fixed...
    return *this;
}


And in fact, using the swap trick would even be better for both assignment operators:

More C++ Idioms - Copy and swap[^]

Assignment operator[^]

Using swap to make assignment operator exception safe[^]

GotW #59[^]

Your + and += operators could be improved either for efficiency or for correctness or both. As an example both+= operators make a useless copy of the string as JoinStrings as already make a copy.
 
Share this answer
 
v2
Comments
Foothill 24-Sep-11 20:21pm    
Thank you for the input. This is a lot of information to assimilate, but you bring up valid points. Checking to make sure that there was enough memory has never crossed my mind (never made a program that big plus my comp has plenty), but it is duely noted. This has truely been a very good educational experience, plus, it's probably the best question I've asked on this site.
Philippe Mori 24-Sep-11 21:02pm    
Well for small application you might never go out of memory... but if someone would take your string class and have a few millions of string, then it might happen.

If the memory is poorly handled, then you application will probably crash because of poblems like double delete...

On the other hand, if the code is exception safe, then you would be able to eventually catch the exception and still have the application is a good state (well the command that fails would not be completed).

For GUI application most of the time an error message could be shown when such an error occurs and you can continue to use the application...

Thus making solid code help have solid application in the end.
Sergey Alexandrovich Kryukov 25-Sep-11 3:41am    
Also, good, my 5.
--SA
Simon Bang Terkildsen 25-Sep-11 9:11am    
lets make it +15, my 5 for this one too :)
The first thing that comes to my mind is No. Why? because I don't like the idea of making something that already exists.
A good developer is a lazy developer who does not make something that already exists, except for educational purposes.
 
Share this answer
 
Comments
Foothill 24-Sep-11 12:14pm    
This is for educational purposes. The more I learn, the better programmer I will be once I finish college.
Albert Holguin 24-Sep-11 12:37pm    
Good for you, great attitude... :)
Foothill 24-Sep-11 12:48pm    
I do understand that I might be re-inventing the wheel here, but I have this attitude about programming; why stand on the shoulders of giants when you can walk in their footsteps? These days, a majority of code is black boxed and neatly packaged. Figuring out how they did it is half the fun.
Simon Bang Terkildsen 24-Sep-11 13:50pm    
Figuring out how they did it is half the fun
I agree, it can be fun, but not necessary for you to become a great programmer.

Now before I start a flaming war I want to point out this is my belief everyone else is welcome to their own.

What you learn from implementing your own string class, will only make you make comfortable with the language in this case c++.
What differentiates a great programmer from an average programmer is the programmers ability to be presented with a problem and seamlessly solve it with the appropriate principles and techniques. First and foremost a great programmer has phenomenal debugging capabilities.

"A great programmer is worth ten thousand times the price of a good programmer" -- Bill Gates
Simon Bang Terkildsen 24-Sep-11 13:53pm    
Oh and to your actual question your code seems perfectly solid and good to me, though it contains way to many comments for my taste. I'm of the belief that "good" code is self-documented

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900