6
\$\begingroup\$

I wanted to make a feature rich string in C++. Similar to one in python. I am using C++14.

I have not added much yet, I just want to get my code reviewed to ensure I am going the right way.

Features:

  • Negative indexing.(.at(-i)) (Slices coming soon)
  • Mutable string, no fixed length.
  • Can delete according to index.(.pop(i))
  • Justify text. (.rjust, .ljust, center)

Questions

  • Am I using new and delete properly?
  • Is my class structure correct?
  • Is my TString optimized well?
  • Am I missing any optimized library functions, but instead implementing myself?

tstring.h

#pragma once

#include <iostream>
#include <stdexcept>

char char_lower(const char);
char char_upper(const char);

class TString
{
private:
    char* __list;
    unsigned int __len;
public:
    TString();
    TString(const char);
    TString(const char*, unsigned int);
    TString(unsigned int, const char);
    TString(const TString&);

    ~TString();

    friend std::ostream& operator<<(std::ostream&, const TString&);

    char& operator[](unsigned int) const;

    bool operator==(const TString&) const;
    bool operator!=(const TString&) const;

    TString& operator=(const TString&);
    TString& operator+=(const TString&);
    TString& operator*=(unsigned int);

    TString operator+(const TString&) const;
    TString operator*(unsigned int) const;

    friend TString operator+(const char, const TString&);
    friend TString operator*(unsigned int, const TString&);

    unsigned int length() const;

    char& at(int) const;
    char& begin() const;
    char& end() const;

    void pop(int index);

    bool is_empty() const;

    TString lower() const;
    TString upper() const;
    TString reverse() const;

    TString ljust(unsigned int, const char) const;
    TString center(unsigned int, const char) const;
    TString rjust(unsigned int, const char) const;

    bool contains(const char) const;
    bool contains(const TString&) const;

    int index(const char) const;
    int index(const TString&) const;

    unsigned int count(const char) const;
    unsigned int count(const TString&) const;
};

str.cpp

#include "tstring.h"

char char_lower(const char char_)
{
    switch (char_)
    {
        case 'A': return 'a';
        case 'B': return 'b';
        case 'C': return 'c';
        case 'D': return 'd';
        case 'E': return 'e';
        case 'F': return 'f';
        case 'G': return 'g';
        case 'H': return 'h';
        case 'I': return 'i';
        case 'J': return 'j';
        case 'K': return 'k';
        case 'L': return 'l';
        case 'M': return 'm';
        case 'N': return 'n';
        case 'O': return 'o';
        case 'P': return 'p';
        case 'Q': return 'q';
        case 'R': return 'r';
        case 'S': return 's';
        case 'T': return 't';
        case 'U': return 'u';
        case 'V': return 'v';
        case 'W': return 'w';
        case 'X': return 'x';
        case 'Y': return 'y';
        case 'Z': return 'z';
        default: return char_;
    }
}

char char_upper(const char char_)
{
    switch (char_)
    {
        case 'a': return 'A';
        case 'b': return 'B';
        case 'c': return 'C';
        case 'd': return 'D';
        case 'e': return 'E';
        case 'f': return 'F';
        case 'g': return 'G';
        case 'h': return 'H';
        case 'i': return 'I';
        case 'j': return 'J';
        case 'k': return 'K';
        case 'l': return 'L';
        case 'm': return 'M';
        case 'n': return 'N';
        case 'o': return 'O';
        case 'p': return 'P';
        case 'q': return 'Q';
        case 'r': return 'R';
        case 's': return 'S';
        case 't': return 'T';
        case 'u': return 'U';
        case 'v': return 'V';
        case 'w': return 'W';
        case 'x': return 'X';
        case 'y': return 'Y';
        case 'z': return 'Z';
        default: return char_;
    }
}

TString::TString():
    __list{new char[0]}, __len{0}
{};

TString::TString(const char char_):
    __list{new char[1]}, __len{1}
{
    __list[0] = char_;
}

TString::TString(const char* char_array, unsigned int len):
    __list{new char[len]}, __len{len}
{
    for (int i = 0; i < len; ++i)
    {
        __list[i] = char_array[i];
    }
}

TString::TString(unsigned int len, const char char_):
    __list{new char[len]}, __len{len}
{
    for (int i = 0; i < len; ++i)
    {
        __list[i] = char_;
    }
}

