C++ custom exception class

Question

I am wondering, how can I improve this simple MissingResource exception class? I am using it in my simple game.

template <typename Resource>
class MissingResource : public std::exception {
 std::string_view fileName{};
 std::string_view msg{};
 
public:
 MissingResource() = default;
 MissingResource(std::string_view fileName)
 : fileName{fileName}
 {
 initMsg<Resource>();
 }
 template <typename T>
 void initMsg() {
 msg = "Failed loading resource: "
 "{ Type: " + std::string(typeid(Resource).name()) + ", "
 "File name: " + fileName.data() + " }";
 }
 virtual const char* what() const noexcept override {
 return msg.data();
 }
};

Throw usage:

template <typename Key, typename Resource>
#if (__cplusplus == 202002L)
 requires Mappable<Key>
#endif
class ResourceHolder {
 std::unordered_map<Key, std::unique_ptr<Resource>> resources;
 std::string resourcesDir;
public:
 explicit ResourceHolder(std::string_view resourcesDir = "../resources/")
 : resourcesDir{std::move(resourcesDir)}
 {}
 template <typename... Args>
 void insert(const Key& key, std::string_view fileName, Args&&... args) {
 auto resPtr = std::make_unique<Resource>();
 bool loaded{};
 if constexpr (std::is_same<Resource, sf::Music>()) {
 loaded = resPtr->openFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
 } else {
 loaded = resPtr->loadFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
 }
 /** Called here */
 if (!loaded) {
 throw MissingResource<Resource>(fileName);
 }
 resources.emplace(key, std::move(resPtr));
}

Try-Catch usage:

ResourceManager::ResourceManager() {
 try {
 loadResources();
 } catch (const std::exception& e) {
 std::cerr << e.what() << std::endl;
 std::exit(EXIT_FAILURE);
 }
}
 

Exemplary error message:

what(): Failed loading resource: { Type: N2sf11SoundBufferE, File name: Death.wav }

P.S: If you see some smelly code or have doubts or found any kind of other mistakes, I would be looking forward to knowing them. Thanks!

• sbecker: I'm surprised that none of the answers mention that you shouldn't derive from std::exception directly, but rather from std::runtime_error or similar. But maybe I'm just wrong.
• Mikael H: @sbecker, I don't see the benefit of deriving from std::runtime_error in this case. std::exception is an abstract class, and he wants to override what basically. Maybe he can remove some code if inheriting from std::runtime_error (removing string member), but I think the class is clear enough as it is. Inheritance chains don't need to be unnecessarily long.

Answer 1

std::string_view

Firstly, there seems to be some misusages of string_view. string_view is non-owning, and is a view to an allocated char array (such as string).

The usage

template <typename Resource>
class MissingResource : public std::exception {
 std::string_view fileName{};
 std::string_view msg{};
 
public:
 [...]
 template <typename T>
 void initMsg() {
 msg = "Failed loading resource: "
 "{ Type: " + std::string(typeid(Resource).name()) + ", "
 "File name: " + fileName.data() + " }";
 }
 [...]
};

Is dangerous, since after calling initMsg(), msg will point to an destroyed object (the right hand string is temporary).

Better change it to be an owning string in this case: std::string_view msg => std::string msg. I assume you already have it like this, otherwise it would provide the output which you gave in the description.

Secondly, the other member variable std::string_view fileName does not seem necessary. Since you create the error message for msg with fileName and do not use alone, we can skip it, right?

General design of MissingResource

Applying the changes above, we have:

template <typename Resource>
class MissingResource : public std::exception {
 std::string msg{};
 
public:
 MissingResource() = default;
 MissingResource(std::string_view fileName)
 {
 initMsg<Resource>();
 }
 template <typename T>
 void initMsg(std::string_view fileName) {
 msg = "Failed loading resource: "
 "{ Type: " + std::string(typeid(Resource).name()) + ", "
 "File name: " + fileName.data() + " }";
 }
 virtual const char* what() const noexcept override {
 return msg.data();
 }
};

initMsg is currently a public method, but from what I see, it's only used locally in the constructor. If this is the case, it should rather be a private method - you want to keep the public API for the class as simple and small as possible.

Another improvement would be to be able to create msg in the constructor list - so that it does not have to be created twice. We can do this by making the initMsg a static method.

Let's remove the extra unnecessary template for initMsg as well - the entire class already holds the template argument.

And note that virtual + override is unnecessary. If you mark a method as overrided, it is implicitly virtual. Remove virtual for unnecessary noise.

We now have something like this:

template <typename Resource>
class MissingResource : public std::exception {
 std::string msg{};
 static std::string initMsg(std::string_view fileName) {
 return "Failed loading resource: "
 "{ Type: " + std::string(typeid(Resource).name()) + ", "
 "File name: " + fileName.data() + " }";
 }
 
public:
 MissingResource() = default;
 MissingResource(std::string_view fileName) : msg{initMsg<Resource>()}
 {}
 const char* what() const noexcept override {
 return msg.data();
 }
};

ResourceHolder

Since this review is already long, I will just point out one thing in ResourceHolder, namely in insert. We can avoid separating declaration from initialization for bool loaded (and make it const!) by encapsulating the logic into a separate method. This might be a bit preferential, but it is good practice to avoid separating declaration and initialization.

Something like this:

private:
 bool fromFile(std::string_view fileName, Args&&... args) 
 {
 if constexpr (std::is_same<Resource, sf::Music>()) {
 return resPtr->openFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
 } 
 else {
 return resPtr->loadFromFile(resourcesDir + fileName.data(), std::forward<Args>(args)...);
 }
public:
 template <typename... Args>
 void insert(const Key& key, std::string_view fileName, Args&&... args) {
 auto resPtr = std::make_unique<Resource>();
 
 const bool loaded = fromFile(fileName, std::forward<Args>(args)...)
 
 /** Called here */
 if (!loaded) {
 throw MissingResource<Resource>(fileName);
 }
 resources.emplace(key, std::move(resPtr));
 }

• infinitezero: Came to say this. An easy rule of thumb is: use std::string to store and std::string_view to look/pass around.

Answer 2

One problem I can see with your code is, that you can't catch MissingResource unless you know the specific Resource type used when that exception was thrown.
All you can do in the catch is to capture the generic std::exception, which is also the base for many other kinds of failure, including the stuff that's used from the c++ standard library.

A better way would be to introduce a specific base class for the whole category of MissingResource exceptions. E.g. like this:

class MissingResourceCategory : public std::exception {
protected:
 std::string_view fileName{};
 std::string_view msg{};
 MissingResourceCategory() = default;
};
template <typename Resource>
class MissingResource : public MissingResourceCategory {
 
public:
 MissingResource() = default;
 MissingResource(std::string_view fileName) : fileName{fileName} {
 initMsg<Resource>();
 }
 template <typename T>
 void initMsg() {
 msg = "Failed loading resource: "
 "{ Type: " + std::string(typeid(Resource).name()) + ", "
 "File name: " + fileName.data() + " }";
 }
 virtual const char* what() const noexcept override {
 return msg.data();
 }
};

This would allow you to distinguish MissingResource exceptions from others in your catch blocks:

ResourceManager::ResourceManager() {
 try {
 loadResources();
 } catch (const MissingResourceCategory& e) {
 std::cerr << e.what() << std::endl;
 std::exit(EXIT_RESOURCE_FAILURE);
 }
 } catch (const std::exception& e) {
 std::cerr << e.what() << std::endl;
 std::exit(EXIT_FAILURE);
 }
}