Gallery template class

Question

I have created a template class Gallery which is intended to be used as a container for objects. I used a private member of typestd::map to maintain the collection alongside other functions. This class was not intended to be used on production code but rather to help me practice design and coding skills. I tried to incorporate templates, Inheritance, Polymorphism, smart pointers, mutators and accessors, a few data structures, operator overloading, the big five, and exceptions. The driver Program doesn't test everything since I tried to save on space.

Be gentle, I am only a newbie.

How can this class be designed and written better?

/**
The Gallery Class can be used to hold a map of objects that has some sort of unique identifier.
The object should implement the functions:
    const Identifier& getName();
    std::unique_ptr<LabeledObject> clone() const;
    overload: <, ==, >, and <<
*/

#ifndef GALLERY_H
#define GALLERY_H
#include <map>
#include <unordered_set>
#include <string>
#include <iostream>
#include "shape.h"

// Gallery of Objects
template<typename LabeledObject, typename Identifier> class Gallery{
public:
    /// constructors
    Gallery() = default;
    Gallery(const Gallery& galleryB);
    Gallery(Gallery&& galleryB);

    /// getters
    LabeledObject& objectCheckOut(const Identifier& name);
    const LabeledObject& viewObject(const Identifier& name) const;

    /// overloaded operators
    Gallery& operator=(const Gallery& galleryB);
    Gallery& operator=(Gallery&& galleryB);
    bool operator<(const Gallery& galleryB) const;  // compare sizes
    bool operator>(const Gallery& galleryB) const;  // compare sizes
    bool operator==(const Gallery& galleryB) const; // compare owned objects
    Gallery& operator+(const Gallery& galleryB);    // append galleryB's objects
    Gallery& operator-(const Gallery& galleryB);    // erase objects that resemble galleryB's objects
    template<typename T, typename U> friend std::ostream& operator<<(std::ostream& out, Gallery<T,U> galleryB);

    /// other utility functions
    void removeObject(const Identifier& name);
    bool isCheckedOut(const Identifier& name) const;
    bool objectExists(const Identifier& name) const;
    void objectCheckIn(const Identifier& name);
    void addObject(const LabeledObject& obj);

private:
    // boolean to show if checked out
    std::map<Identifier, std::pair<std::unique_ptr<LabeledObject>, bool>> objectsMap;

    /// private exception classes
    class GalleryException{
        private: std::string description;
        public:
            GalleryException(const std::string& description = "Exception Occurred in Gallery Class")
                : description(description){}

            const std::string& getException() const;
    };
    class ObjectCheckedOut : public GalleryException{
        public:
            ObjectCheckedOut(const std::string& description = "Object Checked Out Exception Occurred in Gallery Class")
                : GalleryException(description){}
    };
    class ObjectNotInGallery: public GalleryException{
        public:
            ObjectNotInGallery(const std::string& description = "Object Not In Gallery Exception Occurred in Gallery Class")
                : GalleryException(description){}
    };
    class ObjectNotCheckedOut: public GalleryException{
        public:
            ObjectNotCheckedOut(const std::string& description = "Object Not Checked Out Exception Occurred in Gallery Class")
                : GalleryException(description){}
    };

    /// private functions
    void appendToGallery(const Gallery& galleryB);
    static void handleException(GalleryException&&);
};

#endif

/************************************************************************************************
*************************************************************************************************
*   FUNCTION DEFINITIONS                                                                        *
*   -----------------------------                                                               *
*       Note that our class is a template hence functions are best defined within the.h file    *
*                                                                                               *
*************************************************************************************************
*************************************************************************************************/

/// constructors
    /// copy constructor
    template<typename LabeledObject, typename Identifier>
    Gallery<LabeledObject,Identifier>::Gallery(const Gallery& galleryB){
        appendToGallery(galleryB);
    }

    /// move constructor
    template<typename LabeledObject, typename Identifier>
    Gallery<LabeledObject, Identifier>::Gallery(Gallery&& galleryB)
    :objectsMap(std::move(galleryB.objectsMap)){}

/// getters
    /// viewObject
    template<typename LabeledObject, typename Identifier>
    const LabeledObject& Gallery<LabeledObject, Identifier>::viewObject(const Identifier& key) const{
        if(!objectExists(key))
            handleException(ObjectNotInGallery());
        else if(isCheckedOut(key))
            handleException(ObjectCheckedOut());

        // returning reference of unique_ptr from : [Identifier, {unique_ptr, true/false}]
        return *((objectsMap.find(key) -> second).first);
    }

    /// objectCheckOut
    template<typename LabeledObject, typename Identifier>
    LabeledObject& Gallery<LabeledObject, Identifier>::objectCheckOut(const Identifier& key){
        if(!objectExists(key))
            handleException(ObjectNotInGallery());
        else if(isCheckedOut(key))
            handleException(ObjectCheckedOut());

        // change false to true in mapElement : [Identifier, {unique_ptr, false}]
        objectsMap[key].second = true;     // label as checked out
        return *(objectsMap[key].first);
    }