TString::TString(const TString& str):
    __list{new char[str.length()]}, __len{str.length()}
{
    for (int i = 0; i < str.length(); ++i)
    {
        __list[i] = str[i];    
    }
}

TString::~TString()
{
    delete[] __list;
    delete &__len;
}

std::ostream& operator<<(std::ostream& os, const TString& str)
{
    for (int i = 0; i < str.length(); ++i)
    {
        os << str.__list[i];
    }
    return os;
}

char& TString::operator[](unsigned int index) const
{
    return __list[index];
}

bool TString::operator==(const TString& str) const
{
    if (__len != str.length())
    {
        return false;
    }
    for (int i = 0; i < str.length(); ++i)
    {
        if (__list[i] != str[i])
        {
            return false;
        }
    }
    return true;
}

bool TString::operator!=(const TString& str) const
{
    if (__len != str.length())
    {
        return true;
    }
    for (int i = 0; i < str.length(); ++i)
    {
        if (__list[i] != str[i])
        {
            return true;
        }
    }
    return false;
}

TString& TString::operator=(const TString& str)
{
    delete[] __list;
    __len = str.length();
    __list = new char[__len];
    for (int i = 0; i < __len; ++i)
    {
        __list[i] = str[i];
    }
    return *this;
}

TString& TString::operator+=(const TString& str)
{
    if (__len == 0)
    {
        *this = str;
    } else {
        char copy_list[__len];
        int copy_len = __len;
        for (int i = 0; i < copy_len; ++i)
        {
            copy_list[i] = __list[i];
        }
        delete[] __list;
        __len += str.length();
        __list = new char[__len];
        for (int i = 0; i < copy_len; ++i)
        {
            __list[i] = copy_list[i];
        }
        for (int i = 0; i < str.length(); ++i)
        {
            __list[copy_len + i] = str[i];
        }
    }
    return *this;
}

TString& TString::operator*=(unsigned int x)
{
    TString result(*this);
    if (__len == 0)
    {
        return *this;
    }
    for (int i = 1; i < x; ++i)
    {
        *this += *this;
    }
    return *this;
}

TString TString::operator+(const TString& str) const
{
    TString result(*this);
    result += str;
    return result;
}

TString TString::operator*(unsigned int x) const
{
    TString result(*this);
    result *= x;
    return result;
}

TString operator+(const char char_, const TString& str)
{
    TString result(char_);
    result += str;
    return result;
}

TString operator*(unsigned int x, const TString& str)
{
    return (str * x);
}

TString operator""_t(const char array[], unsigned int len)
{
    return TString(array, len);
}

TString operator""_t(const char char_)
{
    return TString(char_);
}

unsigned int TString::length() const
{
    return __len;
}

char& TString::at(int index) const
{
    if (index >= 0 && index < __len)
    {
        return (*this)[index];
    } else if (index < 0 && index >= -__len) {
        return (*this)[index + __len];
    }
    throw std::out_of_range("Index out of range!");
}

char& TString::begin() const
{
    if (__len == 0)
    {
        throw std::domain_error("String is empty!");
    }
    return (*this)[0];
}

char& TString::end() const
{
    if (__len == 0)
    {
        throw std::domain_error("String is empty!");
    }
    return (*this)[__len - 1];
}

void TString::pop(int index)
{
    if (index < 0)
    {
        index += __len;
    }
    char* new_list = new char[__len - 1];
    for (int i = 0; i < index; ++i)
    {
        new_list[i] = __list[i];
    }
    for (int i = index + 1; i < __len; ++i)
    {
        new_list[i - 1] = __list[i];
    }
    delete[] __list;
    __list = new_list;
}

bool TString::is_empty() const
{
    return (__len == 0);
}

TString TString::upper() const
{
    TString result(*this);
    for (int i = 0; i < __len; ++i)
    {
        result[i] = char_upper(__list[i]);
    }
    return result;
}

TString TString::lower() const
{
    TString result(*this);
    for (int i = 0; i < __len; ++i)
    {
        result[i] = char_lower(__list[i]);
    }
    return result;
}

TString TString::reverse() const
{
    TString result;
    for (int i = __len - 1; i >= 0; --i)
    {
        result += __list[i];
    }
    return result;
}

