Good things
In case this review sounds harsh, I'll start with a couple of things I really like about this code:
- You've taken care matching
new[] with delete[] - that's easy to mess up
- You're taking care with
const (you did miss one, though - try changing B to const String and you'll spot it).
Avoid using namespace std;
Importing all names of a namespace is a bad habit to get into, and can cause surprise when names like begin and size are in the global namespace. Get used to using the namespace prefix (std is intentionally very short), or importing just the names you need into the smallest reasonable scope.
The exceptions to this rule are namespaces explicitly intended to be imported wholesale, such as the std::literals namespaces.
Use tools to help
While this program appears to run correctly, running it under Valgrind reveals several memory leaks and uses of uninitialized/unallocated data:
Invalid write of size 1
at 0x4C2FE80: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x1099A8: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:52)
by 0x109E63: main (189465.cpp:229)
Address 0x5b34c80 is 0 bytes after a block of size 0 alloc'd
at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
by 0x109E63: main (189465.cpp:229)
Invalid write of size 1
at 0x4C2FE8D: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x1099A8: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:52)
by 0x109E63: main (189465.cpp:229)
Address 0x5b34c85 is 5 bytes after a block of size 0 alloc'd
at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
by 0x109E63: main (189465.cpp:229)
...
Invalid free() / delete / delete[] / realloc()
at 0x4C2E7BB: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x109A0C: String::~String() (189465.cpp:64)
by 0x10A59C: main (189465.cpp:229)
Address 0x5b34cc0 is 0 bytes after a block of size 0 free'd
at 0x4C2E7BB: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x109A0C: String::~String() (189465.cpp:64)
by 0x10A56F: main (189465.cpp:232)
Block was alloc'd at
at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
by 0x109EB7: main (189465.cpp:230)
...
HEAP SUMMARY:
in use at exit: 100 bytes in 2 blocks
total heap usage: 7 allocs, 7 frees, 73,828 bytes allocated
0 bytes in 1 blocks are definitely lost in loss record 1 of 2
at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
by 0x109E63: main (189465.cpp:229)
100 bytes in 1 blocks are definitely lost in loss record 2 of 2
at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x109CA3: String::operator+(String const&) const (189465.cpp:162)
by 0x10A40D: main (189465.cpp:264)
LEAK SUMMARY:
definitely lost: 100 bytes in 2 blocks
indirectly lost: 0 bytes in 0 blocks
possibly lost: 0 bytes in 0 blocks
still reachable: 0 bytes in 0 blocks
suppressed: 0 bytes in 0 blocks
For counts of detected and suppressed errors, rerun with: -v
ERROR SUMMARY: 444 errors from 78 contexts (suppressed: 0 from 0)
Document the design constraint
The assignment requirements seem to require us to hold only a pointer to start of string. Compared to std::string, which also has a member to hold the length of the string, this means our objects
- will be smaller
- cannot hold a string with embedded NUL
- will have performance of O(n) rather than O(1) for any use of string length
Additionally, we do not have the std::string concept of capacity that's different from the string length, so every length-modifying operation must necessarily require allocation (well, we could shrink without reallocating, but we don't know how wasteful that might be).
If you want to improve your understanding, I recommend that you experiment further than the assignment requires - add a length member and see what you need to change to allow embedded NUL. You'll need to use std::memcpy() in place of std::strcpy() and std::strcat(), for instance.
Also, add a warning comment for the unconventional operator! that's required by the assignment - using ! to mean "size" is alien to C and C++ programmers, who expect it to be a boolean (true if the string is empty or otherwise "false" in some way).
Allocate enough memory for the NUL byte
This is one of the problems found by Valgrind, in the constructor:
data = new char[0];
data[0] = '\0';
If we want to write to data[0], we need to allocate at least 1 char (remember, the number in the the new[] expression is how many objects to allocate - the index of the last will be one less than the quantity):
data = new char[1];
data[0] = '\0';
I'd argue that it's better to use a member initializer to set data (and GCC can warn if you forget - that's helpful):
String::String()
: data(new char[1])
{
data[0] = '\0';
countA();
}
Allocate enough memory for contents
In the std::string constructor, we need to allocate enough characters for the string we're copying, including its terminating NUL:
String::String(const std::string& thing)
: data(new char[1+std::strlen(thing.c_str())])
{
std::strcpy(data, thing.c_str());
countA();
}
I haven't used thing.size() because thing might have embedded NULs, and our implementation will treat that as end of string.
Use appropriate type for sizes
A string might be longer than the range of int. The Standard provides std::size_t which will be big enough for anything that can fit in your program's address space, so we should use that for lengths, indexing and for our count of a characters.
Use the expected return types for operators
Relational operators return bool (those are already correct). Assignment operators should return a reference to the assigned-to object, and binary operators should return an instance of the common type of the arguments. That means:
String& operator = (const String& obj);
String& operator += (const String& obj);
String operator + (const String& obj) const;
Be responsible for allocated resources
This copy constructor has a serious problem:
String::String(const String &s2)
{
data = s2.data;
countA();
}
Suppose I have
{
String a = "foo";
String b = a;
}
At the end of the block, b is destructed, which will delete[] b.data. Then a is destructed, which will delete[] a.data. But a.data is the same as b.data, so we have a double delete!
What we need to do is to allocate new memory for b.data and copy the contents of a into it.
Construct from string literals
The test code provides string literals ("...") - there's no need to convert them into std::string for our constructor to just extract the character array. Instead we can construct directly from these:
String(char const*);
We can even make this act as default constructor, by providing a default argument:
String(const char * = "");
Don't provide a writeable view of our buffer
The assignment requires us to provide operator*, but we're over-generous with what we return. We can share the buffer in read-only form simply by changing its return type:
const char* operator * () const
{
return data;
}
Now, the calling code gets a pointer to constant characters - which can't be written to without a cast.
Refactor adding and subtracting the counts
The only difference between countA() and lowerNumA is whether we add to or subtract from our variable. We can make a single function to count (and it can be private, as the existing methods should have been):
std::size_t String::count_a() const
{
// know your algorithms - if we had an end iterator, we could
// simply return std::count(begin, end, 'a')
std::size_t count = 0;
for (auto p = data; *p; ++p)
count += (*p == 'a'); // no-one asked for 'A'
return count;
}
Then we can replace calls to countA() and lowerNumA with numA += count_a(); and numA -= count_a(); respectively.
Don't forget the assignment operator
If we have to write a copy constructor, that normally means we need to write something similar in operator=() - except that first we need to release our existing contents (you can copy from the destructor).
(Exercise - does it work for self-assignment? If not, can you fix it?)
Reduce duplication in + and +=
Assigning and non-assigning binary operators come as pairs. Instead of having very similar code in two places, we can implement one in terms of the other.
Here's a first attempt:
String& String::operator += (String const &obj)
{
auto prev = data;
data = new char[1+std::strlen(prev)+std::strlen(obj.data)];
std::strcpy(data, prev);
delete[] prev;
std::strcat(data, obj.data);
numA += obj.countA();
return *this;
}
// Not an efficient method, but simple and easy to get correct
// Consider implementing += as '*this = *this + other'
String String::operator + (String const &obj) const
{
String s = *this;
return s += obj;
}
(Exercise - what happens if we do a += a;? How can we fix the problem?)
We can make operator+ more efficient if we provide a constructor that takes two strings, so that it only allocates once, and implement += in terms of +:
String::String(String const &a, String const& b)
: data(new char[1+std::strlen(a.data)+std::strlen(b.data)])
{
std::strcpy(data, a.data);
std::strcat(data, b.data);
numA += countA();
}
String& String::operator += (String const &obj)
{
return *this = *this + obj;
}
String String::operator + (String const &obj) const
{
return String(*this, obj);
}
Complete the relational operators
If we have < and == operators, the rest can be defined in terms of them:
// member functions
bool String::operator < (String const &obj) const
{
return std::strcmp(data, obj.data) < 0;
}
bool String::operator == (String const &obj) const
{
return std::strcmp(data, obj.data) == 0;
}
// non-members
bool operator > (String const& a, String const& b)
{
return b < a;
}
bool operator != (String const& a, String const& b)
{
return !(a == b);
}
bool operator >= (String const& a, String const& b)
{
return !(a < b);
}
bool operator <= (String const& a, String const& b)
{
return !(b < a);
}
Alternatively, the non-members can be implemented with
using namespace std::rel_ops;
This is a namespace that's intended to be imported wholesale, unlike namespace std.
More advanced topics
Consider thread-safety
This code runs in a single thread, and that's fine. If we want to use our strings in different threads, we'll need to deal with concurrent access to the static numA variable. We could use a mutex, but this case is simple and common, so we can get all the benefit just by changing the type:
static std::atomic<std::size_t> numA;
Provide move-constructor and move-assignment
We can make code more efficient, by avoiding allocation when copying from temporary objects and other xvalue ("expiring") objects. These methods are easiest to implement in terms of swap (I recommend, but don't show, a swap() method instead of the simple std::swap() of members used here):
String::String(const String&& s2)
: String()
{
std::swap(data, s2.data);
}
String& String::operator = (String&& obj)
{
std::swap(data, obj.data);
return *this;
}
We don't need to do any allocation when we swap, and neither do we need to adjust the count, as we're just interchanging ownership of the buffers.
Further reading and exercises
A review that may be worth reading:
For more practice, in approximate order of difficulty:
- Add a member to track the length of the string, as mentioned above in the review. Use this length to correctly copy and concatenate when the string has embedded null characters (you'll need to stop using
std::strlen() and std::strcpy()/std::strcat() - you'll find std::memcpy() useful).
- Manage capacity separately from length.
- Add iterators, so we can use standard algorithms such as
std::count() (see review Example of adding STL iterator support to custom collection class for some pitfalls.
- Implement a view class, that exposes a substring of an existing string.
Modified code
#include <atomic>
#include <iostream>
#include <cstring>
class String
{
static std::atomic<std::size_t> numA;
char* data; // no embedded NULs
std::size_t countA() const;
String(String const &a, String const& b); // for operator+
public:
String(const char * = ""); // argument must not be null
String(const String&);
String(String&&);
~String();
String& operator = (String);
bool operator < (const String&) const;
bool operator == (const String&) const;
String& operator += (const String&);
String operator + (const String&) const;
std::size_t operator ! () const; // unconventional (but required by exercise)
char operator[] (std::size_t) const;
const char* operator * () const { return data; }
friend std::ostream & operator << (std::ostream&, const String&);
static std::size_t a_count() { return numA; }
};
String::String(const char *s)
: data(new char[1+std::strlen(s)])
{
std::strcpy(data, s);
numA += countA();
}
String::String(const String &s2)
: data(new char[1+std::strlen(s2.data)])
{
std::strcpy(data, s2.data);
numA += countA();
}
String::String(String&& s2)
: String()
{
std::swap(data, s2.data);
}
String::String(String const &a, String const& b)
: data(new char[1+std::strlen(a.data)+std::strlen(b.data)])
{
std::strcpy(data, a.data);
std::strcat(data, b.data);
numA += countA();
}
String::~String()
{
numA -= countA();
delete[] data;
}
std::size_t String::countA() const
{
std::size_t count = 0;
for (auto p = data; *p; ++p)
count += (*p == 'a');
return count;
}
// Group similar operators together
bool String::operator < (String const &other) const
{
return std::strcmp(data, other.data) < 0;
}
bool String::operator == (String const &other) const
{
return std::strcmp(data, other.data) == 0;
}
// non-member relational operators
// alternative: using namespace std::rel_ops;
bool operator > (String const& a, String const& b)
{
return b < a;
}
bool operator != (String const& a, String const& b)
{
return !(a == b);
}
bool operator >= (String const& a, String const& b)
{
return !(a < b);
}
bool operator <= (String const& a, String const& b)
{
return !(b < a);
}
// equivalent to destructor plus copy constructor
String& String::operator = (String other)
{
std::swap(data, other.data);
return *this;
}
String& String::operator += (String const &other)
{
return *this = *this + other;
}
String String::operator + (String const &other) const
{
return String(*this, other);
}
// N.B. this is the "length" operator, not negation
std::size_t String::operator ! () const
{
return std::strlen(data);
}
char String::operator[] (std::size_t i) const
{
return data[i]; // caller is expected to check range
}
std::atomic<std::size_t> String::numA = 0;
professor said we have to delete each individual element of an array before doing delete [] arrayanother cases where the written form had ("would have") distinct advantages, starting withstd::arrayvs.[]- neither features in the text linked. StackOverflow is not AssignmentUnload: can you turn your post into an answerable programming question? (Heed item 3:Questions asking for homework help must include a summary of the work you've done so far….) \$\endgroup\$