1

I have an assignment that I need to create a custom C type string class in C++. I am having trouble getting this to work. Currently my code crashes with a run time error right at the start. I also know many of my function are wrong but I want to get the member functions sorted before I go on and fix the other functions. Bear in mind that all the function prototypes were given to us and I can not change them. I need to write the 'guts' so to speak.

What is wrong with my constructor for a start?

#include <iostream> #include "tstr.h" using namespace std; //Default constructor to initialize the string to null TStr::TStr() { strPtr = 0; strSize = 0; } //constructor; conversion from the char string TStr::TStr(const char *str) { int i=0; while (str[i] != '/0') { strPtr = new char [strlen(str)+1]; for (i=0; i <strSize;++i) { strPtr[i] = str[i]; } ++i; } strSize = i; } //Copy constructor TStr::TStr(const TStr&) { } //Destructor TStr::~TStr() { if (strPtr) { delete[] strPtr; } } //subscript operators-checks for range char& TStr::operator [] (int i) { assert (i >= 0 && i < strSize); return strPtr[i]; } const char& TStr::operator [] (int i) const { assert (i >= 0 && i < strSize); return strPtr[i]; } //overload the concatenation oprerator TStr TStr::operator += (const TStr& str) { //this->strPtr += str.strPtr; //this->strSize += str.strSize; return *this; } //overload the assignment operator const TStr& TStr::operator = (const TStr& str) { if (this != &str) { delete[] strPtr; strPtr = new char[strSize = str.strSize]; assert(strPtr); for (int i=0; i<strSize; ++i) { strPtr[i] = str.strPtr[i]; } } return *this; } //overload two relational operators as member functions bool TStr::operator == (const TStr& str) const { return (strPtr == str.strPtr && strSize == str.strSize); } bool TStr::operator < (const TStr& str) const { return (strPtr < str.strPtr && strSize < str.strSize); } //the length of the string int TStr::size() { return strSize; } 

Thanks for any replies/help! :)

EDIT 1: Okay the constructor is now working but I am still getting a runtime error and I'm 90% sure it is to do with my overloaded += operator. It looks fine though and compiles okay. What am I missing?

(Note: Only small changes have been made to the above code, but let me know if you want to see the whole lot.)

//overload the concatenation oprerator TStr TStr::operator += (const TStr& str) { for(int i = 0; i < strSize; ++i) { strPtr[i] += str.strPtr[i]; } return *this; } 

EDIT 2: Okay this what I have now. Compiles fine but doesn't actually add the two strings together with the += like it should. Anyone got any ideas?

//overload the concatenation oprerator TStr TStr::operator += (const TStr& str) { char *buffer = new char[strSize + str.strSize + 1]; strcpy(buffer, strPtr); strcat(buffer, str.strPtr); delete [] strPtr; strPtr = buffer; return *this; } //overload the assignment operator const TStr& TStr::operator = (const TStr& str) { if (this != &str) { delete[] strPtr; strPtr = new char[strSize = str.strSize]; assert(strPtr); for (int i=0; i<strSize; ++i) { strPtr[i] = str.strPtr[i]; } } return *this; } 
6
  • I don't know if this is a typo, but it looks like your NULL terminator is using a forward-slash rather than the escape character. Commented Aug 24, 2011 at 21:51
  • You don't need to check if (strPtr) in your destructor. delete[]ing a NULL pointer is a nop and won't crash. Commented Aug 24, 2011 at 21:52
  • Also you probably shouldn't use assert because it's remove in release builds. Use exceptions. Commented Aug 24, 2011 at 21:53
  • Also you should make size const. Commented Aug 24, 2011 at 21:54
  • I always put in stupid mistakes like that. Thanks for pointing it out in a constructive way! :) Commented Aug 24, 2011 at 22:24

3 Answers 3

3

To summarize:

  • i is reinitialized in the pointed line
  • strSize is used uninitialized(God knows what's there) in the same line as above; should be strSize = strlen(str);
  • the string terminator chracter is wrong
  • as the man was saying it's kind of a mess

    int i=0; while (str[i] != '\0') { // as Seth pointed out it's '\0' strPtr = new char [strlen(str)+1]; for (i=0; i <strSize;++i) { // i is reinitialized here !!! strPtr[i] = str[i]; } ++i; } strSize = i; 

To be more constructive:

// as James perfectly illustrated TStr::TStr(const char *str) { int i = 0; while (str[i] != '\0') ++i; strSize = i; strPtr = new char [i+1]; while (*strPtr++ = *str++); // with a bit of k&R } //overload the concatenation oprerator TStr TStr::operator += (const TStr& str) { for(int i = 0; i < strSize; ++i) { strPtr[i] += str.strPtr[i]; } return *this; } 

Problems:

  • you want to concatenate strings meaning you need a bigger storage to hold both strings together meaning you need to reallocate your char array and you don't do that
  • you don't update the size of your string, it's bigger now isn't it?
  • strPtr[i] += str.strPtr[i]; what you're doing here is really adding integers stored on 8 bits

Solution(I'm absolutely sure it can be improved but should get you started):

//overload the concatenation oprerator TStr TStr::operator += (const TStr& str) { unsigned int i = 0; while (str.strPtr[i] != '\0') ++i; // allocate the new buffer char* newStr = new char[i + strSize + 1]; // copy the old string unsigned int j = 0; for (; j < strSize; ++j) { newStr[j] = strPtr[j]; } // update the size strSize += i; // release the old buffer delete[] strPtr; // finally concatenate char* copyPtr = newStr + j; while(*copyPtr++ = *(str.strPtr)++); // and swap the pointers strPtr = newStr; return *this; } 
Sign up to request clarification or add additional context in comments.

2 Comments

Also it's '\0', not '/0'
@Seth Carnegie completely missed that; i jumped into my eyes at first sight; corrected; thanks
2

Your ctor is pretty much a mess.

You use i for two different things -- at the same time. You also copy the entire contents of str into strPtr once for each character in str.

Basically, you have to decide, are you going to use the C run-time library or not?

Using it:

TStr::TStr(const char *str) { strSize = strlen(str); strPtr = new char [strSize+1]; strcpy(strPtr, str); } 

not using it:

TStr::TStr(const char *str) { int i = 0; while (str[i] != '\0') ++i; strSize = i; strPtr = new char [i+1]; for (i=0; i < strSize;++i) strPtr[i] = str[i]; } 

3 Comments

Thanks! I went with the second option as our lecturer is always stressing that using C type functions is bad so I assume if I can avoid the C run-time library that is a good thing.
I don't believe this code will null-terminate the new string even though you've allocated a string of size i+1 ... should the for-loop test be changed to i <= strSize so that the null-terminating character at the end of str is copied into strPtr? Or is a null-terminating character not needed?
@RedFred: your lecturer is a nut. Besides, the whole task is pretty dumb since I can see absolutely no reason why the string class of the std lib does not serve you well.
2

Why two loops one inside the other? You're thinking too hard, to copy characters from one string to another you only need one loop. Here's some code

//constructor; conversion from the char string TStr::TStr(const char *str) { strSize = strlen(str); strPtr = new char [strSize+1]; for (int i=0; i <strSize; ++i) { strPtr[i] = str[i]; } strPtr[strSize] = '\0'; } 

Much simpler!

2 Comments

You should do what James Curran suggests and use strcpy, but I wanted to show you how the loop would look it you wrote it properly.
I disagree; if you're writing your own string class, you probably want to know how to manipulate things by hand, so strcpy is taking the easy way out and shorting yourself.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.