Skip to main content
swapped words to make it more engaging and comprehendible
Source Link
Incomputable
  • 9.6k
  • 3
  • 33
  • 72

The object is an algorithmobject, but pretends to be an objectalgorithm: 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.

The object is an algorithm, but pretends to be an object: 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.

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.

added 159 characters in body
Source Link
Incomputable
  • 9.6k
  • 3
  • 33
  • 72

Interesting: The object reads the data, but doesn't returnthe data is not returned anywhere nor does it exposeis 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 onby the call site anywaycaller. No exceptions, just the error printed out. Since the code doesn't perform any error handling anyway, the vectors construction could look much better:

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

The object is an algorithm, but pretends to be an object: 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 just calldelegate to lower level functions, which are meant to simplify things and make them easier to with simplicity of use. With that in mind:

Interesting: The object reads the data, but doesn't return anywhere nor does it expose it. Is it by design? I guess not ;)

Peculiar function: read_list() returns an int, but the result is discarded on the call site anyway. No exceptions, just the error printed out. Since the code doesn't perform any error handling anyway, the vectors construction could look much better:

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

The object is an algorithm, but pretends to be an object: 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.

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 just call lower level functions, which are meant to simplify things and make them easier to use. With that in mind:

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:

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 algorithm, but pretends to be an object: 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:

Source Link
Incomputable
  • 9.6k
  • 3
  • 33
  • 72

Interesting: The object reads the data, but doesn't return anywhere nor does it expose it. Is it by design? I guess not ;)

Peculiar function: read_list() returns an int, but the result is discarded on the call site anyway. 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 anyway), which leads me to the next points:

The object is an algorithm, but pretends to be an object: 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.

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 just call lower level functions, which are meant to simplify things and make them easier to use. With that 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.