TString TString::ljust(unsigned int width, const char char_) const
{
    if (width <= __len)
    {
        return *this;
    }

    int padding = width - __len;
    return *this + TString(padding, char_);
}

TString TString::center(unsigned int width, const char char_) const
{
    if (width <= __len)
        return *this;

    int total_padding = width - __len;
    int rpad = total_padding / 2;
    int lpad = total_padding - rpad;

    return TString(rpad, char_) + *this + TString(lpad, char_);
}

TString TString::rjust(unsigned int width, const char char_) const
{
    if (width <= __len)
        return *this;

    int padding = width - __len;
    return TString(padding, char_) + *this;
}

bool TString::contains(const char char_) const
{
    for (int i = 0; i < __len; ++i)
    {
        if (__list[i] == char_)
        {
            return true;
        }
    }
    return false;
}

bool TString::contains(const TString& str) const
{
    if (*this == str)
    {
        return true;
    }
    for (int i = 0; i < __len - str.length(); ++i)
    {
        for (int j = 0; j < str.length(); ++j)
        {
            if (__list[j + i] != str[j])
            {
                break;
            } else if (j == str.length() - 1) {
                return true;
            }
        }
    }
    return false;
}

int TString::index(const char char_) const
{
    for (int i = 0; i < __len; ++i)
    {
        if (__list[i] == char_)
        {
            return i;
        }
    }
    return -1;
}

int TString::index(const TString& str) const
{
    if (*this == str)
    {
        return 0;
    }
    for (int i = 0; i < __len - str.length(); ++i)
    {
        for (int j = 0; j < str.length(); ++j)
        {
            if (__list[j + i] != str[j])
            {
                break;
            } else if (j == str.length() - 1) {
                return i;
            }
        }
    }
    return -1;
}

unsigned int TString::count(const char char_) const
{
    unsigned int count = 0;
    for (int i = 0; i < __len; ++i)
    {
        if (__list[i] == char_)
        {
            ++count;
        }
    }
    return count;
}

unsigned int TString::count(const TString& str) const
{
    unsigned int count = 0;
    if (*this == str)
    {
        ++count;
        return count;
    }
    for (int i = 0; i < __len - str.length(); ++i)
    {
        for (int j = 0; j < str.length(); ++j)
        {
            if (__list[j + i] != str[j])
            {
                break;
            } else if (j == str.length() - 1) {
                ++count;
            }
        }
    }
    return count;
}

Basic use

int main()
{
    TString hello = "Hello, World!"_t;
    std::cout << hello << '\n';
}

Thanks!

\$\endgroup\$
10
  • 3
    \$\begingroup\$ Have you run this? The destructor seems broken. \$\endgroup\$ Jan 12 at 9:58
  • 3
    \$\begingroup\$ The line I was referring to was this delete &__len;. In your basic usage, __len will be on the stack and thus delete-ing it will be undefined behavior. \$\endgroup\$ Jan 12 at 10:17
  • 2
    \$\begingroup\$ The problem is that __len did not come from new, thus it should not be deleteed. The rule of thumb is that anything that came from new or new[] must be matched by delete or delete[] respectively. Just remove that line and it should be fine. \$\endgroup\$ Jan 12 at 10:21
  • 1
    \$\begingroup\$ I would recommend starting to learn C++ from a good book instead of diving into it. A lot of the language will be mysterious and you might pick up a lot of bad habits. The language came from a time where people were expected to read quite a lot of manuals and have experience in computer architecture and OS fundamentals. Learning it from some tutorials on the internet that are not so good (they are improving though) is not a good idea if the aim is to get deep and quality knowledge. This is a good book list. \$\endgroup\$ Jan 12 at 10:23
  • 2
    \$\begingroup\$ Why build around std::vector<char> when you could just build around std::string (which is essentially just std::vector<char> that is guaranteed to be NUL-terminated)? You don't need to re-implement the memory management that std::string gives you; you're just trying to enhance the interface. As some of the answers note, current "best practices" in C++ is that these additional functions you want to add shouldn't be member functions of the class, but instead generic free functions. I have somewhat mixed opinions about that, but either way, always prefer composition when possible. \$\endgroup\$
    – Cody Gray
    Jan 13 at 6:52

