I am writing a finite element software in C++ and Qt. I want to load the input data to the model. I have a Model class that holds the data and does the work. I have created a simple lexer and parser to read the input file (things that resemble database records). I don't have the experience to make an informed decision on how relate the file reader class and model class. I have two options:

1) In class Model, create a method to load the file, i.e. in pseudocode:

 class Model
 {
   // private fields

   public:
     loadFile( filename )
  }

2) This:

class Lexer(input filename)
{
    ...
    loadToModel(Model&)  // red the input filename into the Model.
}

Which one is better or makes more sense? What are the pros and cons of each one?

up vote 5 down vote accepted

Although the code in your question isn't very clear, my understanding is that you are proposing to create an object from some input data. You outline two options:

  1. Make loading the data a public method. There are few upsides to this beyond convenience. It's a pretty clear violation of the Single Responsibility Principle and it couples the model's in-memory representation to its on-disk representation. This isn't usually a very good thing. It also potentially leaves your model in an invalid state after creation, which is also very error-prone. It is much better to have classes that are never in invalid states whenever possible. A slight improvement would be to make this a static member function, rather than an instance method.

  2. Pass an existing instance to a separate loader. This is a somewhat better approach if, again, your model object has no invalid states. And if your model has no invalid states, then it should be trivial to initialize a default-constructed one inside the function, rather than taking a reference as a parameter.

The best solution, then, is the adjustment I mentioned in (2). Design your model so that a default-constructed instance is valid, and then write a method in your Lexer (perhaps renamed to "ModelLoader" or "Parser" since it is doing more than lexing) to load a file from disk and populate a fresh Model instance, and then return it. In modern C++, guaranteed return value optimization (RVO) should alleviate any concerns you might have with returning a large object.

Ideally, your loader/deserializer would also take an input stream of some sort, rather than a filename directly. Qt's QDataStream, for example, can abstract over several different media (drives, network, in-memory buffers, etc.)

If you need (or want) your Model to be immutable, then you can employ the Builder pattern to stage updates to a Model before actually constructing it.

  • But isn't (1) the usual way to serialize,/deserialize ? And why is there a violation of SRP ? – Christophe Nov 15 at 8:49
  • 2
    No, it isn't. Even extending the basic std io streams involves writing non-member specializations. That's also the way Qt's QDataStream does it, which OP says they're using. But the violation of SRP here is the fact that loadFile(filename) implies that Model is not only responsible for deserialization but thirdly for disk I/O. – Alex Reinking Nov 15 at 9:21
  • SRP is about reason to change and not about functionality. Here an explanation of Uncle Bob who invented the SRP: blog.cleancoder.com/uncle-bob/2014/05/08/… . – Christophe Nov 15 at 10:09
  • 2
    @Christophe and where you are sourcing your sequences of bytes is a changable choice, with almost no relation to what you are doing with those bytes – Caleth Nov 15 at 10:16
  • 1
    @Christophe: it pretty much depends on what the responsibility of the Model class should be here in this case: if it is just a dumb container, persisting logic could fit into its realms of responsibility. If it contains also finite element calculation logic, then this would give at least two very "different reasons to change" to that class. – Doc Brown Nov 15 at 11:07

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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