Overview
The main issue is to decide if the stored char buffer is '\0' terminated or not. Make it clear in comments then write tests to check for it. The first constructor does this explicitly but other constructors don't seem to (not sure if this is accident or on purpose so make it clear with comments).
Move operations. The src of the move must be in a valid state. I assume this means a nullptr in buf requires a zero size.
Move operations in general should be noexcept unless you really can't do it.
You did not use the Copy and Swap idiom. This is very standard so I would advise going to learn about it.
And one bug in the operator+().
Note:
char a[2] = {'a', 'b'}; // This is not a null terminated string.
// The compiler is not adding the '\0'
// You can do this manually
// {'a', 'b', '\0'}
// But using quotes is easier.
// Also being explicit with the size 2
// is a smell as you might count wrong.
// If you leave the brackets empty the compiler
// will do the counting for you.
// Use this:
char a[] = "ab"; // Create an array three characters long.
// a[0] = 'a'
// a[0] = 'b'
// a[0] = '\0'
Design
Your implementation only supports a size (not size and capacity). You will find that many dynamically resizeable objects contain a current size and a "maxSize" that an object can be expanded up to before reallocation.
You want to do this because memory allocation is relatively expensive so you want to minimize the number of times you do this. So you can initially allocate more than you need and then incrementally expand the object (without further dynamic allocation) until you reach the maxSize then reallocate more space.
Types are important in C++.
If you are storing size you should use std::size_t which is designed for sizes. So prefer this. For times when you want to optimize to a more efficient type, make those arguments in the comments.
Next Steps
In the standard library, strings are "Containers". There are a couple of extra things you need to do to make your class a Container.
But the first things you should do are create iterators and the begin and end methods (in all their variations - have a look at std::string).
When your class is a container you can then use range-based for:
String data("This is a string");
for (auto const& c: data) {
std::cout << "Char: " << c << "\n";
}
Code Review
Don't do this:
using namespace std;
Especially in a header file.
see: Why is "using namespace std;" considered bad practice?
You did not have your code in its own namespace.
Especially with classes that have been written before it is important to isolate your code to make sure the compiler and other users don't confuse them.
Would you not want the default constructor to hold an 'empty' string?
String::String() {
buf=nullptr;
size=0;
}
This would match the behavior of the constructor that could take a null str parameter (see next constructor).
Also a lot of code becomes less complicated if you don't allow the buffer to be nullptr. Then you don't need to check for null before you use it etc..
When writing constructor, prefer to use the initializer list. This is because if you don't the members will be default initialized (for POD this means nothing happens) but in most cases it means you will be doing extra work to default initialize the member then reset it in the body. So do it once in the initializer list.
String::String()
: buf(nullptr)
, size(0)
{}
Just make it the default way you initialize members.
This constructor:
String::String(const char* str) {
In this half you don't set the size member:
if(str==nullptr) {
str=new char[1];
str="\0";
}
First: There are already good (well optimized functions) to handle C strings. You should use those rather than write your own.
It looks like you don't allocate enough space (I think you are one short for the \0 to mark the end of the string, as you did above).
Step 1: Calculate Length:
int i=0;
const char* p=str;
while(*p++) i++;
// Better would be to use for loop then subtract pointers
// to get length.
char const* end;
for (end = str; *end; ++end) {/* No Action */}
std::size_t size = (end - str) + 1; // One for the '\0'
// Best use C functions
std::size_t size = strlen(str) + 1;
Step 2: Allocate memory
buf=new char[i]; // But not long enough. Should by i + 1
// Sure nothing wrong here.
Step 3: Copy String
int j=0;
for(j;*str;str++,j++) buf[j]=*str;
// You can declare variables inside the for statement.
for (char* dst = buf; *src; ++dst, ++src) {
*dst = *src;
}
*dst = '\0';
// But better still use C function.
strcpy(buf, src);
Step 4: Set Size:
// Why do you have a separate value?
// You could have used `size` above rather than introduce `j`.
size=j;
Again you forgot to copy the \0 at the end.
Again I assume you want the string to be null-terminated because you set it in the default constructor. But no other place.
String::String(const String &str) {
size=str.size;
buf=new char[size];
for(int i=0;i<size;i++) buf[i]=str.buf[i];
}
This is broken:
String::String(String&& str) {
buf=str.buf;
size=str.size;
str.buf=nullptr;
}
The str value has to be put into a valid state. After you have moved the value, that state is not valid (the size has the old value but the buffer is null). The state needs to be valid.
Normally, you want to make sure that move operators are noexcept. A lot of algorithms are optimized to use move operators IF they are noexcept. But these algorithms MUST fallback to copy operations if the move operator is not exception safe as otherwise they can not provide the Strong exception guarantee.
Normally the simplest way to do this is to set a default state (like the default constructor) then swap the state of the two objects (as swap is exception safe).
String::String(String&& str) noexcept
: buf(nullptr) // If you want to force null termination
, size(0) // I leave that as an exercise.
{
swap(str);
}
void String::swap(String& other) noexcept
{
std::swap(size, other.size);
std::swap(buf, other.buf);
}
friend void swap(String& lhs, String& rhs) {
lhs.swap(rhs);
}
This is useful because the swap function is used by a lot of algorithms in the standard. And it can be used by the assignment operator.
Looks good.
int String::length() const {
return size;
}
This is not the best implementation.
String& String::operator =(const String& str) {
if(this!=&str) {
if(buf) delete []buf; // Why not try and re-use?
size=str.size;
buf=new char[size];
for(int i=0;i<size;i++) buf[i]=str.buf[i];
}
}
String& String::operator =(String&& str)
{
if(this!=&str){
delete []buf;
size=str.size;
buf=str.buf;
str.buf=nullptr;
str.size=0;
}
return *this;
}
First issue that checking for self assignment is a pessimization (the extra if branch is not optimal). Yes you do have to account for self assignment. BUT this is exceedingly rare and thus optimizing for the normal case and occasionally taking a hit for self assignment is the better pattern.
Note: This is the normal logic there are situation where this does not hold. But these are by far the exception and in those specific cases you will do a check for self.
But the "standard way to implement this is to use the "Copy and Swap" idiom.
String& String::operator=(String str) noexcept
{
swap(str);
return *this;
}
You will notice that str is passed by value. You then swap this parameter with the current state and return a reference to self.
This works for both the copy and move operations.
If this is a copy assignment. Then the rhs value is copied into str (thus you get a copy like above). You then swap the state of the copied parameter with the state of the current object. Then when the function exits the parameter is destroyed (thus releasing the original buffer).
If this is a copy assignment. Then the rhs value is moved into str (thus you get a move like above). You then swap the state of the moved parameter with the state of the current object. Then when the function exits the parameter is destroyed (thus releasing the original buffer).
So:
String a("Plop1");
String b("Plop2");
a = b; // Copy assignment.
// Because you have a copy constructor.
// b copied into `str`.
// a and str swapped.
// str destroyed (releasing the original value of a)
a = std::move(b);// Move assignment.
// Because you have a move constructor.
// b moved into `str`.
// a and str swapped.
// str destroyed (releasing the original value of a)
Yes.
String::~String()
{
delete []buf;
}
Good if you want to update the string.
char& String::operator [](int pos)
{
return buf[pos];
}
But what if you passed the String as a const reference into a function (this happens a lot because of const correctness).
void debugInfo(String const& str)
{
std::cout << "Size: " << str.length() << " F: " << str[0] << "\n"
}
// Fails to compile because str[0] is not a const operation.
So modifiers like this usually have two versions. A normal version that allows mutations and a const version that allows read operations.
char& String::operator [](int pos);
char const& String::operator [](int pos) const;
Owwwwwww.
That's broken.
String& String::operator +(const String& str)
{
// Stuff.
// This string is dynamically allocated with `new`
String *s=new String(p);
// So it must be released with a call to `delete`
// But you are not returning a pointer so the user
// can not release it.
// returning a reference to a pointer.
// This is leaking.
return *s;
}
Here you can not return by reference. You have to return by value.
String String::operator +(const String& str)
{
// Stuff : create p
String result(p);
return result;
}
But normally you would implement operator+ in terms of the += operator. So I would actually do this:
String& String::operator+=(const String& str)
{
// This could be much more efficient if you
// had the concept of capacity (MaxSize) as you could
// preallocate all the space for both during construction
// of a temporary.
// Create new Value
int newSize = size + str.size;
char* newBuf = new char[newSize + 1];
strcpy(newBuf, buf);
strcat(newBuf, str.buf);
// Swap state.
swap(size, newSize);
swap(buf, newBuf);
// Release old buffer
delete [] newBuf;
return *this;
}
String String::operator+(const String& str) const
{
// This could be much more efficient if you
// had the concept of capacity (MaxSize) as you could
// preallocate all the space for both during the copy.
// so not need a reallocation.
String copy(*this); // copy the current string.
copy += str;
return copy;
}
Example:
#include <iostream>
namespace ThorsAnvil
{
class String
{
private:
std::size_t capacity; // Allocated space (all of it, including space for null)
std::size_t size; // Character in string (not null)
char* buf; // null terminated string.
public:
// Creation / Destruction
String(char const* str = nullptr);
String(String const& str);
String(String&& str) noexcept;
~String();
// Use Copy and Swap idiom.
// Handles both copy and move assignment.
String& operator=(String str) noexcept;
// Standard Func
void swap(String& other) noexcept;
// Trivial one liners put in the header.
std::ostream& print(std::ostream& stream) const {return stream << buf;}
std::size_t length() const {return size;}
std::size_t maxLenght() const {return capacity;}
char& operator[](std::size_t pos) {return buf[pos];}
char const& operator[](std::size_t pos) const {return buf[pos];}
// Manipulators
String& operator+=(String const& str);
String operator+(String const& str) const;
// Friends
friend void swap(String& lhs, String& rhs) {lhs.swap(rhs);}
friend std::ostream& operator<<(std::ostream& str, String const& data) {return data.print(str);}
private:
String(String const& str, std::size_t capacity);
static constexpr std::size_t defaultMinCapacity = 10;
static std::size_t getCapacity(char const* str);
static void copyString(char const* src, char* dst);
};
std::size_t String::getCapacity(char const* str)
{
return str == nullptr
? defaultMinCapacity
: std::max(defaultMinCapacity, (strlen(str) * 2) + 1);
}
void String::copyString(char const* src, char* dst)
{
// compensate for src being null
char const* dataSrc = src == nullptr ? "" : src;
strcpy(dst, dataSrc);
}
String::String(const char* str)
: capacity(getCapacity(str))
, size(0)
, buf(new char[capacity]{0})
{
copyString(str, buf);
size = strlen(buf);
}
String::String(const String &str)
: capacity(str.capacity)
, size(str.size)
, buf(new char[capacity]{0})
{
std::copy(str.buf, str.buf + size + 1, buf);
}
String::String(String&& str) noexcept
: capacity(defaultMinCapacity)
, size(0)
, buf(new char[capacity]{0})
{
swap(str);
}
String::String(String const& str, std::size_t capacity)
: capacity(capacity) // The burden is on the user to make sure capacity is larger than size (hence it is private)
, size(str.size)
, buf(new char[capacity]{0})
{
std::copy(str.buf, str.buf + size + 1, buf);
}
String& String::operator=(String str) noexcept
{
swap(str);
return *this;
}
String::~String()
{
delete []buf;
}
void String::swap(String& other) noexcept
{
using std::swap;
swap(capacity, other.capacity);
swap(size, other.size);
swap(buf, other.buf);
}
String& String::operator+=(String const& str)
{
// If there is not enough space
// To hold this and the extra string then reallocate.
if (size + str.size >= capacity) {
// Use private constructor that accepts a capacity.
// To create a copy of this string with enough space.
String temp(*this, (size + str.size) * 2 + 1);
// Swap the temp and this.
swap(temp);
}
// Append the new string
std::copy(str.buf, str.buf + str.size + 1, buf + size);
size += str.size;
return *this;
}
String String::operator+(String const& str) const
{
String temp(*this, (size + str.size) * 2 + 1);
return temp += str;
}
}
int main()
{
using ThorsAnvil::String;
String s1;
std::cout << s1.length() << " s1 >" << s1 << "< " << s1.maxLenght() << "\n";
char a[] = "ab";
String s2(a);
std::cout << s2.length() << " s2 >" << s2 << "< " << s2.maxLenght() << "\n";
std::string s3(a);
std::cout << s3.length() << " s3 >" << s3 << "< " << "\n";
String s4(s2);
std::cout << s4.length() << " s4 >" << s4 << "< " << s4.maxLenght() << "\n";
String s5(String("abc"));
std::cout << s5.length() << " s5 >" << s5 << "< " << s5.maxLenght() << "\n";
String s6=s5;
std::cout << s6.length() << " s6 >" << s6 << "< " << s6.maxLenght() << "\n";
String s7=String("abcd");
std::cout << s7.length() << " s7 >" << s7 << "< " << s7.maxLenght() << "\n";
char x[] = "String";
std::cout << ">" << x << "<\n";
String s8=s7+s6;
std::cout << s8.length() << " s8 >" << s8 << "< " << s8.maxLenght() << "\n";
}
