3
\$\begingroup\$

I'm creating my own general purpose library for personal use in C++ and it needs a string class that fits in with the rest of the library. Currently, it only has a few basic features, but because strings are a pretty important thing, I want my class to be very optimized. I don't want to be writing a bunch of methods for a string model that doesn't work very well, so please leave some feedback so I can change things before it's too late.

common.h:

#ifndef _COMMON
#define _COMMON

namespace chaos {
    typedef long long llong;
    typedef unsigned char uchar;
    typedef unsigned short ushort;
    typedef unsigned int uint;
    typedef unsigned long long ulong;
    typedef signed char schar;
    typedef signed short sshort;
    typedef signed int sint;
    typedef signed long long slong;
}

#endif

string.h:

#ifndef _STRING
#define _STRING

#include "common.h"

namespace chaos {

    class String {
    private:
        char* str;
        ushort len;
    public:
        String();
        String(ushort);
        String(const char*);
        String(const String&);
        String(const String&, ushort, ushort);
        ~String();

        bool operator==(const String&);
        bool operator!=(const String&);
        String& operator=(const String&);
        String& operator+=(const String&);
        String operator+(const String&);

        char* getStr() const;
        ushort getLen() const;

        void set(ushort, char);
        char get(ushort) const;

        template<class T> static String From(T value, ushort base);
        template<class T> T to() const;
    };

}

#endif

string.cpp:

#include "string.h"

namespace chaos {

    String::String() {
        this->len = 0;
        this->str = new char[this->len + 1];
    }

    String::String(ushort length) {
        this->len = length;
        this->str = new char[this->len + 1];
        for (ushort i = 0; i < this->len; i++)
            this->str[i] = '.';
        this->str[this->len] = '\0';
    }

    String::String(const char* str) {
        for (this->len = 0; str[this->len] != '\0'; this->len++);
        this->str = new char[this->len + 1];
        for (ushort i = 0; i <= this->len; i++)
            this->str[i] = str[i];
    }

    String::String(const String& string) {
        this->len = string.len;
        this->str = new char[this->len + 1];
        for (ushort i = 0; i <= this->len; i++)
            this->str[i] = string.str[i];
    }

    String::String(const String& string, ushort offset, ushort length) {
        // TODO: out of bounds?
        this->len = length;
        this->str = new char[this->len + 1];
        for (ushort i = 0; i <= this->len; i++)
            this->str[i] = string.str[offset + i];
    }

    String::~String() {
        delete[] this->str;
    }

    bool String::operator==(const String& string) {
        if (this->len != string.len) return false;
        for (ushort i = 0; i < this->len; i++)
            if (this->str[i] != string.str[i]) return false;
        return true;
    }

    bool String::operator!=(const String& string) {
        return !(*this == string);
    }

    String& String::operator=(const String& string) {
        this->len = string.len;
        delete[] this->str;
        this->str = new char[this->len + 1];
        for (ushort i = 0; i <= this->len; i++)
            this->str[i] = string.str[i];
        return *this;
    }

    String& String::operator+=(const String& string) {
        char* str = new char[this->len + string.len + 1];
        for (ushort i = 0; i < this->len; i++)
            str[i] = this->str[i];
        for (ushort i = 0; i <= string.len; i++)
            str[this->len + i] = string.str[i];

        delete[] this->str;
        this->len = this->len + string.len;
        this->str = str;
        return *this;
    }

    String String::operator+(const String& string) {
        char* str = new char[this->len + string.len + 1];
        for (ushort i = 0; i < this->len; i++)
            str[i] = this->str[i];
        for (ushort i = 0; i <= string.len; i++)
            str[this->len + i] = string.str[i];
        return String(str);
    }

    char* String::getStr() const {
        return this->str;
    }

    unsigned short String::getLen() const {
        return this->len;
    }

    void String::set(ushort index, char value) {
        // TODO: out of bounds?
        this->str[index] = value;
    }

    char String::get(ushort index) const {
        // TODO: out of bounds?
        return this->str[index];
    }

