C++ Settings Manager

Question

I'm starting a new project, and it needs to be set up using YAML files.

To handle all the settings, I've created a singleton class, I'd like you guys to review.

Here is the header:

#pragma once

#include <memory>
#include <string>

struct Settings {
    struct General {
        bool verbose;
        bool debug;
    } general;

    struct Server {
        uint16_t port;
    } server;

    struct SGX {
        bool enabled;

        struct RemoteAttestation {
            bool enabled;
            std::string server;
            uint16_t port;
            bool is_linkable;
            bool use_rdrand;
            bool quote_only;
            std::string nonce;
            std::string pubkey;
            std::string spid;
            bool epid_mode;
        } remote_attestation;
    } sgx;
};

class SettingsManager {
private:
    Settings s {
        .general = {
            .verbose = false,
            .debug = true
        },
        .server {
            .port = 8080
        },
        .sgx = {
            .enabled = true,
            .remote_attestation {
                .enabled = true,
                .server = "localhost",
                .port = 7777,
                .is_linkable = true,
                .use_rdrand = true ,
                .quote_only = false,
                .nonce = "" ,
                .pubkey = "",
                .spid = "",
                .epid_mode = false,
            }
        }
    };
    bool open_and_parse(std::string file);

    SettingsManager() {}  // Disallow instantiation outside of the class.

public:
    SettingsManager(const SettingsManager&) = delete;
    SettingsManager& operator=(const SettingsManager &) = delete;
    SettingsManager(SettingsManager &&) = delete;
    SettingsManager & operator=(SettingsManager &&) = delete;

    void print();

    static std::shared_ptr<SettingsManager> instance(const std::string file) {
        static std::shared_ptr<SettingsManager> instance(new SettingsManager());
        if (instance->open_and_parse(file) == false)
            instance = nullptr;
        return instance;
    }
    const Settings& settings = s;
};

And the implementation:

#include "SettingsManager.h"
#include <fstream>
#include <streambuf>

#include <yaml-cpp/yaml.h>
#include <fmt/core.h>

bool SettingsManager::open_and_parse(const std::string file) {
    // read file content to buffer
    YAML::Node config = YAML::LoadFile(file);

    s.general.debug = config["settings"]["general"]["debug"].as<bool>();
    s.general.verbose = config["settings"]["general"]["verbose"].as<bool>();

    s.server.port = config["settings"]["server"]["port"].as<uint16_t>();

    s.sgx.enabled = config["settings"]["sgx"]["enabled"].as<bool>();

    s.sgx.remote_attestation.enabled = config["settings"]["sgx"]["remote_attestation"]["enabled"].as<bool>();
    s.sgx.remote_attestation.server = config["settings"]["sgx"]["remote_attestation"]["server"].as<std::string>();
    s.sgx.remote_attestation.port = config["settings"]["sgx"]["remote_attestation"]["port"].as<uint16_t>();
    s.sgx.remote_attestation.is_linkable = config["settings"]["sgx"]["remote_attestation"]["is_linkable"].as<bool>();
    s.sgx.remote_attestation.use_rdrand = config["settings"]["sgx"]["remote_attestation"]["use_rdrand"].as<bool>();
    s.sgx.remote_attestation.quote_only = config["settings"]["sgx"]["remote_attestation"]["quote_only"].as<bool>();
    s.sgx.remote_attestation.nonce = config["settings"]["sgx"]["remote_attestation"]["nonce"].as<std::string>();
    s.sgx.remote_attestation.pubkey = config["settings"]["sgx"]["remote_attestation"]["pubkey"].as<std::string>();
    s.sgx.remote_attestation.spid = config["settings"]["sgx"]["remote_attestation"]["spid"].as<std::string>();
    s.sgx.remote_attestation.epid_mode = config["settings"]["sgx"]["remote_attestation"]["epid_mode"].as<bool>();

    return true;
}

void SettingsManager::print() {
    fmt::print("Settings:\n");

    auto bool_to_symbol {
        [](bool value) -> std::string {
            if (value == true)
                return "✅";
            else
                return "❌";
        }
    };

    // print all the settings
}

I know singletons are considered as a bad practice, but it really helps making everything clean. Oh and yes, I hate naming a class "...Manager" too.

In advance, thanks everyone.

Answer 1

A better way to make a singleton

You don't need to use a std::shared_ptr to make a singleton, and you also can do it with just one class, so no need for a "manager". See this post outlining how to make a singleton in C++11. In your case, it can look like this:

class Settings {
    // Private constructor, also opens and parses the file
    Settings(const std::string& file);

public:
    // Deleted copy constructor and assignment
    Settings(const Settings&) = delete;
    void operator=(const Settings&) = delete;

    // Static function for getting an instance
    Settings& instance(const std::string& file) {
        static Settings settings(file);
        return settings;
    }

    // Any member functions operating on Settings
    void print() const;

    // The actual data members
    struct {
        bool verbose;
        bool debug;
    } general;

    ...
};

The only effective difference here is that you return an empty std::shared_ptr if the file could not be loaded, whereas here you always get a valid reference to the Settings singleton. You could decide to either throw an exception if loading settings failed, or add a boolean member variable to indicate whether settings were successfully loaded from a file or not.

Consider not using the singleton pattern

The singleton pattern really is not that great. Often when you need a singleton, you can just have a regular class or struct, and just instantiate one object of that type. In a header file you put this:

struct Settings {
    ...
};

extern Settings settings;

And then in exactly one source file you write:

Settings settings;

If you can make sure that you parse and load the file into settings before any other code starts using the contents of settings, the above will work absolutely fine. It also looks very clean to me. It avoids all the shenanigans with deleted destructors, instance() functions returning references to static member variables, or std::shared_ptrs in your code.

Also note that having to pass a filename to instance() everytime doesn't make a lot of sense. Also think about what would happen if two callers pass a different filename.

Answer 2

bool open_and_parse(std::string file);

Come on, passing a string by value?
It should be a standard filesystem::path anyway.

And it returns bool? So, you're making it easy to expect failure, as one is want to do with file-related operations, but you just say "sorry, didn't do that" without any details to report back? Do you get these details in some convoluted way or are they unaccessible?

Rather than returning bool, return a standard std::error_code, which is what the failing operation has for you already so just pass it along.

---

SettingsManager() {} // Disallow instantiation outside of the class.

Did you want the default constructor to be private and usable, or not exist at all? I'm guessing the latter. So write:

SettingsManager() = delete;

But you know about =delete so maybe you do want it to exist as private. In that case, don't write an empty body for a constructor. Instead use =default.

---

if (instance->open_and_parse(file) == false)
     instance = nullptr;

It will be much more idiomatic to write:

if (!instance->open_and_parse(file))
    instance.reset();

That is, don't compare against true and false in your boolean expressions. And on the second line, be direct and use a member that's available for the express purpose, as it may be more efficient than "getting it done" by doing more general steps to achieve that.

---