4

although I'm programming in C++ for some time now, I'm always faced with design decisions (probably due to the language's flexibility). One such problem is deciding if a method should be part of the class or not.

As an example, oftentimes, I have a Simulation class similar to the following,


class Simulation{
public:
    Simulation(void);
    bool init(const char* init_file){
        //...
        particles_ = read_particles(init_file);
        //etc
    }
    //or even,
    bool init(int argc, char* argv[]);
    void run(void);
private:
    std::vector particles_;
    //Other private members
};

Here, I'm not sure if this init method, which parses a file and initializes the simulation should really be a class method. A different way to implement similar functionality, would be to have an external function or class which handles the parsing of either the command line arguments or the initialization file, and calls appropriate methods of the Simulation class.


bool init_simulation(Simulation& sim, const char* init_file){
    //...
    Particles particles = read_particles(init_file);
    sim.set_particles(particles);
    //etc
}

On the one hand it's easier to implement the functionality as a class method and it avoids having to introduce new classes/structs, specifically for passing parameters. On the other hand, externally handling the initialization, means that one can more easily extend it, although any changes on the Simulation class will propagate to the function.

What is the preferred design in situations similar to the one described above, and how should I approach similar design decisions?

  • 3
    This creates a coupling between the object and how it's used. A big part of good design is to isolate the parts of a program so changes in one part don't require changing other parts. My opinion is that you shouldn't do this, but if you only ever use this object in this program it might be easier to just go with it. It's up to you. – Doval Nov 24 '14 at 17:36
  • Agreed about the coupling. In this example, I would be surprised if it would be easier - assuming starting from scratch - to integrate the file parsing into the class. After all, creating a class that just accepts simple data input is easy and it lets you run an early version of the code and prove it works. You can worry about file input later. – itsbruce Nov 24 '14 at 18:46
  • I think stream operators (<</>>) are the most common way of implementing this. But then again, you would have to choose between a member operator or a global one. – glampert Nov 24 '14 at 21:45
10

Your problem isn´t having a class with a constructor method, your problem is what you have put into it. Parsing a text file should not be part of the object - its job is to create and manage Simulations. The constructor/init method should take a simple list or map of particles as one of its parameters and use that data when initialising. Reading data from a file and converting it into a simple list or map is the job of some other part of your code.

  • If you change the file format, or completely change how your data is stored (e.g. in a relational database), you should not have to rewrite the Simulation class.
  • You should not need an input file to write tests for this class. Your tests should be able to create simple input data and pass that directly to the object.

Separation of concerns. The file (its location, format etc.) is no concern of your Simulation object.

  • Thank you for your answer. I kind of realized this after posting the question. Unfortunately, Particles is of course not the only information Simulation needs. As such I'll have to either implement a struct holding all the information needed or/and implement various setters. – Grieverheart Nov 25 '14 at 9:32
  • Setters so much a better solution than a struct. – itsbruce Nov 25 '14 at 10:56
3

For discussion, I'm going to present a counter-argument to @itsbruce. YMMV.

What is more likely to change, the file format or the object itself? (as in additional or changed fields).

In my experience, if the file format is standard, and accessed through abstract interfaces (e.g. standard XML, JSON libraries), the core code for that doesn't change. It's much more likely for the object to change than the file format.

In that case, it does make some sense for the object to be able to read and write itself. When the Foo object changes, only one file, Foo, needs to change.

If you have a separate FooPersister, and the Foo object adds or changes a field, you are likely to have to modify FooPersister. e.g., in Foo, you'd add a .newField, appropriate setters getters and logic. And, in FooPersister.read(Foo) and FooPersister.write(Foo), you have to remember to add that field to both methods. What if a field changes from int to double? Likely some FooPersister code changes there.

Of course, it depends on your persistence framework, if it automagically reads the fields, or it's as simple as adding an annotation somewhere. And how mature your project is, if your Foo object is already a gazillion lines of code, etc.. As said before, YMMV. (You could farm out the actual persistence code to a friend class, or, in Java-ese, a private internal class, so your code remains somewhat separated, but the public API rests in the Foo class.

One huge advantage of having the object be able to persist itself is that you may be able to eliminate many of your getters and setters. If you have a FooPersister, you pretty much must expose all your non-transient internal fields to it. If you persist yourself, sometimes you can keep a lot more internal and hidden (and final)

Now, I wouldn't do exactly as OP does, reading/writing to files:

bool init(const char* init_file)

I'd probably have it read/write from/to a Stream, a Node or Element, or something like that, more abstract than a file. Also, even though it "feels weird", consider having the read/write methods be instance methods (read should returns a new Foo, not overwrite the old one), not a static class function. This opens a lot of interesting doors in your design.

  • You've started with a huge assumption - that there is a persistence framework. You also have no idea if the format and information needed to create the Simulation object in an initial state is identical to (or even similar) that which would be necessary to persist a running Simulation. I really do not get that impression from the OP but you should have asked before writing all that. – itsbruce Nov 24 '14 at 22:00
  • And in the end, you don't seem to realise that you're not actually disagreeing with me. I have absolutely no objection to the object providing a stream of data that could be put into a file or db or session data by something else. And you say the same. So I don't see where you think we are at odds. – itsbruce Nov 24 '14 at 22:07
  • @itsbruce Yes, we are largely agreeing, but just looking at it from slightly different angles. – user949300 Nov 27 '14 at 17:49
2

[Note: This question already has an accepted answer, and this answer only adds more arguments to it - but it is too long for a comment].

One such problem is deciding if a method should be part of the class or not.

I have found a very good guideline offered by A. Stepanov on this question:

He basically says that objects should be constructed from fully constructed data members. If the construction of an object requires extra processing (for example, reading the data from a file), it should be done by an external factory function. This maximizes flexibility and reusability. (Eventually, you could set the factory function as a friend).

An example:

class Simulation{
public:
    Simulation(std::vector<particle> particles);
        // instance keeps it's own copy,
        // so we might as well construct
        // it in the argument itself and
        // pass it into particles_ using
        // std::move

    void run();
private:
    std::vector<particle> particles_;
};

// factory function only 
Simulation load(std::istream& in) // use an istream instance here
                                  // (see below for reasoning)
{
    std::vector<particle> particles;
    particle p;
    while(in >> particle)
        particles.emplace_back(std::move(p));
    return Simulation{ std::move(particles) };
}

Test code (working with hardcoded particles):

std::vector<particle> p = test::get_particles();
Simulation s{ std::move(p) };

Test code (loading particles function):

std::string serialized_particles = "jdfkdfhaljkdfhlakjdfhalsjkfh";
std::istringstream in{ serialized_particles };
auto s = load( in );

Production code:

std::string path_to_file{ "/tmp/aaaaa" };
std::ifstream in{ path_to_file };
auto s = load( in );

Outside of this example (and your particular case), the guideline I use to determine if a function should be a member or not, is solved by answering these questions:

  • does the functionality apply to all instances? (if yes, it should be a member)
  • does the functionality require an active instance? (if no, it should not be a member)
  • does the functionality apply to an instance in any state? (if no, function should probably not be a member)
  • why not make it a static member instead of a friend? If its only needed in the context of that class I would think that would make the most sense, and you could avoid spamming the namespace even if your class already exists in a namespace itself (your probably going to need more of these functions for different orthogonal classes where one factory cannot cover all) regardless I found this answer useful. – opa Oct 2 '17 at 3:18
  • 1. If you add a function to the class (as static) you are making it a part of the API (even if not called on an instance). If you want to reuse the class in a different context, you will carry baggage with you that you (perhaps) do not need. 2. An external API can also have dependencies that you do not want to mix with the class itself. A separate function will keep dependencies separate. 3. Consistency: I may have a save free function (for example) that I can apply (in a polymorphic way) to three different hierarchies of objects. This would make no sense with static functions. – utnapistim Oct 2 '17 at 8:09
  • Consider this, if you have an arbitrary namespace, and you want to and you want to do IO for class A,B,C. No class shares the same purpose, parent class, or constructor arguments. If you want to load these classes, a factory function would not make sense, they don't share the same base so you would have to use variant trickery to get the right value out. If you don't make it a static member, you would have to call each function loadA.... In this situation I imagine that static member makes more sense A A::load(filename);, but I really think this comes down to a load/save tradeoff. – opa Oct 2 '17 at 13:26
  • The 'loadA' notation sounds like C to me. For C++, my APIs would all be called load (in your example), and be distinguished by the compiler through argument-based polymorphism; code would be: load(filename, objA); load(filename, objB); load(filename, someOtherGizmo);. – utnapistim Oct 2 '17 at 14:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.