Reading and storing values from JSON

Question

I have a class ConfigFile that reads some values from a JSON using Boost and stores them in variables. Everything is done in the constructor:

ConfigFile::ConfigFile(const std::string& configFileNamePathIn)
{
 PTree tmpPT;
 std::string path;
 /// reading aws config file ///
 json::read_json(configFileNamePathIn, tmpPT);
 // read a string value
 path = "path1";
 m_param1 = tmpPT.get<std::string>(path, "");
 checkEmptyness(m_param1, "param1", path);
 // read a float value
 path = "path2";
 m_param2 = tmpPT.get< float >(path, -1);
 checkNegativity(m_param2, "param2", path);
 // ... much more lines like these two also for int and double
}

Because the constructor is long, I thought of creating 4 functions that return string, int, float or double and that receive as input parameters the string path and the PTree:

std::string readStringValue(const std::string& pathIn, const PTree& tmpPT)
{
 path = "path1";
 return tmpPT.get<std::string>(path, "");
}

In the constructor, I just call the function and verify the result:

// ...
m_param1 = readStringValue("path1", tmpPT);
checkEmptyness(m_param1, "param1", path);
// ...

Do you think this is a good improvement, or shall I keep the initial constructor?

Answer 1

I think your proposed change is an improvement for two reasons. First, it breaks up one long function into multiple smaller ones which is better for human comprehension. Second, it also allows for reuse of the functions which may also improve efficiency, reduce code size, etc.

There are, however, a couple of things that might be considered for further improvements.

Exceptions

With a long and complex constructor like this, and especially one which involves I/O, things can go wrong. Carefully considering what might go wrong and using C++ exceptions has the possibility of making your code more durable.

Named variables vs. object collection

Although you're using a constructor, the code looks very procedural. Instead of populating a number of named variables, consider each of those variables itself to be an object. You could store the name (e.g. "path1"), type, and any required value checks encapsulated within individual objects, making the code easier to read, understand and maintain. I'd be inclined to use a static data structure to store and associate those things rather than burying all of those details in code. A std::map might be appropriate.

Example code

To flesh out that latter suggestion a bit, consider that you might have a string, and an unsigned int as configuration items. To refer to them, you could use just the variable names, or you could create a map:

std::map<std::string, std::unique_ptr<ConfigItem> > config;

Here ConfigItem would be a base class for all configuration items. You would specialize that class for each different kind of configuration item, such as a plain string, a string that represents a filename, a floating point number, etc. Using the items would then use the std::string as a key to look up and retrieve the corresponding object. The std::unique_ptr is just to make sure that the contained objects are deleted when config is deleted.

Here's an example base class:

class ConfigItem
{
public:
 friend std::ostream &operator<<(std::ostream &out, const ConfigItem &ci) {
 return ci.printTo(out);
 }
 virtual operator int() const { return 0; }
 virtual operator std::string () const { return std::string(); }
 virtual ~ConfigItem() {};
protected:
 virtual std::ostream &printTo(std::ostream &out) const {
 return out;
 }
};

Note that the member functions are all virtual. Here's a specialization for a string:

class ConfigString : public ConfigItem
{
public:
 ConfigString(std::string value) : mValue(value) {}
 std::ostream &printTo(std::ostream &out) const {
 return out << mValue;
 }
 operator std::string () const { return std::string(mValue); }
private:
 std::string mValue;
};

Here's a specialization for an int:

class ConfigInt : public ConfigItem
{
public:
 ConfigInt(int value = 0) : mValue(value) {}
 std::ostream &printTo(std::ostream &out) const {
 return out << mValue;
 }
 operator int() const { return mValue; }
private:
 int mValue;
};

If you look at those two classes carefully, I'm sure it will be pretty easy to spot that they could have been written as a template instead.

Here's a test driver:

#include <iostream>
#include <iterator>
#include <memory>
#include <string>
#include <map>
#include "ConfigItem.h"
 
int main()
{
 std::map<std::string, std::unique_ptr<ConfigItem> > config;
 config.emplace("inputFilename", 
 make_unique<ConfigString>("/var/log/messages"));
 config.emplace("maxMessages", 
 make_unique<ConfigInt>(200));
 for (const auto &it : config)
 std::cout << it.first << " = " << *it.second << '\n';
 int count = *config["maxMessages"];
 std::cout << "count = " << count << '\n';
 std::string filename = *config["inputFilename"];
 std::cout << "file name = \"" << filename << "\"\n";
}

Also note that if you're only using C++11 and don't have access to a C++14 compiler (one that contains a definition for make_unique() yet), you can use this:

template<typename T, typename ...Args>
std::unique_ptr<T> make_unique( Args&& ...args )
{
 return std::unique_ptr<T>( new T( std::forward<Args>(args)... ) );
}

Sorry that's so long, but I wanted to show a clear, complete and correct example.

• sop: can you make a small example of the map, so I can understand better? (something like mymap["path1"] // of type map<string,string> and mymap["path2"] // of type map<string,float>, etc?)
• sop: Another thing: do you think is better to put the exception in the reading function (so giving as parameter the PTree)?
• Edward: I've added example code to show how one might use a std::map, but it doesn't address exceptions. For those, I think it would be best to have the reading function throw and then you could either catch them in the constructor or let them percolate up to the caller. I'd be inclined to the latter; otherwise you risk creating a zombie object.
• sop: I have made templates and I am getting the error: CMakeFiles/optimisation.dir/src/cpp/CConfigFile.cpp.o: In function std::unique_ptr<ConfigItem<std::string>, std::default_delete<ConfigItem<std::string> > > make_unique<ConfigItem<std::string>, std::string>(std::string&&): undefined reference to ConfigItem<std::string>::ConfigItem(std::string const&)
• sop: Or this one: /home/stefan/git_repos/optinisations/src/cpp/CConfigFile.cpp:13: undefined reference to std::unique_ptr<ConfigItem<std::string>, std::default_delete<ConfigItem<std::string> > > make_unique<ConfigItem<std::string>, std::string>(std::string&&), if I put the definition of the make_unique in the cpp file, and not in the hpp