critique my string class

188

Author

April 06, 2007 02:29 PM

Hello, Community During the last few days, I've been writing a string class for c++. This is only as an experiment to see if it can be done. For me, it's kind of annoying working with stuff that I have no clue as how it works. So, I wrote this class as an experiment. Anyway, I would like some constructive criticism regarding this class.


class CODE_SYMBOL_PORT String
{
public:
	String();
	String(const String& String);
	String(const Char* String);
	//String(const Char* String, UInt32 Start, UInt32 Count);
	virtual ~String();

	Char *GetString() const;
	const UInt32 GetLength() const;
	const bool IsNull() const;
	const bool IsValidIndex(const UInt32 Index) const;

	UInt32 Compare(const String& String, bool IgnoreCase=false) const;
	bool Find(String& Str, UInt32& Pos, bool IgnoreCase=false) const;
	void Delete(UInt32 Pos, UInt32 Count);
	void Insert(UInt32 Pos, Char c);
	void Insert(UInt32 Pos, const String& String);
	void ClipLeadingWhitespace();
	void ClipTrailingWhitespace();
	void Trim();
	UInt32 ToInt() const;

	void operator =(const String& String);
	void operator =(const Char *String);
	void operator +=(const String& String);
	operator Char *() const;
	Char& String::operator [](UInt32 Pos);
	bool operator < (const String& Str) const;
	bool operator > (const String& Str) const;
	bool operator <=(const String& Str) const;
	bool operator >=(const String& Str) const;
	bool operator ==(const String& Str) const;
	bool operator !=(const String& Str) const;
	bool operator < (const Char *Chr) const;
	bool operator > (const Char *Chr) const;
	bool operator <=(const Char *Chr) const;
	bool operator >=(const Char *Chr) const;
	bool operator ==(const Char *Chr) const;
	bool operator !=(const Char *Chr) const;

	static UInt32 Length(const Char* String);

	friend String CODE_SYMBOL_PORT operator +(const String& Str1, const String& Str2);

protected:
	void Optimize();
	void Grow(UInt32 Num);

protected:
	Char *Text;
	UInt32 Size;
	UInt32 Len;
};


String::String()
{
	Text = NULL;
	Len = Size = 0;
};

String::String(const String& String)
{
	Size = String.Size;
	Len = String.Len;
	if (Size && Len)
	{
		Text = new Char[Size];
		_tcscpy(Text, String.Text);
	}
	else
	{
		Text = NULL;
	};
};

String::String(const Char *String)
{
	if (String == NULL)
	{
		Text = NULL;
		Size = Len = 0;
		return;
	};

	Size = Length(String) + 1;
	Len = Size-1;
	Text = new Char[Size];
	for (UInt32 i=0 ; i<Len ; i++)
		Text = (Char)String;
	Text[Len] = T('\0');
};

//String::String(const Char* Str, UInt32 Start, UInt32 Count)
//{
//	if (!IsValidIndex(Start) || Count <= 0)
//	{
//		Text = T("");
//		return;
//	};

//	Char *Text = new Char[Count + 1];
//	_tcsncpy(Text, &Str[Start], Count);
//	Text[Count] = T('\0');

//	return;
//};

String::~String()
{
	if (Size && Text != NULL)
		delete[] Text;
};

Char *String::GetString() const
{
	return Text;
};
const UInt32 String::GetLength() const
{
	return Len;
};
const bool String::IsNull() const
{
	return (Text == NULL || !Length(Text));
};
const bool String::IsValidIndex(const UInt32 Index) const
{
	return (Index >= 0 && Index < Len);
};