5 Answers 5

17
\$\begingroup\$

Justify text. (.rjust, .ljust, center)

That is becoming a "fat interface". These functions should be generic algorithms that can work with any string class, including yours, std::string, std::string_view, and even vectors, arrays, and C-style strings. They should not be members of this class.

Am I using new and delete properly?

Probably not. See ⧺C.149 — no naked new or delete.

You ought to use std::vector for your underlying storage. Then you need only to implement the members that make it "string like" that are not available already in vector.


char char_lower(const char);
A top-level const on the parameter has no effect in this declaration. It has its normal meaning in the definition, but it's not part of the calling signature and should be omitted in the declaration.

        case 'A': return 'a';
        case 'B': return 'b';

Holy cow! Are you expecting to compile and run with a bizarre character set like EBCDIC? Even EBCDIC (famous for having non-contiguous letters) has a constant offset between the corresponding upper and lowercase letters.

char to_upper (char inchar)
{
    if (!std::isalpha(static_cast<unsigned char>(inchar))  return inchar;  // unchanged
    constexpr auto offset = 'a'-'A';
    return char(inchar + offset);
}

Of course, this only works for ASCII characters.


The constructors don't seem to match the meanings of the constructors on std::string. You should make them match, for drop-in compatibility and to avoid confusing people reading (or modifying) your code.

Of course, you can have more constructors that std::string doesn't have. But the ones with the same meaning should be the same to call.

You want to include others: convert from std::string and std::string_view, and a pair of iterators.


char& operator[](unsigned int) const;
No, that should not work. The const-ness of the container means that the elements are const. You are returning a mutable reference.

You should have

char operator[](unsigned int) const;  // read-only if const
char& operator[](unsigned int) &; // reference only on non-const lvalue
char operator[](unsigned int) &&;  // read-only for rvalue (temporary) too.

Rather than unsigned int you should define an index type as a type alias inside your class. I suggest matching that of the underlying vector.


   TString operator+(const TString&) const;
   friend TString operator+(const char, const TString&);

You should not need that second one.
By making the operator+ a member, you get asymmetric behavior. The left argument must be of that class, and will not be found via implicit conversions. You normally should write these operators as non-members so both arguments are treated the same way. A non-member operator+ will have no trouble accepting both s+'c' and 'c'+s equally well.

Normally, you implement operator+= as a member, then implement + very simply to call +=.


char& begin() const;
char& end() const;

Huh? A char& doesn't work as an iterator.
Your code compiles, right? Try using your class in a range-based for loop and see that it chokes rather than working as expected. for (char c : mystring) { ... }


I think count is another candidate for not making it a member function. It should be a generic algorithm that works with any string-like type.


   for (int i = 0; i < len; ++i)
    {
        __list[i] = char_array[i];
    }

Don't write loops when standard algorithms already exist. As Stroustrup says, "Don't write code; use algorithms". This is just copy. But, if you use vector as the backing memory, you can just defer this to the corresponding constructor of vector.

I see you only have a pointer and a length. There's no separate length for capacity. That means you'll have to re-allocate and copy every time the string changes. Using a vector will use a more performant memory management and automatically take care of the recopying when that is necessary.

You can't convert given a C-style string alone. You should default the length to use strlen automatically.


TString::TString(const TString& str):
    __list{new char[str.length()]}, __len{str.length()}
{
    for (int i = 0; i < str.length(); ++i)
    {
        __list[i] = str[i];    
    }
}

Again, outch. This is just copy. But, the work can be deferred to the underlying vector.

You ask about optimal implementation: Besides the lack of a separate capacity measure, you should have a move constructor and move assignment operator, and a swap function.


bool TString::operator==(const TString& str) const
{
    if (__len != str.length())
    {
        return false;
    }
    for (int i = 0; i < str.length(); ++i)
    {
        if (__list[i] != str[i])
        {
            return false;
        }
    }
    return true;
}

<sigh> how about

   return __len==str.length() && std::equals(list,list+__len,str.list);

Lesson:

Know what's in the Library.
Read through https://en.cppreference.com/w/cpp/algorithm


bool TString::operator!=(const TString& str) const
{
    if (__len != str.length())
    {
        return true;
    }
    for (int i = 0; i < str.length(); ++i)
    {
        if (__list[i] != str[i])
        {
            return true;
        }
    }
    return false;
}

Oh come on!!
return !(*this==str);
You should never do it any other way. (In C++20, this is automatic)


TString& TString::operator+=(const TString& str)
{
    if (__len == 0)
    {
        *this = str;
    } else {
        char copy_list[__len];  // 👈 Not Legal!
        int copy_len = __len;
        for (int i = 0; i < copy_len; ++i)
        {
            copy_list[i] = __list[i];
        }
        delete[] __list;
        __len += str.length();
        __list = new char[__len];
        for (int i = 0; i < copy_len; ++i)
        {
            __list[i] = copy_list[i];
        }
        for (int i = 0; i < str.length(); ++i)
        {
            __list[copy_len + i] = str[i];
        }
    }
    return *this;
}

First of all, the line char copy_list[__len]; is not even legal C++. A C-style VLA is a gcc extension supported in the C++ compiler.

But why are you copying twice? You copy to copy_list and then copy it again into the allocated memory. I think you are confused about the __list memory only living in that member data, rather than being a pointer you can do anything with. Create a local variable for the new data. Copy into that. Then delete __list and assign __list = new_list;.

You are also doing the copy loop again.... sure, you didn't know that there's a copy in the library already, but you already did this in your own code. Why can't you call a common place for copying the string contents? You should naturally think about making common code and not duplicating code. Maybe you've heard DRY: Don't Repeat Yourself.


TString& TString::operator*=(unsigned int x)
{
    TString result(*this);
    if (__len == 0)
    {
        return *this;
    }
    for (int i = 1; i < x; ++i)
    {
        *this += *this;
    }
    return *this;
}

Hmm, you duplicate the original string into result but then never use it! Do you have compiler warnings disabled, or are not paying attention to them? Reverse that situation: warnings are useful and informative.

I would have thought this function repeats the original string x times. But it actually doubles the original string x times. That's an odd thing to do, IMO. And you're doing it inefficiently, allocating and deallocating and re-copying x times. You should figure out the size of the result, allocate that, and copy the original 2x times into it using a loop.


char& TString::at(int index) const
{
    if (index >= 0 && index < __len)
    {
        return (*this)[index];
    } else if (index < 0 && index >= -__len) {
        return (*this)[index + __len];
    }
    throw std::out_of_range("Index out of range!");
}

Put your preconditions at the top. E.g.

   if (index > __len || index < -__len)  throw...

I can see how the "whatever's left" logic is handy in this particular case, but it's generally better to put preconditions at the top so it's worth going to a small amount of effort to arrange it that way.

You're repeating the entire indexing operation when you only changed the value of the index. Put the condition around the smallest amount of code possible rather than repeating entire calls when only one argument changed.

   if (index<0)  index += __len;
   return __list[index];

bool TString::is_empty() const
Match the member names from the std::string class.
In this case, put the implementation inline inside the class definition rather than in a separate CPP file.


TString TString::reverse() const
You don't even need that! std::reverse should work on your class. It doesn't right now because your begin/end are wonky, but when written properly you will be able to use all the library algorithms and other reusable code.

Hmm, your function returns a new reversed string, rather than reversing in-place. That's confusing, as I expected a member function to reverse in-place. Anyway, it can be implemented as follows:

TString TString::reverse() const
{
   Tstring result (*this);
   std::reverse (result.__list,result.__list+result.__len);
   return result;
}

But you can see that morally we're implementing an in-place reverse, but artificially forcing it to be called on a copy.
I'd say this is inefficient, copying twice. But it blows away your original code, which re-allocates and re-copies for every character in the string! You ask whether your code is optimal?? This is what you would call the opposite of that.


contains functions, count functions: see Lesson above.


Lesson 2

You seem to be writing every function as if it's the only code that exists in this body of work. That is, it operates on the most primitive things directly.

You should write code in hierarchical manner. Start with primitive routines that need to be implemented directly, but then other function can call the more primitive ones to do their work. This is commonly known as top-down design but when you're writing code it might be bottom-up implementation. Functions should be expressed in terms that are one level of detail deeper; not go all the way back down to the most primitive in every function.

Even if you don't know the available library functions, or are writing something where library functions don't help you, you should be doing that with your own functions! Think hierarchy, not every function stands alone.

Lesson 3

When writing a novel class that's like a common or standard class, but different in some way that warrants your own implementation, start by looking at the standard class. Look over all the members and use that as a starting point for your own. Edit it to change the things you are intentionally changing, remove the things that don't apply, and make notes where the behavior is different. Don't just make up everything from scratch. It should be easy and familiar to programmers, because they know the standard classes already.

In your case, you should understand how C++ containers work, and make sure your implementation is a valid container.

Lesson 4

"Feature Rich" might be misguided. I think you are actually meaning "fat interface", which is the antithesis of current best practices. A truly feature-rich string would refer to fundamental features of the string itself, not more functions that operate on strings.

Many of the things you want are already available, just not as members of the string class. Many are not so easy to call on a string (pre C++20). You might instead want to write a library of generic algorithms that take any string-like type as an argument, and call the standard algorithm where available and implement things where necessary. For example, you might write:

template <typename Str>
auto ljust (const Str& s, char pad, size_t width)
{
   using RetType = ret_for<Str>::type;
    ⋮
}

The RetType line is a bit of template metaprogramming that lets you map the input type to a possibly different return type. This is to allow accepting a string_view and having that return a string. You might also have a second optional template argument to allow the user to specify the return type, optionally.

If you want to be simpler and stick with standard string only, the functions that don't modify the argument but return a new string with the result should just return std::string and should accept a string_view (by value).

std::string ljust (std::string_view s, char pad, size_t width)
{
   std::string retval;
   const auto padding = s.size() >= width ? 0 : width - s.size();
   retval.reserve (s.size()+padding);
   retval.resize (padding. pad);
   retval += s;
   return s;
}

For example look at Boost.StringAlgo, though that predates string_view and even C++11, so it does not show current best practices. Point is, making a separate library of non-member functions means you can use them with existing code, and have code that uses these functions be able to call other library functions and otherwise interoperate with other modules, since it still uses standard types!

Fat interfaces are bad. A string class should have only what it needs to expose its feature set efficiently. Anything else can be written as a non-member that uses that string class, and additional functions like what you want will be just the same — you're not limited to what the class designer provided.

final words

Keep it up! Be ambitious in your projects, and keep learning. I look forward to your next effort.

\$\endgroup\$
6
  • 2
    \$\begingroup\$ Obligatory link to GotW Monoliths "Unstrung". \$\endgroup\$ Jan 12 at 19:06
  • \$\begingroup\$ From Deduplicator's link, "If that's not monolithic, it's hard to imagine what would be." Er, Qt's string class? \$\endgroup\$
    – JDługosz
    Jan 12 at 22:39
  • 1
    \$\begingroup\$ Thanks! I didn't understand much yet... Will surely follow up! \$\endgroup\$ Jan 13 at 4:43
  • \$\begingroup\$ @Random_Pythoneer59 you might want to read my article on Code Project. Follow the documentation links in the first section and bookmark those. \$\endgroup\$
    – JDługosz
    Jan 13 at 14:47
  • 1
    \$\begingroup\$ @thegreatemu Tough; that's the way it is now. Generic code needs to use the common name, not the name you wish it had. I never liked size since it's ambiguous whether it's the number of elements or the memory usage in bytes; I always used elcount. Today, empty could have the [[noignore]] attribute to avoid mistakes. But, after 35 years everyone knows what it means. \$\endgroup\$
    – JDługosz
    Jan 14 at 14:56
5
\$\begingroup\$

Names beginning with __ are reserved for the implementation to use, for any purpose. That means that macros could redefine your identifiers, to give very obscure errors. Stick to the permitted identifiers for your code.

Why are we implementing our own case-conversion functions, instead of using the properly-localised ones in <cctype>?

The header file can include <iosfwd> rather than <iostream>.

Why are we using loops like this instead of std::memcpy?:

for (int i = 0; i < len; ++i)
{
    __list[i] = char_array[i];
}

We should be providing efficient move construction and assignment, which can be noexcept (implement these in terms of swap()).

char& at(int) const;
char& begin() const;
char& end() const;

That's a violation of expectations in two ways. Firstly, begin() and end() are standard names for functions returning iterators. Secondly, if we have a const string, we shouldn't be providing a way to modify its contents. I'd expect:

char const& at(int) const;
char& at(int);

char const& front(int) const;
char& front(int);
char const& back(int) const;
char& back(int);

Destructor is attempting to delete a value that wasn't created by new:

delete &__len;

Remove that statement.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks! I will look up iterators... \$\endgroup\$ Jan 13 at 4:44
  • 1
    \$\begingroup\$ s/std::memcpy/std::copy Only use memcpy when you actually need memcpy's unique semantics (i.e., copying the entire block, one byte at a time). If memcpy is suitable, the compiler will call it when you call std::copy. If not, it will do whatever is appropriate to perform the copy. std::copy will never be slower than std::memcpy, might be faster, and will always preserve more semantic information. If you just get in the habit of using std::copy everywhere in C++, you won't have a problem, and you won't run aground accidentally trying to use std::memcpy on non-PODs. \$\endgroup\$
    – Cody Gray
    Jan 13 at 6:47
  • \$\begingroup\$ Fair point @Cody. std::memcpy() is obviously fine here for dealing with char[], but std::copy() is a good habit that's never wrong. (BTW, "one byte at a time" isn't strictly true, but I think I know what you mean) \$\endgroup\$ Jan 13 at 7:35
  • \$\begingroup\$ Re: will look up iterators You really need a proper organized book or lesson website. You might want to look for videos on STL in the major C++ conferences (like CppCON) on YouTube. If you can find a talk from Alexander Stepanov, that is great. \$\endgroup\$
    – JDługosz
    Jan 13 at 15:14
  • \$\begingroup\$ Semantically, memcpy is one byte at a time. The compiler is free to optimize it to multi-byte copies as long as it can prove that that has equivalent observable semantics to a one-byte-at-a-time copy. This is why standard library implementations that optimize it to multi-byte copies have to have leading or trailing code that handles unaligned values. Don't have a C++ spec handy, but the C(17) spec defines memcpy specifically as copying "an array of character type". It is always a contiguous, bitwise copy, which is why it copies padding bytes, v-tables, etc. \$\endgroup\$
    – Cody Gray
    Jan 13 at 22:38