    template<class T> String String::From(T value, ushort base) {
        // TODO: check negative base
        if (value == 0) return "0";
        ushort length = value < 0 ? 1 : 0;
        T temp = value;
        while (temp != 0) {
            length++;
            temp /= base;
        }

        String result(length);
        if (value < 0) {
            result.str[0] = '-';
            value = 0 - value;
        }
        while (value != 0) {
            ushort rem = (value % base);
            result.str[--length] = (rem <= 9) ? rem + '0' : (rem - 10) + 'a';
            value /= base;
        }
        return result;
    }

    template<class T> T String::to() const {
        T result = 0;
        bool sign = str[0] == '-';
        for (ushort i = sign; i < this->len; i++)
            result = result * 10 + str[i] - '0';
        return sign ? 0 - result : result;
    }

    template String String::From<char>(char, ushort);
    template String String::From<short>(short, ushort);
    template String String::From<int>(int, ushort);
    template String String::From<llong>(llong, ushort);
    template String String::From<uchar>(uchar, ushort);
    template String String::From<ushort>(ushort, ushort);
    template String String::From<uint>(uint, ushort);
    template String String::From<ulong>(ulong, ushort);
    template String String::From<schar>(schar, ushort);
    template String String::From<sshort>(sshort, ushort);
    template String String::From<sint>(sint, ushort);
    template String String::From<slong>(slong, ushort);

    template char String::to<char>() const;
    template short String::to<short>() const;
    template int String::to<int>() const;
    template llong String::to<llong>() const;
    template uchar String::to<uchar>() const;
    template ushort String::to<ushort>() const;
    template uint String::to<uint>() const;
    template ulong String::to<ulong>() const;
    template schar String::to<schar>() const;
    template sshort String::to<sshort>() const;
    template sint String::to<sint>() const;
    template slong String::to<slong>() const;

}
\$\endgroup\$
5
  • 2
    \$\begingroup\$ I can't stop to ask and you may answer "it's an exercise/it's for fun" but: why? Isn't std::string good enough? It also handles locale aware string comparisons... \$\endgroup\$ Commented Sep 20, 2016 at 20:47
  • \$\begingroup\$ Why not use std::string? \$\endgroup\$ Commented Sep 20, 2016 at 20:52
  • \$\begingroup\$ std::string is a fine string class, but it doesn't fit my library. My string class also has some extra features that allow it to work nicely with the rest of my library, but i removed them since they are not relevant for reviewing the string class itself. \$\endgroup\$ Commented Sep 20, 2016 at 20:55
  • 1
    \$\begingroup\$ @DutChen18: I think it would be helpful to tell us something about why your class needs to be different from std::string. It's hard to help you meet your goals when we don't know what those goals are. \$\endgroup\$ Commented Sep 20, 2016 at 21:28
  • \$\begingroup\$ @JerryCoffin my class needs to implement some extra features that std::string doesn't have in order to fit in my library. I'll give an example. My library has a Writable class, all children of Writable should implement a toString method. Writable also implements a write method that calls toString and prints it out to the screen. std::string does NOT extend Writable so you can't do std::string("Hello, World!").write();, but you can do MyString("Hello, World!").write();. This is why i need my own String class. \$\endgroup\$ Commented Sep 20, 2016 at 22:09

2 Answers 2

Reset to default
2
\$\begingroup\$

The DRY principle

Don't repeat yourself. If you find that you are writting the same piece of code again and again then you should probably pull that code into its own function.

I keep seeing this:

        this->str = new char[this->len + 1];
        for (ushort i = 0; i < this->len; i++)
            this->str[i] = /*STUFF*/;

Constructor chaining

C++ (since C++11) allows constructor chaining. This may help a lot with drying your code. Especially since most of those constructors do the same thing.

Initializing Elements.