UInt32 String::Compare(const String& String, bool IgnoreCase) const
{
	if (IsNull() && !String.IsNull())
		return 1;
	else if (!IsNull() && String.IsNull())
		return -1;
	else if (IsNull() && String.IsNull())
		return 0;
	/////////????????????????????//////////
	if (IgnoreCase)
		return _tcsicmp(Text, String.Text);
	else
		return _tcscmp(Text, String.Text);
};
bool String::Find(String& Str, UInt32& Pos, bool IgnoreCase) const
{
	if (IsNull() || Str.IsNull())
		return false;

	int (* cmpfn)(const Char*, const Char*, size_t) = (IgnoreCase) ? _tcsnicmp : _tcsncmp;

	for (Pos=0; Pos <= (Len-Str.Len); Pos++)
	{
		if(IgnoreCase)
			if (_tcsnicmp(&Text[Pos], Str.Text, Str.Len) == 0)
				return true;
		else
			if (_tcsncmp(&Text[Pos], Str.Text, Str.Len) == 0)
				return true;
	};

	return false;
};
void String::Delete(UInt32 Pos, UInt32 Count)
{
	if (Pos > Len || Count==0)
		return;
	if (Pos + Count > Len)
		Count = (Len - Pos);

	for (UInt32 i=Pos ; i<Len-Count ; i++)
		Text = Text[i+Count];

	Len -= Count;
	Text[Len] = T('\0');
	Optimize();
};
void String::Insert(UInt32 Pos, Char c)
{
	if (Pos<0 || Pos>Len)
	return;

	Grow(1);

	memmove((void *)&Text[Pos+1], (const void *)&Text[Pos], Len-Pos);
	Text[Pos] = c;
	Text[++Len] = T('\0');
};
void String::Insert(UInt32 Pos, const String& String)
{
	if (Pos<0 || Pos>Len || String.IsNull())
	return;

	Char *New = new Char[String.Len + Len + 1];

	if (Pos > 0)
		_tcsncpy(New, Text, Pos);

	_tcsncpy(&New[Pos], String.Text, String.Len);

	if (Len-Pos > 0)
		_tcsncpy(&New[Pos+String.Len], &Text[Pos], Len-Pos);

	if (New == NULL)
	{
		Text = NULL;
		Size = Len = 0;
		return;
	};

	Size = Length(New) + 1;
	Len = Size-1;
	Text = new Char[Size];
	_tcscpy(Text, New);
};

void String::ClipLeadingWhitespace()
{
	if (IsNull())
		return;
	UInt32 i;
	for (i=0 ; i<Len ; i++)
	{
		if (Text != 0x20 && Text != 0x09)
		break;
	};
	Delete(0, i);
};

void String::ClipTrailingWhitespace()
{
	if (IsNull())
		return;
	UInt32 i;
	for (i=Len-1 ; i>=0 ; i--)
	{
		if (Text != 0x20 && Text != 0x09)
		break;
	};
	Delete(i+1, Len-i-1);
};

void String::Trim()
{
	ClipLeadingWhitespace();
	ClipTrailingWhitespace();
};

UInt32 String::ToInt() const
{
	if (IsNull())
		return 0;
#		if CODE_UNICODE == 1
		return _wtoi(Text);
#		else
		return atoi(Text);
#		endif
};

void String::operator=(const String& str)
{
	Size = str.Size;
	Len = str.Len;
	if (Size && Len)
	{
		Text = new Char[Size];
		_tcscpy(Text, str.Text);
	}
	else
	{
		Text = NULL;
	};
};
void String::operator=(const Char *str)
{
	Len = Length(str);
	Size = Len + 1;
	if (Size && Len)
	{
		Text = new Char[Size];
		_tcscpy(Text, str);
	}
	else
	{
		Text = NULL;
	};
};

void String::operator +=(const String& String)
{
	if (String.Len > 0)
	{
		Grow(String.Len);
		_tcsncpy(&Text[Len], String.Text, String.Len);
		Len += String.Len;
	};
};

String::operator Char *() const
{
	return Text;
};
Char& String::operator [](UInt32 Pos)
{
	if (Pos < 0)
		return Text[0];
	else if (Pos >= Len)
	{
		Grow(Pos+2);
		return Text[Pos];
	}
	else
		return Text[Pos];
};

