Feedback on the design - C++

This is the header of my project (I know it's not ideal to contain all the classes at the same header but this is the requirement of our presubmission tool).
Maybe you can review the header and give some feedback on the construction and design from an "OOP view of point".

Thanks in advance!

Code:

---

#ifndef FRACTAL_H
#define FRACTAL_H


#include <vector>
#include <string>
#include <iosfwd>


const char WHITESPACE = ' ';


const std::string FRACTALCELL = "#";


const int FIRSTTYPE = 1;


const int SECONDTYPE = 2;


const int FIRSTBASEDIM = 3;


const int SECONDBASEDIM = 2;


const int THIRDBASEDIM = 3;


/**
 * @class Fractal
 * @brief Abstract class for specific fractals to be depicted
 */
class Fractal
{
public:
    /**
    * @brief Destructor of Fractal base class - to be able to destroy the sub-objects of the base class
    * There are no some buffers/new allocations in this class - so the default one is sufficient
    */
    virtual ~Fractal() = default;
protected:


    const int m_Dim; // Dimension of the fractal grid
    std::vector<std::vector<bool>> m_Pattern; // Grid matrix representation via boolean values
    mutable std::vector<std::string> m_Output; // The final fractal grid's lines


    /**
    * @brief Constructor of base-class sub-objects
    * @param dim The dimension of the fractal to be drawn
    */
    explicit Fractal(int dim);
    /**
    * @brief Here we use the OOP concept of Factory Method Pattern
    * Using this method, we can create derived-fractal objects without having to specify the exact
    * class type of the object that will be created.
    * @param smallerDim The dimension of the sub-fractal of another fractal
    * Thus, this dimension will always be lower than the superior fractal by 1.
    * @return Specific deriving class object - in particular, a sub-fractal .
    */
    virtual Fractal *_smallerClone(int smallerDim) const = 0;
    /**
    * @brief This function is recursively building the grid of our fractal.
    * @return Returns the lines of its final representative grid.
    */
    std::vector<std::string> toLines() const;
    /**
    * @brief This is an helper function of toString method - it generates one string that contains at once
    * all the lines of the grid received from toLines method
    * @return Returns the output-stream received, buffered with the suitable string.
    */
    std::ostream &toString(std::ostream &os) const;
    /**
    * @brief The print function - the requested output stream is buffered with the draw of a fractal
    * and is output.
    * @param os The chosen output stream
    * @param f The fractal to be printed
    * @return Reference to the exact output stream, now buffered with the draw of fractal f
    */
    friend std::ostream &operator<<(std::ostream &os, const Fractal &f); // The actual print function
};


/**
 * @class Deriving class of Fractal
 * @brief represents specific kind of fractal to be output
 */
class SierpinskyCarpet: public Fractal
{
public:
    /**
    * @brief Constructor of a specific fractal
    * @param dim The dimension of the fractal
    */
    explicit SierpinskyCarpet(int dim);
private:
    /**
    * @brief Using this method, we can create SierpinskyCarpet fractal objects without having to
    * specify the exact class type at run-time.
    * @param smallerDim The dimension of the sub-fractal the current fractal
    * Thus, this dimension will always be lower than the superior fractal by 1.
    * @return Same class object - in particular, a sub-fractal .
    */
    SierpinskyCarpet *_smallerClone(int smallerDim) const override;
};


/**
 * @class Deriving class of Fractal
 * @brief represents specific kind of fractal to be output
 */
class SierpinskyTriangle: public Fractal
{
public:
    /**
    * @brief Constructor of a specific fractal
    * @param dim The dimension of the fractal
    */
    explicit SierpinskyTriangle(int dim);
private:
    /**
    * @brief Using this method, we can create SierpinskyTriangle fractal objects without having to
    * specify the exact class type at run-time.
    * @param smallerDim The dimension of the sub-fractal the current fractal
    * Thus, this dimension will always be lower than the superior fractal by 1.
    * @return Same class object - in particular, a sub-fractal .
    */
    SierpinskyTriangle *_smallerClone(int smallerDim) const override;
};


/**
 * @class Deriving class of Fractal
 * @brief represents specific kind of fractal to be output
 */
class Vicsek: public Fractal
{
public:
    /**
    * @brief Constructor of a specific fractal
    * @param dim The dimension of the fractal
    */
    explicit Vicsek(int dim);
private:
    /**
    * @brief Using this method, we can create Vicsek fractal objects without having to
    * specify the exact class type at run-time.
    * @param smallerDim The dimension of the sub-fractal the current fractal
    * Thus, this dimension will always be lower than the superior fractal by 1.
    * @return Same class object - in particular, a sub-fractal .
    */
    Vicsek *_smallerClone(int smallerDim) const override;
};


/**
 * @class Factory class
 * @brief Using this class one can generate a specific fractal object
 */
class FractalFactory
{
public:
    /**
    * @brief Single static method to generate specific fractals
    * @param type The type of one of three fractals indicated by 1/2/3
    * @param dim The dimension of the wanted fractal
    * @return Pointer to object of the specific fractal, held by the base class variable
    * Thus, we can perform this way polymorphism as we wish
    */
    static Fractal *factoryMethod(int type, int dim);
};


#endif

---

You shouldn't define constants like this in a header file:

Code:

---

const char WHITESPACE = ' ';
 
 
const std::string FRACTALCELL = "#";
 
 
const int FIRSTTYPE = 1;
 
 
const int SECONDTYPE = 2;
 
 
const int FIRSTBASEDIM = 3;
 
 
const int SECONDBASEDIM = 2;
 
 
const int THIRDBASEDIM = 3;

---

The problem is that these constants will be defined at this point, but this means that if the header is included in more than one source file, the constants will be defined more than once in the program, which is an error (breaks the one definition rule).

Is the user of the class expected to use these constants? If so, then define them inline:

Code:

---

inline const char WHITESPACE = ' ';

---

Inline definitions can appear multiple times in the same program and yet not break the one definition rule.

If not, i.e., if these are purely implementation detail, then move these definitions to an unnamed namespace in the relevant source file. The reason for the unnamed namespace is so as to eliminate the possibility of name collisions.

For your Fractal class hierarchy: it looks like the Fractal base class only has two functions in its public interface: the destructor and the overloaded operator<< for ostream. Is that correct? Incidentally, I would move the declaration of operator<< to the public section: it doesn't matter for correctness, but it avoids the impression that operator<< is protected when it isn't.

Related to this, do the derived classes need access to toLines and toString? If not, they should be private, not protected. Likewise, when overriding _smallerClone, do derived classes need to call the base class _smallerClone? If not, it should be private too. I would suggest just naming it smallerClone, or perhaps there might be a more apt name, say generateSubFractal. I suspect that this member function should actually be public though: it sounds like it should be part of the Fractal public interface. That said, since you're declaring the member variables protected, having private member functions don't matter so much, but it does indicate to derived classes what isn't part of the protected interface that they are expected to use, i.e., it is pure implementation detail to the Fractcal class. It could be that m_Output should actually be private too (i.e., it is intended to cache the output of toLines or toString), in which case having these private member functions would make even more sense.

Speaking of toString, it is weird that it returns a reference to ostream instead of a string. Perhaps it would be better named print or write. You don't actually need to return a reference to ostream because you probably don't want function chaining for this: if you want to indicate success/failure based on the state of the stream, returning a bool might be better, then you just convert the stream to bool when returning.

Because m_Dim is const, you should disable the Fractal copy assignment operator and move assignment operator because assignment would not make sense. Because m_Dim doesn't have to be changed when being moved from, unlike objects of some types that get their resources "stolen" when moved from, the move constructor might still make sense. Furthermore, since Fractal is an abstract base class, I would disable the copy constructor too, and instead declare a public pure virtual clone member function for cloning objects of Fractal derived classes. Derived classes would then override this clone member function to clone themselves (similiar to what you have in mind for _smallerClone, except that it would be an actual copy). If you want to leave the copy constructor as-is, that's fine too, except that it means that if you only have a Fractal pointer, you cannot copy the Fractal derived class object it points to without some ugly determining of the derived class type so you can invoke its copy constructor.

Note that std::vector<bool> isn't your ordinary vector: that particular vector specialisation is potentially different in order to accommodate possible optimisations related to bool. Read cppreference on std::vector<bool> for details. This might not matter for what you're doing, but you need to be aware in case it does matter.

Other than some pedantic reason of not having the base class know anything about its derived classes, I don't see the point of a separate FractalFactory class. You're basically using it as a namespace. factoryMethod also sounds like a rather odd name for a function. I suggest defining a static member function named generate in the Fractal class instead. (Fractal::generate is more palatable and to-the-point than FractalFactory::factoryMethod, wouldn't you agree?) Of course, this member function cannot be defined inline in the Fractal class definition because it needs to know the derived classes in order to instantiate them, yet the derived classes need the Fractal class definition in order to be defined, but you're already defining this member function separately, so there's no chicken-and-egg scenario to begin with.

@laserlight -
Amazing.
I want to scrutinize your remarks more in depth, but now I have no time because I have some exercises in math courses to submit.

Thank you so much!