Selecting which variables to read from file

Question

I'm reading in data from an external file. The data comes in different types, both integers and strings and is shown here:

{
    "height":170,
    "weight":73,
    "country":"UK",
    "city":"Sheffield",
    "year":2017,
    "month":7,
    "day": 2
}

I'd like to decide manually which fields to read without having to recompile the program. Therefore, I do this by listing in a text file which fields I am interested in. I have a file for integer-variables and one for strings:

ints.txt

weight
year
month

strings.txt

country

Now, the data I read I store in an object of class Read_Data (see code below). I keep the data in maps, one for integers and one for strings. I'd like to know where/how can improve on my program and make it more professional. I have myself been thinking if it is possible to merge the integer- and string map such that I don't have to treat them individually as I do currently.

Here is the complete code (and a link to nlohmann's json reader):

main.cpp

#include <iostream>
#include "data.h"

int main()
{
    ///create object to store data
    Read_Data read_data;

    ///read data from file
    read_data.read();

    return 0;
}

data.h

#ifndef SFE_H
#define SFE_H

#include <fstream>
#include "json.hpp"

class Read_Data
{
private:

  ///maps of the read variables
  std::map<std::string, int> int_map;
  std::map<std::string, std::string> string_map;

  ///vectors of which variables to read
  std::vector<std::string> int_list;
  std::vector<std::string> string_list;


public:
  Read_Data();
  int read_list(std::string file_name, std::vector<std::string> &list);
  void read();
};

#endif

data.cpp

#include "data.h"

///which variables we want to read
Read_Data::Read_Data()
{
  read_list("ints.txt", int_list);
  read_list("strings.txt", string_list);
}

int Read_Data::read_list(std::string file_name, std::vector<std::string> &list)
{
  std::string value;

  std::ifstream file_strm(file_name);
  if (file_strm.is_open())
  {
    while (file_strm >> value)
      list.push_back(value);
    file_strm.close();
    return 0;
  }
  else
  {
    std::cout << "Error" << "\n";
    return 1;
  }
}

void Read_Data::read()
{
  ///load data
  std::ifstream strm("data.json");
  nlohmann::json j = nlohmann::json::parse(strm);

  ///read ints from data
  for (auto const &value : int_list)
    int_map[value] = j[value];

  ///read strings from data
  for (auto const &value : string_list)
    string_map[value] = j[value];
}

Answer 1

Interesting: The object reads the data, but the data is not returned anywhere nor is it exposed. Is it by design, I guess not ;)

Peculiar function: read_list() returns an int. I believe the function was in the main() before becoming part of a class, but the return has no effect, since the result is discarded by the caller. No exceptions, just the error printed out. Since the code doesn't perform any error handling anyway, the vectors construction could look much better:

Read_Data::Read_Data():
        int_list(std::istream_iterator<int>(std::ifstream("ints.txt")), {}),
        string_list(std::istream_iterator<std::string>(std::ifstream("strings.txt")), {})
{}

But the std::istream_iterator has constructor that only takes non-const lvalue reference to the stream (not that const version would be useful), which leads me to the next point:

The object is an object, but pretends to be an algorithm: I think that the class should be refactored to be a function which accepts two vectors (with int and string list) and a parsed json. Object has different properties than algorithm, and in current situation clearly algorithm is desired and needed.

Usually I make bare bones interface which has greatest power and doesn't take usability into account. Then I make some higher level functions which delegate to lower level functions with simplicity of use in mind:

auto separate_info(const nlohmann::json& content, 
                      const std::vector<std::string>& int_list, 
                      const std::vector<std::string>& string_list)
{
    std::map<std::string, int> int_list;
    std::map<std::string, std::string> string_list;

    /*populate them*/

    return std::pair{int_list, string_list};
}

auto read_info() //some defaulted parameters if needed
{
    std::ifstream int_parameter_list{"..."};
    std::ifstream string_parameter_list{"..."};

    template <typename T>
    using isiter = std::istream_iterator<T>;

    std::vector<std::string> int_attributes(isiter<int>{int_parameter_list}, {});
    std::vector<std::string> text_attributes(isiter<std::string>{string_parameter_list}, {});

    //parse the json

    return separate_info(j, int_attributes, text_attributes);
}

Then on the call site:

auto [int_list, string_list] = separate_info(...);

The names could've been better: naming is hard. May be there is something in common between them? What do they refer to? I guess it is something related to a person. The maps could be packed into person_info if my guess was right. It won't be that significant, but maybe pair won't be a good choice if the read data is used in many places.

struct person_info
{
     std::map<...> int_info;
     std::map<...> text_info;
}

And the last, not so important point:

Premature optimization, or ignorance: I don't mean to be offending, but returning vector from a function is probably very cheap. One thing is NRVO, which will probably kick in since it is gonna be created on the first line and returned from one point. If NRVO will fail, move constructor will kick in, so it is gonna be something like copying/swapping of 3 pointers. Although standard doesn't require this, most compilers do it. If you're on mainstream, e.g. gcc/clang/vc++, then returning vector shouldn't be a problem.