This is a past exam question that I'm doing for revision. I didn't think this was going to be as tricky as it turned out.

I would just appreciate some advice on how I can make it more elegant/professional/etc.. I'm sure this has been discussed before and there are some accepted ideas among you guys..

Anyway here is my effort:

// String
#include <iostream>
using namespace std;

class String
{
    char* str;
    int length; // Full length
    
public:
    
    // Default constructor
    String() : length(1)
    {
        // Allocate with null char
        str = new char[length];
        *str = '\0';    
    }
    
    // Overloaded constructor
    String(const char* s) : length(1)
    {
        // Get length of s
        while (s[length-1] != '\0')
            length++;

        // Allocate memory            
        str = new char[length];
        
        // Copy s to str
        for (int i=0; i<length; i++)
            str[i] = s[i];
    }
    
    // Revert to default construction
    void clear()
    {
        delete[] str;
        length = 1;  
        str = new char[length];
        *str = '\0';                             
    }
    
    // Print string
    void print() const
    {
        for (int i=0; i<length-1; i++)
            cout << str[i];
    }
    
    // Size of the string
    const int size() const
    {
        return length;
    }
    
    // Single char accessor
    const char operator[] (const unsigned int i) const
    {
        // Prevent invalid access requests
        return (i < length-1) ? str[i] : '\0';
    }

    // Assignment using const char
    String& operator= (const char* rhs)
    {
        // Clear *this
        this->clear();
        delete[] str;
        
        // Get length of rhs
        while (rhs[length-1] != '\0')
            length++;

        // Allocate memory            
        str = new char[length];
        
        for (int i=0; i<length; i++)
            str[i] = rhs[i];
        
        return *this;
    }   
    
    // Assignment using String
    String& operator= (const String& rhs)
    {       
        // Clear *this
        this->clear(); 
        
        // Allocate for new string
        length = rhs.size();
        str = new char[length];
        
        // Copy rhs to *this
        for (int i=0; i<length; i++)
            str[i] = rhs[i];
        
        return *this;      
    }
    
    // Concatenation
    String& operator+ (const String& rhs)
    {
        // Remove the null char
        char* p = &str[length-1];
        delete p; 
        
        // Concatenate *this and rhs
        p = &str[length-1];
        for (int i=0; i<rhs.size(); i++)
            *p++ = rhs[i];       
        
        // Update new length
        length += (rhs.size()-1);
        
        return *this;
    }
    
    ~String()
    {
        this->clear();
        delete[] str;
    }
};

int main(void)
{
    String p;
    p = "hello";
    p.print();
    cin.get();
    
    String q("world");
    q.print();
    cin.get();   
    
    String r = p + q;
    r.print();
    cin.get();
    
    return 0;
}

edit: took out some debugging messiness and put in comments

The only thing would suggest is to lose the
namespace directive and just list what you use
using std::cin

here are a few observations for starters.
a. your assignment operator does not take care of self-assignment. try running

// ...
void do_assign( String& a, const String& b ) { a = b ; }

int main()
{
  String str( "hello world" ) ;
  do_assign( str, str ) ;
}

b. you definitely do require a non-trivial copy constructor. (hint: this is almost always the case if you have non-trivial assignment and a non-trivial destructor.)

c. principle of least surprise: if you define an = operator and a + operator, you should also define a += operator.

d. calling clear in the destructor is pointless. no one is going to use the String after it is destroyed.

Big thanks for the replies. Will definitely simplify the destructor, and also I noticed my operator+ should be returning a value and not a reference. That's a big mistake..

I should have mentioned, this is a 30 minute question, so I can only really stick to the bare bones...

This article has been dead for over six months. Start a new discussion instead.