Parsing JSON using jsoncpp

Question

I left questions in the comments strategically, since I think it's easier to answer questions this way.

Basically what my small class does is abstracts away functionality that was originally in a function.

This class reads json form a file and creates an object which I can use to traverse the json. I understand that if something can be made into a function it shouldn't be made into a class, but I need to practice.

Below I outlined things I'm looking for. Choose one or choose all.

Looking for:

• Did I use references "&" correctly
• #include header files correctly
• A way to initialize object like so: Root rate("test.json", ["query"]["results"]["rate"]) (syntax can be different)
• Best practice advice

Not looking for (at least not yet):

• Exceptions and Error handling
• Advice form C programmers

root.h

#ifndef ROOT_H #define ROOT_H // Should header files always be included in root.h as opposed to root.cpp? #include <fstream> #include <string> // Seems like I do not need to include this string container, why? //is it because json/json.h contains it? #include "json/json.h" // Would I ever create a class with a dependency like this one? // jsoncpp.sourceforge.net/annotated.html class Root { private: std::ifstream m_json; public: Json::Value m_root; Json::Value m_query; Root(const std::string&); ~Root(); }; #endif // ROOT_H 

root.cpp

#include "root.h" Root::Root(const std::string & filename) : m_json(filename, std::ifstream::binary) // Is std::ifstream::binary ok to put it in here like this ^? // It's working, but would that be good practice? { m_json >> m_root; m_json.close(); // Do I need .close() here? } Root::~Root(){} 

main.cpp

#include <iostream> #include "root.h" int main() { Root rate("test.json"); rate.m_query = rate.m_root["query"]["items"]; // Is it ok to assign member to a member like so, // as opposed to just a variable? // How can I instantiate my object like the line below? // Root rate("test.json", ["query"]["results"]["rate"]); // Syntax does not have to match precisely? for(const auto & it : rate.m_query) { std::cout << it << std::endl; } } 

test.json

{ "query": { "count": 3, "items": [ {"item": "4"}, {"item": "3"}, {"item": "2"} ] } 

Answer 1

// Should header files always be included in root.h as opposed to

You should include all header files that are required (no more than are required).

In your case you use the following types in the header file.

std::ifstream std::string Json::Value 

So you should include the appropriate header file for these types.

#include <fstream> #include <string> #include "json/json.h" 

The only curve ball is that if you only use a type reference then you can technically get away with a forward declaration (rather than including the header file). So you could use a forward declaration of std::string, unfortunately its not your class and you don't actually know how to forward declare it because that is not defined in the standard, so you have to include the header file.

// Seems like I do not need to include this string container, why?

It may be included indirectly via <fstream> or "json/json.h". BUT you should still include the <string> header file because these dependencies may not always hold (on different platforms or different versions of the compiler it may including things differently). So do not assume because it worked on this platform it will always work. Think worst case and explicitly include <string>

The include guard seems a bit too generic:

ROOT_H 

Seems like somebody else may use this. You need to make sure your guards are unique. I always use guards that include the namespace

// I own the domain name thorsanvil.com // and I put all my classes in the namespace ThorsAnvil // So my include guards go like this. #define THORSANVIL_<Optional Nested Namespace>_ROOT_H 

// Is std::ifstream::binary ok to put it in here like this ^?
// It's working, but would that be good practice?

Sure. That's totally fine.

You just have to understand what it mean and the difference between binary and text mode (the default). In Text mode: => "Platform Specific" "End of Line Sequence" is converted into '\n' when reading from a file. In Binary mode: no conversion is performed and you get the raw bytes.

So in the constructor:

Root::Root(const std::string & filename) : m_json(filename, std::ifstream::binary) { m_json >> m_root; m_json.close(); // Do I need .close() here? // Yes probably. // Closing it releases the related OS resources. // If you don't the resource will be held until // the whole object is destroyed. } 

I don't see the need for the m_json object to be part of the object. It is only every going to be used inthe constructor. Once the data is loaded it will never be used again. So just declare it as an automatic variable that is local to the constructor.

Root::Root(const std::string & filename) { std::ifstream m_json(filename, std::ifstream::binary) m_json >> m_root; } // Now we don't need `close()` because the object goes out of scope // and the `std::ifstream` destructor calls close for us. 

// Is it ok to assign member to a member like so,
// as opposed to just a variable?

It is. But why. It seems much more logical to use a local variable.

Json::Value query = rate.m_root["query"]["items"]; 

Prefer '\n' over std::endl

The difference is that std::endl forces the buffer to flush. The buffer will flush automatically when it needs to. Forcing it to flush is only going to make your code inefficient.

// How can I instantiate my object like the line below?
// Root rate("test.json", ["query"]["results"]["rate"]);

You can define the operator[]. You can make it take a string as an argument.

class Root { public: JsonValue operator[](std::string const& index) { return m_root[index]; } // Other stuff in your class. }; 

Now it can be used:

Root json("file.name"); Json::Value = json["query"]["results"]["rate"];