4
\$\begingroup\$

A few more points:


The C++ standard library provides many helpful algorithms in the <algorithm> header which can be used to implement several of the functions here, e.g.:

  • std::copy (or std::memcpy) can be used in many places.
  • operator==() can use std::equal
  • upper() and lower() could use std::transform.
  • reverse() can use std::reverse.
  • contains(char) can use std::find.
  • contains(string) can use std::search.
  • index() can also use std::find.
  • count(char) can use std::count.
  • count(string) could use std::search.

unsigned int __len;
...
void pop(int index);

An int doesn't have the range to cover all the indices in the container (in either sign direction).

Doing index += __len in pop() may also result in overflow (e.g. with index as -1 and __len as UINT_MAX).

(Note that C++ usually uses std::size_t for container sizes and indices, which allows larger containers. If we want to access elements from the end of a container, we still use an unsigned index, but with a reverse iterator, e.g.: container.rend() + 1 gives us an iterator pointing to the penultimate item in the container.)


TString& TString::operator*=(unsigned int x)
{
    TString result(*this);
    if (__len == 0)
    {
        return *this;
    }
    for (int i = 1; i < x; ++i)
    {
        *this += *this;
    }
    return *this;
}

Seems incorrect (doesn't use result and does *this += *this;, which will grow exponentially)?


TString lower() const;
TString upper() const;
TString reverse() const;

TString ljust(unsigned int, const char) const;
TString center(unsigned int, const char) const;
TString rjust(unsigned int, const char) const;

It might be normal in Python to have a member function return a (mutated) copy of the object it's called on. In C++ that's pretty weird. We'd normally make these free functions outside the string class:

TString lower(TString const&);
TString ljust(TString const&, unsigned int, const char);

[Digression:

This makes it much easier to write generic algorithms that work for many different data types with similar interfaces. So we could write something like:

template<class T>
T lower(T const& container)
{
    auto out = T();
    out.resize(container.size());
    
    std::transform(container.begin(), container.end(), out.begin(),
        [] (char c) { return std::tolower(static_cast<unsigned char>(c)); });
    
    return out;
}

This function will work for any type T that provides a default constructor, resize() and size() functions and begin() and end() functions that return an appropriate iterator. If you look at the various C++ standard container types, you'll see they provide these functions.

(We could reduce the necessary interface further by taking only iterators as template arguments:

template<class InIt, class OutIt>
void lower(InIt in_begin, InIt in_end, OutIt out_begin)
{
    std::transform(in_begin, in_end, out_begin,
        [] (char c) { return std::tolower(static_cast<unsigned char>(c)); });
}

This would allow the function to copy and make lowercase characters from any container type to any other container type.)

Anyway, that's probably not very important for now...]

\$\endgroup\$
3
\$\begingroup\$

A few additional points to the ones people have mentioned. I agree that many of these should not be class functions, but algorithms working on generic strings or ranges. I also agree that you should model the interface after the STL.

Use Move Semantics When Appropriate

One major missed optimization is that you only ever make deep copies of the string data. If passed an temporary xvalue, a TString&&, you would be able to re-use the container. For example, upper and lower could iterate through and replace each character of the existing string, reverse could swap using a pair of iterators until they meet in the middle, a move constructor could make a shallow assignment or swap, and so on. Many of your functions would benefit.

Be Consistent About Side-Effects

Currently, you have member functions that modify the string, and other member functions that return a modified copy of the string, and there’s no consistent convention for which is which. This is likely to make people confused about whether they're modifying the object or creating a new temporary, and cause bugs.

Support std::string_view

In places lke the constructor, you currently accept a char*. A std::string_view is a very efficient format that, in much modern code, serves as a common denominator. This will automatically support any other string type that converts to one.

Support Iterators

Many, many constructors and algorithms accept an iterator pair. An iterator to contiguous memory is basically just a pointer. Implementing begin, end, rbegin and rend will let all of this code work with your class.

You should also accept an arbitrary start and end iterator in a template constructor, which would let you copy any container of the right type into a TString.

Support Localization

Your lower and upper code assumes that the current language is English. It will break, for example, in Turkish, where a capital i is İ and a lowercase I is ı. You should be using the standard library’s toupper and tolower functions.

Support Unicode

At the very minimum, a modern string class ought to support UTF-8, the default encoding for mainstream operating systems. This means you should accept char8_t data. You might also wish to support other encodings, such as wchar_t or char32_t. Most of the algorithms are independent of the character type, meaning they could be a template.

If you only support one encoding, though, it should be UTF-8.

Don’t Start Names with Double Underscores

Names like __list are reserved for the compiler, and likely to break somewhere. If you want to avoid name conflicts, put them in a namespace or class scope.

\$\endgroup\$
0
\$\begingroup\$

I see no reason for using const in either char_lower or char_upper. I would implement it much more simply:

char char_lower(char char_)
{
  if((char_>='A')&&(char_<='Z')) char_+=('a'-'A');
  return char_;
}

char char_upper(char char_)
{
  if((char_>='a')&&(char_<='z')) char_+=('A'-'a');
  return char_;
}

Also given you have operator<<, why not also operator>>?

Since no namespece was define this class shouldnt be used with embarcadero (previosly borland) as there is already a TStrings class.

\$\endgroup\$
12
  • \$\begingroup\$ What does that do to characters like } and \, which fall between A and Z in some codes (such as EBCDIC)? \$\endgroup\$ Jan 13 at 13:11
  • 1
    \$\begingroup\$ Yes, your code assumes that 'a' through 'z' is contiguous, which is famously not true in EBCDIC. \$\endgroup\$
    – JDługosz
    Jan 13 at 15:20
  • 1
    \$\begingroup\$ A const for char_ is no different than it would be for a local variable. There's nothing wrong with it and it's arguably good practice. It just shouldn't go in the declaration in the header file; only in the definition with the function body. \$\endgroup\$
    – JDługosz
    Jan 13 at 15:21
  • \$\begingroup\$ @JDługosz What that strange ASCII table? It´s defenetly nothing wrong in using const, but just not necesary in this case, as it used by value. \$\endgroup\$
    – convert
    Jan 13 at 19:31
  • \$\begingroup\$ @Toby Speight '}' and '\' are just ignored, char_lower works only for values between 65 and 90, while char_upper works only for values between 97 and 122. \$\endgroup\$
    – convert
    Jan 13 at 19:39

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.