bool String::operator < (const String& Str) const	{ return (bool)(Compare(Str) == -1);				};
bool String::operator > (const String& Str) const	{ return (bool)(Compare(Str) == 1);					};
bool String::operator <=(const String& Str) const	{ return (bool)(Compare(Str) != 1);					};
bool String::operator >=(const String& Str) const	{ return (bool)(Compare(Str) != -1);				};
bool String::operator ==(const String& Str) const	{ return (bool)(Compare(Str) == 0);					};
bool String::operator !=(const String& Str) const	{ return (bool)(Compare(Str) != 0);					};
bool String::operator < (const Char *Chr) const		{ return (bool)(Compare(String(Chr)) == -1);	};
bool String::operator > (const Char *Chr) const		{ return (bool)(Compare(String(Chr)) == 1);		};
bool String::operator <=(const Char *Chr) const		{ return (bool)(Compare(String(Chr)) != 1);		};
bool String::operator >=(const Char *Chr) const		{ return (bool)(Compare(String(Chr)) != -1);	};
bool String::operator ==(const Char *Chr) const		{ return (bool)(Compare(String(Chr)) == 0);		};
bool String::operator !=(const Char *Chr) const		{ return (bool)(Compare(String(Chr)) != 0);		};

void String::Optimize()
{
	Size = Len + 1;
	Char *Temp = new Char[Size];
	_tcscpy(Temp, Text);
	delete[] Text;
	Text = Temp;
};
void String::Grow(UInt32 Num)
{
	Size += Num;
	Char *Temp = new Char[Size];
	_tcscpy(Temp, Text);
	delete[] Text;
	Text = Temp;
};

UInt32 String::Length(const Char* String)
{
	UInt32 Length = 0;

	while(*String++)
		++Length;

	return Length;
};

String operator +(const String& Str1, const String& Str2)
{
	Char *Temp = new Char[Str1.Size + Str2.Size];
	if (Str1.Len)
		_tcscpy(Temp, Str1.Text);
	if (Str2.Len)
		_tcscpy(&Temp[Str1.Len], Str2.Text);

	String Result = Temp;
	return Result;
};

Thanks for any thoughts[smile], Chad Seibert [Edited by - Chad Seibert on April 6, 2007 3:07:05 PM]

Sneftel

1,788

April 06, 2007 02:34 PM

What's the difference between Size and Len?

Wavesonics

330

April 06, 2007 02:40 PM

I didn't look through the function implementation but if they work thats a nice set of functionality I wish was included in std::string.

From what I understand the std::string implementation uses every advanced templating feature and is something like 3k lines of code. At least thats what I've been told. So i'll bet speed wise your will be a bit worse. But over all pretty nice.

Might wanna add in like a reverseFind() as well.

Would be absolutely rock if you included other data type parsing to string in there. Overload the + operator to parse ints and floats onto it and that'd b freaking awesome.

Maybe a tokenize set of functions?
setTokenizerDelimitor( char c ) // defaults to " "
getNumTokens();
getToken( int i ); // u know the # of tokens available from the above func

Also probably want a toArray() func.

[edit] just saw u have Char *GetString() const; which does that.
Also you might want to stay away from reserved words like delete even if you are capitalizing it, just less confusing. Use erase or remove or something maybe. Just food for thought.[/edit]

But over all pretty cool man!

[edit]
Ha, ya I've gotta strongly agree with the post below mine. It's like defining a word and using that word in the definition or something... Kinda self defeating.
Unless I guess this is just kinda a wrapper to include more advanced functionality onto std:string.
[/edit]

==============================
A Developers Blog | Dark Rock Studios - My Site

Bregma

9,461

April 06, 2007 02:56 PM

Here's a few things off the top of my head.

(1) Find the invariant(s) for your class and ensure they always hold for any valid object of your class. In the case of a string class, one of those things is that the string is always a valid string of length zero or more. That means do not allow the string object to get in a state in which a function like IsNull() has any meaning.

(2) Prvide a minimal interface (as small as nececssary but no smaller). You don't need an IsValidIndex() function if you have a GetLength() function, since the a valid index of a string must be on (0, GetLength()].

(3) Technically you don't need to overload operators on const Char* since you have a converting constructor. I understand providing them as an optimization, but your implementation is just doing what the compiler-generaed functions would do.

(4) Your assignment operator leaks memory. Okay, it pours memory into the bitbucket. Consider providing a swap() nothrow() operator an implementing your assignment operator in terms of the copy constructor and swap instead. Less code to maintain, fewer leaks, improved exception safety.

