I'm new to C++ and am new to using RAII for deleting allocated memory. I wrote this code as a sample of what it would take to automatically allocate and later delete a character array. I know there's a string class out there, but thought I'd start with the older stuff first. Does this sample look correct? And is it the way others would create a string and delete it automatically?
#include <iostream>
using namespace std;
class StringGetter
{
private:
char* theString;
public:
StringGetter(char* name)
{
this->theString = new char[1024];
strcpy_s(this->theString, strlen(name) + 1, name);
}
char* GetString()
{
return this->theString;
}
~StringGetter()
{
delete[] this->theString;
}
};
int main()
{
char* response;
{
StringGetter getter("testing");
response = getter.GetString();
}
cin.get(); // pauses the console window
}
It looks like you get the main idea, but there's a couple things worth mentioning.
1) As @chris mentioned, you're forgetting your copy constructor, copy assignment operator, move constructor, and move assignment operator. Copy should be either manually written or disabled, move can be defaulted. (aka You've not followed the rule of 5)
2) Prefer to use std::unique_ptr
to represent ownership. It's already done all of the hard work for you. By keeping your wrapped string in a std::unique_ptr
the default versions of the copy/move special functions will preserve correctness (though you'll still want to implement the copy operations if you want them to be enabled).
Here's what this might look like:
class StringGetter {
public:
explicit StringGetter(char* name) {
strSize = strlen(name);
str = std::unique_ptr<char[]>(new char(strSize+1));
std::copy_n(name, strSize + 1, str.get());
}
StringGetter(const StringGetter& other) {
strSize = other.strSize;
str = std::unique_ptr<char[]>(new char(strSize+1));
std::copy_n(other.str.get(), strSize + 1, str.get());
}
StringGetter(StringGetter&& other) = default;
StringGetter& operator=(const StringGetter& rhs) {
auto temp = rhs;
swap(temp);
return *this;
}
StringGetter& operator=(StringGetter&& rhs) = default;
const char* getString() const noexcept {
return str.get();
}
void swap(StringGetter& other) {
std::swap(str, other.str);
std::swap(strSize, other.strSize);
}
private:
std::unique_ptr<char[]> str;
int strSize;
};
Miscellaneous items:
1) Note that std::unique_ptr
handles the RAII. When I replace 'str' in the copy constructor, it deletes the old string automatically.
2) I size the dynamically allocated memory to match the input string. This prevents overflows/wasted memory.
3) The constructor is explicit
. This prevents undesirable conversions. Rule of thumb is to use the explicit
keyword on all single argument constructors.
4) I made getString
constant so that you can still call it on const
instances of the class.
5) I replaced the str-family copy function with std::copy_n
. It's more general and can avoid certain pitfalls in obscure cases.
6) I used the copy-swap idiom in the copy assignment operator. This promotes code reuse/avoids duplication.
7) When C++14 comes out, replace the std::unique_ptr
constructor call with std::make_unique
for added exception-safety and to remove redundancy.