6
\$\begingroup\$

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 with cout and cin
  • ==, < and > operators to compare Strings
  • [ ] operator to access a certain char in a string
  • Len() function to get the length of the string
  • CStr() to access the C string in the String class

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
\$\endgroup\$
  • \$\begingroup\$ You know there is already a standard string class? std::string \$\endgroup\$ – Martin York Jan 31 '15 at 0:14
  • \$\begingroup\$ Of course I do (this is the reason for the reinventing-the-wheel tag), but I thought it will be a good introduction to classes for me. \$\endgroup\$ – yaloner Jan 31 '15 at 8:38
5
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ If a method does new char[] and later uses delete[] on it, I believe the allocation will be forced to the heap, while if one uses char zz[BUFFSIZE] the compiler will prefer to use stack. Depending upon what the stream's "get" method requires for stack, that could be a problem. I don't think a compiler can be expected to know such things. \$\endgroup\$ – supercat Mar 19 '15 at 0:32
  • \$\begingroup\$ @supercat: Stop using the term heap/stack they are not relevant to C++ (That's a Java construct). The terms you are looking for a dynamically storage duration and automatic storage duration objects. You are correct that automatic storage duration objects can be limited in size (see compiler documentation) but it is usually very large and the compiler will generate a compile time error if you exceed it (so not worried about it) What I am worried about is memory leaks during exception handling. As an alternative to the array initially proposed a std::vector<char> could be used. \$\endgroup\$ – Martin York Mar 19 '15 at 16:31
  • \$\begingroup\$ I work largely in embedded systems, where a 1K stack would be considered "large". In such systems, having a method allocate 256 bytes of automatic storage duration on the stack might not be unreasonable if neither its caller nor any methods it calls needs very much stack, but I wouldn't expect a compiler to know when that will or won't be safe. Your point about exception-safety is a good one, though many embedded C++ systems disable exceptions anyway (since exception handling adds overhead to every function call whether or not the function can actually throw exceptions). \$\endgroup\$ – supercat Mar 19 '15 at 17:16
  • \$\begingroup\$ @supercat: Then you would not be writing a string class like the above. So the discussion is mute. \$\endgroup\$ – Martin York Mar 19 '15 at 17:19
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ you memcpy variant makes no real improvement, code will anyway be optimized by the compiler so better to put effort in making code as readable as possible. \$\endgroup\$ – Anders Feb 6 '15 at 18:50
  • \$\begingroup\$ @CyberSpock: I would not particularly expect a compiler to recognize that the same object has been given to strlen and strcpy with no actions in-between. Further, if one creates an array of length+1 bytes, copies length bytes, and zeroes the last byte, code will be safe from buffer overrun even if another thread changes the source string during the operation. \$\endgroup\$ – supercat Mar 19 '15 at 0:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.