Writing the Rule of Three for a 2D map in C++

Question

I am making a program that determines if a tweet is happy or sad, and I was thinking I tokenize the tweets, then create a map that stores the word as the key, how many times it was used in total, and how many times it was used in a happy tweet and a sad tweet.

I think this is a good way to go about it, although there may be a better alternative, but I cannot quite understand how to write the Rule of Three for the map I want to create. The Classifier object will be the object used to classify the sentiment of the tweets, and below is the CLassifier.h file, not including additional methods for training data (the DSString object is a character array)

class Classifier {
private:
  map<DSString, map<char, float>> *words;

public:
  Classifier();
  
  Classifier(const DSString& objToCopy);  // Copy Constructor
  ~Classifier();  // Destructor
  Classifier &operator=(const Classifier& objToCopy);  // Copy Assignment Overload
}

Below is the CLassifier.cpp file that does not include code for the methods used for training and predicting.

using namespace std;

Classifier::Classifier() {
  map<DSString, map<char, float>> *words;
}

// 1/3 Copy Constructor
Classifier::Classifier(const Classifier& objToCopy) {
  words = new map<DSString, map<char, float>>(*objToCopy.words);
}

// 2/3 Destructor
Classifier::~Classifier() {
  delete words;
  words = nullptr;
}

// 3/3 Copy Assignment Operator Overload
Classifier& Classifier::operator=(const Classifier& objToCopy) {
  if (this != &objToCopy) {
    delete words;
    words = new map<DSString, map<char, float>>(*objToCopy.words);
  }
  return *this;
}

When I try to compile the program, I get an error about my copy constructor saying the following:

Classifier.cpp:15:51: error: definition of implicitly-declared ‘constexpr Classifier::Classifier(const Classifier&)’
   15 | Classifier::Classifier(const Classifier& objToCopy) {

What am I doing wrong that I cannot get my program to compile? If I am using maps incorrectly, what can I change to make the program still function? Is there a better/more efficient way to do this? I need to scan a CSV of about 20k tweets.

Answer 1

You have an error in your header file. You used the wrong class name in the copy constructor. You have this:

Classifier(const DSString& objToCopy);  // Copy Constructor

It should be this:

Classifier(const Classifier& objToCopy);  // Copy Constructor

Answer 2

Here's your code rewritten without the pointer (and with the bugs fixed)

class Classifier {
private:
  map<DSString, map<char, float>> words;

public:
};

This is known as the rule of zero because you don't need to write the destructor, copy constructor or copy assignment operator. The compiler generated ones are correct.

Answer 3

Your copy constructor is declared incorrectly. It is taking a DSString object as input, but it needs to take a Classifier object instead.

Also, your default constructor is not initializing the words member at all, which causes the rest of the code to have undefined behavior whenever it tries to act on the member.

Also, there is no need for your copy assignment operator to allocate a completely new std::map, it should just copy the data from the source std::map to the existing std::map. std::maps own copy assignment operator will free the old data and copy the source data for you.

A proper Rule-of-3 implementation would look more like this:

#include <map>

class Classifier {
private:
  using WordMap = std::map<DSString, std::map<char, float>>;
  WordMap *words;

public:
  Classifier();
  
  Classifier(const Classifier& objToCopy);  // Copy Constructor
  ~Classifier();  // Destructor
  Classifier &operator=(const Classifier& objToCopy);  // Copy Assignment Overload
};

#include "Classifier.h"

Classifier::Classifier() {
  words = new WordMap();
}

Classifier::Classifier(const Classifier& objToCopy) {
  words = new WordMap(*objToCopy.words);
}

Classifier::~Classifier() {
  delete words;
}

Classifier& Classifier::operator=(const Classifier& objToCopy) {
  if (this != &objToCopy) {
    *words = *objToCopy.words;
  }
  return *this;
}

In C++11 and later, you should implement the Rule-of-5 as well, by adding a move constructor and move assignment operator, eg:

#include <map>

class Classifier {
private:
  using WordMap = std::map<DSString, std::map<char, float>>;
  WordMap *words;

public:
  Classifier();
  
  Classifier(const Classifier& objToCopy);  // Copy Constructor
  Classifier(Classifier&& objToMove);  // Move Constructor
  ~Classifier();  // Destructor
  Classifier &operator=(const Classifier& objToCopy);  // Copy Assignment Overload
  Classifier &operator=(Classifier&& objToMove);  // Move Assignment Overload
};

#include "Classifier.h"
#include <utility>

Classifier::Classifier() {
  words = new WordMap();
}

Classifier::Classifier(const Classifier& objToCopy) {
  words = new WordMap(*objToCopy.words);
}

Classifier::Classifier(Classifier&& objToMove) {
  words = new WordMap(std::move(*objToMove.words));
}

Classifier::~Classifier() {
  delete words;
}

Classifier& Classifier::operator=(const Classifier& objToCopy) {
  if (this != &objToCopy) {
    *words = *objToCopy.words;
  }
  return *this;
}

Classifier& Classifier::operator=(Classifier&& objToMove) {
  *words = std::move(*objToMove.words);
  return *this;
}

Alternatively:

#include <map>

class Classifier {
private:
  using WordMap = std::map<DSString, std::map<char, float>>;
  WordMap *words;

public:
  Classifier();
  
  Classifier(const Classifier& objToCopy);  // Copy Constructor
  Classifier(Classifier&& objToMove);  // Move Constructor
  ~Classifier();  // Destructor
  Classifier &operator=(const Classifier& objToCopy);  // Copy Assignment Overload
  Classifier &operator=(Classifier&& objToMove);  // Move Assignment Overload

  void swap(Classifier& objToSwap);
};

#include "Classifier.h"
#include <utility>

Classifier::Classifier() {
  words = nullptr;
}

Classifier::Classifier(const Classifier& objToCopy) : Classifier() {
  if (objToCopy.words)
    words = new WordMap(*objToCopy.words);
}

Classifier::Classifier(Classifier&& objToMove) : Classifier() {
  objToMove.swap(*this);
}

Classifier::~Classifier() {
  delete words;
}

Classifier& Classifier::operator=(const Classifier& objToCopy) {
  if (this != &objToCopy) {
    Classifier(objToCopy).swap(*this);
  }
  return *this;
}

Classifier& Classifier::operator=(Classifier&& objToMove) {
  Classifier(std::move(objToCopy)).swap(*this);
  return *this;
}

void Classifier::swap(Classifier& objToSwap) {
  std::swap(words, objToSwap.words);
}

That being said, since std::map already implements the Rule-of-3/5 for its own data, you don't need to use it dynamically at all. You should strive to implement the Rule-of-0 instead, by simply getting rid of the pointer altogether and letting the compiler-generated constructors/operators invoke the corresponding methods of a std::map object for you, eg:

#include <map>

class Classifier {
private:
  std::map<DSString, std::map<char, float>> words;

public:
  // everything you need is auto-generated for you!
  // a generated default constructor will default-construct the map...
  // a generated copy constructor will copy the map...
  // a generated move constructor will move the map...
  // a generated destructor will destroy the map...
  // a generated copy assignment will copy the map...
  // a generated move assignment will move the map...
};

See how much simpler that is? :-)