C++ RAII - Do I have this right?

Question

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
}

Answer 1

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.

Answer 2

Here is an attempt at an RAII class that does something similar:

template<std::size_t N>
class StringGetter_N
{
private:
  std::unique_ptr<char[]> buffer;

public:
    StringGetter_N()
    {
      buffer.reset( new char[N] );
      buffer.get()[0] = 0;
    }
    explicit StringGetter_N(char const* name)
    {
      buffer.reset( new char[N] );
      strcpy_s(buffer.get(), N-1, name);
      buffer.get()[N-1] = 0; // always null terminate
    }
    StringGetter_N( StringGetter_N&& other ) = default;

    char* GetString()
    {
        return buffer.get();
    }
};
class StringGetter : StringGetter_N<1024> {
  explicit StringGetter( const char* name ):StringGetter_N<1024>(name) {}
  StringGetter():StringGetter_N<1024>() {}
};

notice that I delegated the resource management to a smart pointer. Single responsibility principle means that the resource management object (like a smart pointer) can do just that, and if you want to write a class that represents a heap-allocated buffer of fixed size, you delegate that sub problem to the smart pointer, and just manage it in your class.

As it happens, std::unique_ptr properly implements

But really, it is usually much simpler to just use a std::vector, as you can usually determine how much space you need at run-time before needing a buffer to write to.

If you do implement your own RAII class, you should follow these rules:

• Single argument constructors that are not copy/move constructors should usually be explicit.
• If you implement a non-trivial destructor, you must (implement or block usage of) (copy and/or move constructor) and (copy and/or move operator=). This is known as the rule of 3 (or rule of 5 in C++11).
• Your RAII class should do little except manage the resource. If you want to do something else, use an RAII class to manage the resource, then store it within another class that does the fancy extra work.