String class

[phalaris_trip]

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

[JRM]

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

[vijayan121]

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.

[phalaris_trip]

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...