You are obviously adding the extra byte onto your strings to make them null terminated. But you never set that value (you seem to assume that dynamically allocated memory will be zero'd. Take not it is not. The value of the memory is random and reading from it before initialization is UB.

String::String() {
    this->len = 0;
    this->str = new char[this->len + 1];  // So a size of 1!
    // But that single block of memory is not initialized. So you can't
    // assume that it contains the value '\0'
}

Use of this->

This is very uncommon in C++. I my opinion this is also bad practice as it hides real problems that would otherwise be spotted by the compiler.

The only real reason to do this is because you have a naming conflict and a local variable is shadowing a member variable. If this is true you should think of better names for your local variables. Shadowing can be detected by the compiler and thus prevent accidental assignment to the wrong variable.

On the other hand if you explicitly use this-> everywhere you don't turn on the check for shadowing (as most of your local variables now shadow members). So if you accidently forget to use this-> then you assign to the local and not the member and the compiler can not catch this type of error.

Const member functions

Members that do not mutate the state should be marked as const.

In a lot of C++ code parameters are passed as const references to a function to prevent copying. When you have a const reference to a member you are only allowed to call const member functions.

    bool String::operator==(const String& string) const {
                                                  ^^^^^

Assignment

The assignment operator should provide the strong exception guarantee. If anything fails then the object should remain unchanged. Your assignment operator fails.

    String& String::operator=(const String& string) {
        this->len = string.len;
        delete[] this->str;                    // str now points at invalid memory
        this->str = new char[this->len + 1];   // if new fails then an exception
                                               // is thrown. Leaving your object
                                               // in an invalid state if the
                                               // exception is caught.
        for (ushort i = 0; i <= this->len; i++)
            this->str[i] = string.str[i];
        return *this;
    }

The standard technique to get around this problem is called the copy and swap idiom.

String& String::operator=(String string)
{
    string.swap(*this);
    return *this;
}

I have written in detail about it in my blog.

Memory Leaking

You dynamically allocate memory in operator+ then pass it to an interface that does not accept ownership chaos::String::String(). As a result your function leaks memory.

    String String::operator+(const String& string) {
        char* str = new char[this->len + string.len + 1];  // dynamically allocated.

        // STUFF

        return String(str);  // Passed to constructor.
                             // But that constructor does not take ownership.
                             // Instead another copy is made. So this
                             // memory is leaked.
    }

Normally the operator? is implemented in terms of operator?=. It may not be the most efficient implementation but it usually works.

String String::operator+(String const& string) const
{
    String tmp(*this);
    return tmp += string;
}

This was wrong:

Explicit template expansion

    template String String::From<char>(char, ushort);
    template String String::From<short>(short, ushort);
    template String String::From<int>(int, ushort);
    template String String::From<llong>(llong, ushort);
    template String String::From<uchar>(uchar, ushort);
    template String String::From<ushort>(ushort, ushort);
    template String String::From<uint>(uint, ushort);
    template String String::From<ulong>(ulong, ushort);
    template String String::From<schar>(schar, ushort);
    template String String::From<sshort>(sshort, ushort);
    template String String::From<sint>(sint, ushort);
    template String String::From<slong>(slong, ushort);

This will not prevent other types from being valid in From. To do that you must separate the declaration and definition. See here

Underscore in Identifiers

Best to avoid. Especially if you don;t know the rules. But you have an error:

#ifndef _COMMON
#define _COMMON

The identifier _COMMON is reserved for the implementation. Using it like this is UB. The rule you are violating is an identifier that begins with an underscore and is followed by an uppercase letter is reserved in all contexts for the implementation. Note: If it is followed by a lowercase letter it is reserved in the global scope. For full details see: What are the rules about using an underscore in a C++ identifier?

\$\endgroup\$
8
  • \$\begingroup\$ wow, thanks. I thought that what i had so far wat pretty descent but it seems like i missed out on a lot of things. Currently don't have time to fix those issues, but i'll do so later. \$\endgroup\$ Commented Sep 21, 2016 at 8:08
  • \$\begingroup\$ I would also note. That there is no way you will make your private string as efficient as std::string. The best you will be able to do is equal std::string and the chances of that are very slim as it took them over a decade to get to this point. You have no ability to reserve space so you're always going to be inefficient when it comes to adding more to the string and you are missing out on all their optimizations. You should be re-using std::string not re-writting. So as @utnapistim suggests wrap the std::string in your class and just provide a thin wrapper. \$\endgroup\$ Commented Sep 21, 2016 at 11:51
  • \$\begingroup\$ @DutChen18 Note: You did do pretty well. C++ is a large complex language. You just have to learn a few more idioms and things I suggest will become natural. \$\endgroup\$ Commented Sep 21, 2016 at 11:53
  • \$\begingroup\$ Is there any other reason for not using this-> for every member? IMO it's so much easier to read and i often get confused when i don't use this-> for a member. You say "This will not prevent other types from being valid in From." but when i try to call String::From(true, 2) it says unresolved external symbol, so it does prevent other types from being valid. \$\endgroup\$ Commented Sep 22, 2016 at 11:55
  • \$\begingroup\$ @DutChen18: Apart from doing it the standard way catches more errors at compile time (rather than run time). If not using this-> confuses you this is a sure sign that you have badly named variables. If you work with other C++ developers they will also complain as this is the standard way is not to add this prefix because it catches more errors. \$\endgroup\$ Commented Sep 22, 2016 at 14:29
1
\$\begingroup\$

@Loki gave you a good response, but there's (always) more to be said (so I will add to it).

Currently, it only has a few basic features, but because strings are a pretty important thing, I want my class to be very optimized.

You are falling into a trap with this. Instead of attempting to create a String class for generic use that is "very optimized", set concrete requirements for it beforehand (e.g. "should be able to perform X instantiations in 10 seconds"; measure these on the machine you need it to work on, and optimize only when the implementation doesn't match your target; most other approaches to optimization are forms of premature optimization).

I don't want to be writing a bunch of methods for a string model that doesn't work very well, so please leave some feedback so I can change things before it's too late.

You use ushort for the size of the string. This will allow a size of max 65535 bytes in your string; Is this by design?

Consider using standard algorithms:

for (ushort i = 0; i < this->len; i++)
    this->str[i] = '.';

could be:

std::fill_n(str, len, '.');

and

bool String::operator==(const String& string) {
    if (this->len != string.len) return false;
    for (ushort i = 0; i < this->len; i++)
        if (this->str[i] != string.str[i]) return false;
    return true;
}

could be:

bool String::operator==(const String& string) {
    return (len == string.len) &&
        std::equal(str, str+len, string.str);
}

For this API:

char* getStr() const;

consider this client code:

String s{ "test" };
delete s.getStr(); // UB: new[] follwed by delete
delete []s.getStr(); // fine here, but application will crash on
                     // scope exit with UB: double delete

std::sprintf(s.getStr(), "test1"); // looks fine but buffer overflow occurs

All these can be avoided by not returning a raw pointer from your class. Consider returning a char const * const instead, and using other means to edit or set the buffer, than this API.

You say:

my class needs to implement some extra features that std::string doesn't have in order to fit in my library. I'll give an example. My library has a Writable class, all children of Writable should implement a toString method. Writable also implements a write method that calls toString and prints it out to the screen. std::string does NOT extend Writable so you can't do std::string("Hello, World!").write();, but you can do MyString("Hello, World!").write();. This is why i need my own String class.

Have you considered wrapping std::string instead? It has small string optimization (for example) which will make it more efficient for short strings than your implementation.

\$\endgroup\$
1
  • \$\begingroup\$ i have no idea how many instantiations an average string class should be able to perform, so i don't know how many should be my goal. Yes it is by design that strings can only fit a maximum of 65535 characters, I don't think i'll ever need more. You have suggested using the standard algorithms, but why? I like for loops... I haven't considered wrapping std::string, until now, it seems like a good idea but i have not yet made a final decision. Whatever the case i will still finish my own string class because it's fun. \$\endgroup\$ Commented Sep 22, 2016 at 12:05

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.