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?
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