/// utility functions
    /// addObject
    template<typename LabeledObject, typename Identifier>
    void Gallery<LabeledObject, Identifier>::addObject(const LabeledObject& obj){
        // new Element is in the form [Identifier, {unique_ptr, true/false}]
        objectsMap[obj.getName()] = std::make_pair(obj.clone(), false);
    }

    /// objectCheckIn
    template<typename LabeledObject, typename Identifier>
    void Gallery<LabeledObject, Identifier>::objectCheckIn(const Identifier& key){
        objectsMap[key].second = false;
    }

    /// removeObject
    template<typename LabeledObject, typename Identifier>
    void Gallery<LabeledObject, Identifier>::removeObject(const Identifier& key) {
        if(objectExists(key))
            objectsMap.erase(key);
    }

    /// isCheckedOut
    template<typename LabeledObject, typename Identifier>
    bool Gallery<LabeledObject, Identifier>::isCheckedOut(const Identifier& key) const{
        //find() returns true/false form : [Identifier, {unique_ptr, true/false}]
        return objectExists(key) && (objectsMap.find(key)-> second).second;
    }

    /// objectExists
    template<typename LabeledObject, typename Identifier>
    bool Gallery<LabeledObject, Identifier>::objectExists(const Identifier& key) const{
        return objectsMap.find(key) != objectsMap.end();
    }


/// overloaded operators
    /// assignment operator
    template<typename LabeledObject, typename Identifier>
    Gallery<LabeledObject, Identifier>& Gallery<LabeledObject, Identifier>::operator=(const Gallery& galleryB){
        if(objectsMap != galleryB.objectsMap){
            objectsMap.clear();
            appendToGallery(galleryB);
        }
        return *this;
    }

    /// move assignment
    template<typename LabeledObject, typename Identifier>
    Gallery<LabeledObject, Identifier>& Gallery<LabeledObject, Identifier>::operator=(Gallery&& galleryB){
        objectsMap = std::move(galleryB.objectsMap);
        return *this;
    }

    /// == operator
    template<typename LabeledObject, typename Identifier>
    bool Gallery<LabeledObject, Identifier>::operator==(const Gallery& galleryB) const{
        return objectsMap.size() == galleryB.objectsMap.size();
    }

    /// < operator
    template<typename LabeledObject, typename Identifier>
    bool Gallery<LabeledObject, Identifier>::operator<(const Gallery& galleryB) const{
        return objectsMap.size() < galleryB.objectsMap.size();
    }

    /// > operator
    template<typename LabeledObject, typename Identifier>
    bool Gallery<LabeledObject, Identifier>::operator>(const Gallery& galleryB) const{
        return objectsMap.size() > galleryB.objectsMap.size();
    }

    /// + operator
    template<typename LabeledObject, typename Identifier>
    Gallery<LabeledObject, Identifier>& Gallery<LabeledObject, Identifier>::operator+(const Gallery& galleryB){
        appendToGallery(galleryB);
        return *this;
    }

    /// - operator
    template<typename LabeledObject, typename Identifier>
    Gallery<LabeledObject, Identifier>& Gallery<LabeledObject, Identifier>::operator-(const Gallery& galleryB){
        for(auto it = galleryB.objectsMap.begin(); it != galleryB.objectsMap.end(); ++it)
            removeObject(it -> first);
        return *this;
    }

    /// << operator
    template<typename LabeledObject, typename Identifier>
    std::ostream& operator<<(std::ostream& out, Gallery<LabeledObject, Identifier> galleryB){
        // it points to an element in the form : [Identifier, {unique_ptr, true/false}]
        for(auto it = galleryB.objectsMap.begin(); it != galleryB.objectsMap.end(); ++it){
            std::string status = (it -> second).second ? "checked out \n" :"available \n";
            out << "This Object is " << status;
            out << ((it ->second).first) -> clone();
        }
        return out;
    }

/// private member functions
    /// appendToGallery
    template<typename LabeledObject, typename Identifier>
    void Gallery<LabeledObject, Identifier>::appendToGallery(const Gallery<LabeledObject, Identifier>& galleryB){
        // s is in the form : [Identifier, {unique_ptr, true/false}]
        for(auto s = galleryB.objectsMap.begin(); s != galleryB.objectsMap.end(); ++s)
            addObject(*((s -> second).first));
    }

    /// handleException
    template<typename LabeledObject, typename Identifier>
    void Gallery<LabeledObject, Identifier>::handleException(GalleryException&& exp){
        try{
            throw exp;
        }catch(GalleryException){
            std::cout << exp.getException();
        }
    }

/// functions of private member classes
    /// getException
    template<typename LabeledObject, typename Identifier>
    const std::string& Gallery<LabeledObject, Identifier>::GalleryException::getException() const{
        return description;
    }

/******************************************
/* DRIVER PROGRAM
/*****************************************/

#include <iostream>
#include <vector>
#include "location.h"
#include "shape.h"
#include "circle.h"
#include "rectangle.h"
#include "gallery.h"
using namespace std;

