Problem
I wanted to create a basic String class that will hold a CString and have:
- constructor, destructor, copy constructor
- assignment operator (both from String and CString)
<<and>>operators to stream withcoutandcin==,<and>operators to compareStrings[ ]operator to access a certaincharin a stringLen()function to get the length of the stringCStr()to access the C string in theStringclass
This is really a part of my baby steps in C++, so any feedback about style, design and coding will be appreciated. Attached are both the header file (the interface, as I learned it is called) and the cpp file:
/*
string.h
Written by me on Jan 27, 2015
*/
#include <iostream>
using namespace std;
namespace exercises
{
class String;
ostream& operator<<(ostream&, const String&);
istream& operator>>(istream&, String&);
class String
{
public:
explicit String(const char* str_="");
~String();
String(String const &oStr_);
String& operator=(const String& oStr_);
String& operator=(const char* oStr_);
bool operator==(const String& oStr_)const;
bool operator<(const String& oStr_)const;
bool operator>(const String& oStr_)const;
char operator[](std::size_t)const;
char& operator[](std::size_t);
const std::size_t Len()const;
const char* CStr()const;
private:
char* m_str;
};
} //namespace exercises
/* string.cpp
Written by me on Jan 27, 2015
*/
#include <iostream>
#include <cstring>
#include "string.h"
using namespace std;
namespace exercises
{
static int BUFFSIZE = 256; // buffer size limit for streams
// ---------- C~=tor funcs ----------
String::String(const char* str_)
{
m_str = new char[strlen(str_)+1];
strcpy(m_str, str_);
}
String::~String()
{
delete[] m_str;
}
String::String(String const &oStr_)
{
m_str = new char[strlen(oStr_.m_str)+1];
strcpy(m_str, oStr_.m_str);
}
String& String::operator=(const String& oStr_)
{
char* newString = new char[strlen(oStr_.m_str)+1];
delete[] m_str;
m_str = newString;
strcpy(m_str, oStr_.m_str);
return *this;
}
String& String::operator=(const char* oCStr_)
{
char* newCStr = new char[strlen(oCStr_)+1];
delete[] m_str;
m_str = newCStr;
strcpy(m_str, oCStr_);
return *this;
}
// ---------- global funcs ----------
ostream& operator<<(ostream& os_, const String& str_)
{
return os_ << str_.CStr();
}
istream& operator>>(istream& is_, String& str_)
{
char* newCStr = new char[BUFFSIZE];
is_.get (newCStr,BUFFSIZE);
str_ = newCStr;
delete[] newCStr;
return is_;
}
// ---------- interface funcs ----------
bool String::operator==(const String& oStr_)const
{
return !strcmp(m_str, oStr_.m_str);
}
bool String::operator<(const String& oStr_)const
{
return strcmp(m_str, oStr_.m_str) < 0;
}
bool String::operator>(const String& oStr_)const
{
return strcmp(m_str, oStr_.m_str) > 0;
}
char String::operator[](std::size_t i)const
{
return *(m_str+i);
}
char& String::operator[](std::size_t i)
{
return *(m_str+i);
}
const std::size_t String::Len()const
{
return strlen(m_str);
}
const char* String::CStr()const
{
return const_cast<const char*>(m_str);
}
} // namespace exercises
Solution
Don’t use this:
using namespace std;
Your infraction here is even worse than most people.
NEVER EVER EVER put this in a header file. It not only pollutes the namespace for your code but covertly infects the code of anybody that uses your header file (you can potentially break code just because they added your headerfile).
Putting it in your source file is still pretty bad as it makes certain errors hard to spot and some types of error are hidden from you. see: Why is “using namespace std;” considered bad practice?
Prefer to use the copy and swap idiom for assignment operator:
String& String::operator=(String oStr_) // Note the pass by value generates
{ // your copy for you automatically.
std::swap(m_str, oStr_.m_str);
}
Again for assignemnt from a C-String you can use copy and swap:
String& String::operator=(const char* oCStr_)
{
String tmp(oCStr_);
std::swap(m_str, oCStr_.m_str()); // PS I hate that trailing underscore.
}
Your are trying not to tightly couple your output operator with the code (its a nice thought). But input and output are really intrinsically bound to the implementation. If you change the implementation you are going to have to make changes to these operators. So you may as well tightly couple their interface to your class (and thus give them access to your members).
class String
{
public:
..... STUFF
private:
char* m_str;
friend ostream& operator<<(ostream& os_, const String& str_)
{
return os_ << str_.m_str;
}
};
Why bother with new and delete in the same method?
char* newCStr = new char[BUFFSIZE];
// STUFF
delete[] newCStr;
Much easier to just delcare a local buffer:
char newCStr[BUFFSIZE];
// STUFF
Now you let scope take care of it. If BUFFSIZE is huge then you way want to use an object that does the memory mangement for you.
std::vector<char> newCStr(BUFFSIZE);
// STUFF
I am answering my own question just to show the ways I chose to fix and improve my code after getting feedback and some more research:
No need to count the chars twice
By writing this code:
m_str = new char[strlen(str_)+1];
strcpy(m_str, str_);
I called strlen() that moves on each character of the string while counting until reaching its end. And then, I called strcpy() which moves on each character of the string while copying until reaching its end. Can you see the similar pattern?
This should do the work better:
size_t len = strlen(str_) + 1;
m_str = new char[len];
memcpy(m_str, str_, len);
No need to repeat same code
As can be seen, the copying code repeats in the Ctor, CCtor and both assignment operators. This should have alerted me to write an implementation function:
char* String::CopyIMP(const char* str_)
{
size_t len = strlen(str_) + 1;
m_str = new char[len];
memcpy(m_str, str_, len);
return m_str;
}
Having my CopyIMP return *char has the added bonus of using it in the initialization list of the Ctor:
String::String(const char* str_): m_str(CopyIMP(str_))
{}
Next project
My next mini-project will be to optimize this class and make it a “copy on write” string. will add a link here when it is done.