(5) Your use of UInt32 (a non-standard type, but I'm guessing it's a 32-bit unsigned int type) is limiting. Why not typedef a size_type (it can be UInt32 for now) to make future changes easier? Or, you could take the attitude that nobody will need more than 640K of RAM.

(6) Consider providing a range constructor so you can play nicely with other language featiures, like the standard library.

(7) Use your Grow() member to grow the string, always. You can modify the Grow() function to use various allocaton strategies and immediately benefit.

(8) Use initializer lists in your constructors.

(9) Use the standard library where possible: std::copy() is likely to as efficient or more efficient that hand-rolling loops. It will do the right thing so you don't have to use C casts with ::memmove and you don't have to revert to non-standard non-portable functions like _tcsncpy() and the like. Just because you're writing a non-standard string class doesn't mean you have to avoid the standard library and make it non-portable.

As I said, those are just a few thoughts off the top of my head.

--smw

Stephen M. Webb
Professional Free Software Developer

Chad Seibert

188

Author

April 06, 2007 02:57 PM

Quote:Original post by Sneftel
What's the difference between Size and Len?

Size includes the null, len doesn't. Now that you mention it, it does seem to be kind of worthless, though. And I will change Delete to Remove.

Thanks,
Chad Seibert

iMalc

2,466

April 06, 2007 03:17 PM

1. You're missing the const version of operator []
2. You've implemented your comparison operators as member functions, instead of two-param friend functions, which prevents comparing a char* on the left hand side to a String oin the right-hand side.
3. Should use initialiser lists in constructor.
4. Destructor does not need to check for NULL before calling delete [].
5. Implicit conversions are very troublesome. best to remove the operator char *() and use GetString.
6. Why noy make the class use TCHARs? You do use the _tcscmp routines etc, and at the moment it will break if unicode is defined. The memmove will need *sizeof(TCHAR) too.
7. Compare preforms a few more IsNull calls than it needs to - could optimise this.

if (IsNull()) return String.IsNull() ? 0 : 1;if (String.IsNull()) return -1;

Darn I have to go, but I'm sure there's more.

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

Chad Seibert

188

Author

April 06, 2007 03:35 PM

Well, I've gotten through 2, 3, 5, 7, and 8 on Bregma's list of things. I still don't quite understand what you mean by "invariants". What do you exactly mean by a range constructor. Just operator[]? And regarding number 9, the objective is to make a portable string class, just to see if I can. And how can I create this swap nothrow operator using my copy constructor?

Thanks for all the suggestions, it's great!

[Edited by - Chad Seibert on April 6, 2007 4:35:20 PM]

Spoonbender

1,258

April 06, 2007 03:45 PM

Quote:Original post by Chad Seibert
I still don't quite understand what you mean by "invariants".

The basic rules an instance of the class must follow to be valid. (In this case, it *must* always point to a valid string of length >= 0.
Any string class that can contain an invalid string is hopelessly flawed. (and as said above, what's the point in a function testing if the string is null?)

Quote:What do you exactly mean by a range constructor.

A constructor that takes a range of characters as argument (typically in the form of start and end iterators)

Chad Seibert

188

Author

April 06, 2007 03:45 PM

As for iMalc, I've implemented 3, 4, 5, 6. Regarding 7, my Char type is typedef'd to either unsigned char or wchar_t based on whether unicode support is enabled during compilation(_UNICODE). And what do you mean I'm missing the const version of operator[]?

Sorry for all the questions, it's how I learn[smile],
Chad Seibert

Brother Bob

10,350

April 06, 2007 04:59 PM

Quote:Original post by Chad Seibert
And what do you mean I'm missing the const version of operator[]?

const String foo = "foobar";char c = foo[0];

What do you expect that to do? foo is a const object, but there is no const version of operator [], so the code above will not compile (trying to call a non-const member function on a const object).

Add a const version of the operator aswell. The code is exactly the same, just a const and a non-const version. Note that you cannot implement one with the other either.

const Char& String::operator [](UInt32 Pos) const{    ...};

... which leads me to another point. Why do you have semicolons at the end of every function definition? And on a second look, you have a semicolon after every if-statement also. Although legal, it is extremely dangerous. Remove all semicolons that aren't necessary. Additional semicolons aren't useless and can cause trouble.

critique my string class

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

critique my string class

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines