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...
class cString
{
private:
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.
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.
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.
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.
void CopyToLocalString(char*);
Parameter should be constant
void DeleteLocalString();
No comment
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.
public:
cString();
cString(char*);
virtual ~cString();
};
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:
cString::cString(const cString &other) :
cStringLength(other.cStringLength)
{
string = new char[cStringLength];
for (int i = 0; i != cStringLength; ++i)
{
string[i] = other.string[i];
}
}