int main() {
    Location a(2,4, "A");
    Location c(5,3, "C");
    vector<shared_ptr<Shape>> shapes {make_shared<Shape>(c, "Tree", 35), make_shared<Circle>(c, "Circle C", 4), make_shared<Rectangle>(c, 6, 7, "Rec1")};
    Gallery<Shape, std::string> g, h;

    for(auto s: shapes)
        g.addObject(*c);

    h = g;
    cout << g << h;
 
    return 0;
}
```

Answer 1

First impression: looking good.

#ifndef GALLERY_H
#define GALLERY_H

The name is hardly unique. In real code, you need to consider that programs will include headers (possibly indirectly, several levels deep) from different unrelated libraries. Any simple name has a serious possibility of clashing.

Use a UUID for this.

Later: why is the #endif in the middle of the file? If you've already included the file, why do you want to repeat the template body definitions?

---

template<typename LabeledObject, typename Identifier> class Gallery{

I think the template arguments are backwards. It should work like map, with the key being first.

---

overload: <, ==, >,

Do you know about the new three-way comparison operator? You don't need separate relational operators anymore. These will be automatically generated from the "master" three-way comparison. Look up operator<=>

But wait a minute... < and > compare sizes, while == compares "owned objects"? That sounds like a bad idea.

---

GalleryException(const std::string& description = "Exception Occurred in Gallery Class")
You should generally use string_view (by value) for parameters taking strings. This being a constructor "sink" parameter, you might instead use the "sink" idiom which takes a string by value. Anytime the default argument is used, it creates a fresh string instance and copies the text into it; then it copies again to the member and deletes the first copy.

Is GetException the right name for something that gets a descriptive string? It's not derived from std::exception so it's (hopefully) not being used as an exception, and this is nothing more than a wrapper around a string, so I don't really know why you have a hierarchy here.

---

Trivial (especially single-function-call) template bodies should just be defined inline in the class. There's no reason to move them out into a separate definition, especially if it's in the same source file anyway. The template verbiage is longer than the body being supplied! This is especially true for constructors and simple accessors.

---

You don't have to put a comment on the copy constructor that says //copy constructor. We expect people to be able to read the code. Comments should add something, or give context, not restate what the code says.

---

I'm thinking the copy constructor and move constructor can just be =default. Am I missing something? You're just copying/moving the underlying instance data, right? The move constructor can just be =default, since the generated body will just move all the data members. Implementing copy constructor to call appendToGallery is a sound design. (not so much the name appendToGallery though. Look at the standard library containers: you don't have appendToMap and appendToVector and appendStrings etc. You just have append and it appends to whatever object it's called on.)

---

     if(!objectExists(key))
            handleException(ObjectNotInGallery());

Hmm, another case where your nomenclature causes confusion. You call the function to raise an exception handleException? Handling is when you catch the thing.

---

I see the same two preconditions in the next function body. Move that to common code so you only have to say it once.

---

   template<typename LabeledObject, typename Identifier>
    void Gallery<LabeledObject, Identifier>::handleException(GalleryException&& exp) 
   {
        try{
            throw exp;
        }catch(GalleryException){
            std::cout << exp.getException();
        }
    }

Whaaaaat??
You throw the object only to immediately catch it and then you return to the caller to continue, even though the preconditions have not been met?

And, you're printing the original parameter that you threw, not the thing you caught, and you're catching by value which makes a copy. This is seriously messed up.

How can the code possibly work if this function catches its own error and then returns, essentially doing nothing? The body of the caller will proceed as if the check had not failed... if that's possible, why did you need to check? Did you test this?

Looking back at one of those uses:

    template<typename LabeledObject, typename Identifier>
    LabeledObject& Gallery<LabeledObject, Identifier>::objectCheckOut(const Identifier& key){
        if(!objectExists(key))
            handleException(ObjectNotInGallery());
        else if(isCheckedOut(key))
            handleException(ObjectCheckedOut());
/// what happens after handleException returns??????
        // change false to true in mapElement : [Identifier, {unique_ptr, false}]
        objectsMap[key].second = true;     // label as checked out
        return *(objectsMap[key].first);
    }

You use objectsMap[key] twice when looking up in a map is expensive. You should look up once and keep using the same reference or iterator to what was found.

   auto& [obj,co]= objectsMap[key];
   co= true;
   return *obj

Here, you also look up the same map in each of the precondition checks, which are buried inside other function calls. This is very inefficient.

I'd write it more like this:

auto std::indirectly_readable it = lookup(key);  //throws if not present or already checked out.
auto& [obj,co]= *it;
co= true;
return *obj

The implementation of lookup should directly call map::find and check the resulting iterator against the end. Then, check (through that iterator) if it's already checked out; then return that iterator.

As for what it throws if there is a problem...
If you needed a hierarchy to represent different errors, you would derive from a standard base class and implement what. Don't make unrelated objects and make up your own naming convention for getting the string out of it!

But don't do that.

You are not adding any additional information, so just provide an error code. Make an enumeration of the different errors, and put the associated strings in an error category structure. Throw the existing std class that holds an error code.