38

If I want to save and retrieve an object, should I create another class to handle it, or would it better to do that in the class itself? Or maybe mixing both?

Which is recommended according to OOD paradigm?

For example

Class Student
{
    public string Name {set; get;}
    ....
    public bool Save()
    {
        SqlConnection con = ...
        // Save the class in the db
    }
    public bool Retrieve()
    {
         // search the db for the student and fill the attributes
    }
    public List<Student> RetrieveAllStudents()
    {
         // this is such a method I have most problem with it 
         // that an object returns an array of objects of its own class!
    }
}

Versus. (I know the following is recommended, however it seems to me a bit against the cohesion of Student class)

Class Student { /* */ }
Class DB {
  public bool AddStudent(Student s)
  {

  }
  public Student RetrieveStudent(Criteria)
  {
  } 
  public List<Student> RetrieveAllStudents()
  {
  }
}

How about mixing them?

   Class Student
    {
        public string Name {set; get;}
        ....
        public bool Save()
        {
            /// do some business logic!
            db.AddStudent(this);
        }
        public bool Retrieve()
        {
             // build the criteria 
             db.RetrieveStudent(criteria);
             // fill the attributes
        }
    }
  • 3
    You should do some research on the Active Record pattern, such as this question or this one – Eric King Dec 9 '14 at 19:08
  • @gnat, I thought asking which is better is opinion based then added "pros and cons" and have no problem to remove it, but in fact I mean regarding the OOD paradigm which is recommended – Ahmad Dec 9 '14 at 19:12
  • 3
    There is no correct answer. It depends on your needs, your preferences, how your class is to be used and where you see your project going. The only correct OO thing is that you can save and retrieve as objects. – Dunk Dec 9 '14 at 23:29
34

Single Responsibility Principle, Separation of Concerns and Functional Cohesion. If you read up on these concepts, the answer you get is: Separate them.

A simple reason to separate the Student from the "DB" class (or StudentRepository, to follow more popular conventions) is to allow you to change your "business rules", present in the Student class, without affecting code that is responsible for persistence, and vice-versa.

This kind of separation is very important, not only between business rules and persistence, but between the many concerns of your system, to allow you to make changes with minimal impact in unrelated modules (minimal because sometimes it is unavoidable). It helps to build more robust systems, that are easier to maintain and are more reliable when there are constant changes.

By having business rules and persistence mixed together, either a single class as in your first example, or with DB as a dependency of Student, you're coupling two very different concerns. It may look like they belong together; they seem to be cohesive because they use the same data. But here's the thing: cohesion cannot be measured solely by the data that is shared among procedures, you must also consider the level of abstraction at which they exist. In fact, the ideal type of cohesion is described as:

Functional cohesion is when parts of a module are grouped because they all contribute to a single well-defined task of the module.

And clearly, performing validations about a Student while also persisting it do not form "a single well-defined task". So again, business rules and persistence mechanisms are two very different aspects of the system, which by many principles of good object oriented design, should be kept separate.

I recommend reading about Clean Architecture, watching this talks about Single Responsibility Principle (where a very similar example is used), and watching this talk about Clean Architecture as well. These concepts outline the reasons behind such separations.

  • 11
    Ah, but the Student class should be properly encapsulated, yes? How can then an external class manage persistence of private state? Encapsulation has to be balanced against SoC and SRP, but simply choosing either over the other without carefully weighing the tradeoffs is probably wrong. A possible solution to this conundrum is using package-private accessors for the persistence code to use. – amon Dec 9 '14 at 16:57
  • 1
    @amon I agree that it is not so simple, but I believe it is a matter of coupling, not encapsulation. For instance, you can make the Student class abstract, with abstract methods for any data that the business needs, which are then implemented by the repository. This way, the data-structure of the DB is completely hidden behind the repository implementation, so it is even encapsulated from the Student class. But, at the same time, the repository is deeply coupled to the Student class - if any abstract methods are changed, the implementation has to change as well. It's all about trade-offs. :) – MichelHenrich Dec 9 '14 at 17:10
  • 4
    The claim that SRP makes programs easier to maintain is dubious at best. The problem SRP creates is that instead of having a collection of well-named classes, where the name tells everything the class can and can't do (in other words easy to understand which leads to easy to maintain) you end up with hundreds/thousands of classes which people needed to use a thesaurus in order to pick a unique name, many names are similar but not quite and sorting through all that chaff is a chore. IOW, Not maintainable at all, IMO. – Dunk Dec 9 '14 at 23:18
  • 3
    @Dunk you speak as if classes were the only modularization unit available. I've seen and written many projects following SRP, and I agree that your example does happen, but only if you do not make correct use of packages and libraries. If you separate responsibilities into packages, since the context is implied by it, you can name the classes freely again. Then, to maintain a system like this, all it takes is to drill-drown to the right package before searching for the class that you need to change. :) – MichelHenrich Dec 9 '14 at 23:26
  • 5
    @Dunk OOD principles can be stated as: "In principle, doing this is better because of X, Y and Z". It does not mean that it should always be applied; people have to be pragmatic and weight the trade-offs. In big projects, the benefits are such that usually outweigh the learning curve that you speak of - but of course, that is not the reality for most people, and so it shouldn't be applied as heavily. However, even in the lighter applications SRP, I think we can both agree that separating persistence from business rules always yields benefits beyond its cost (except maybe for throw-away code). – MichelHenrich Dec 10 '14 at 0:25
10

Both approaches violate the Single Responsibility Principle. You first version gives the Student class to many responsibilities and ties it to a specific DB access technology. The second leads to a huge DB class which will be responsible not just for students, but for any other kind of data object in your program. EDIT: your third approach is the worst, since it creates a cyclic dependency between the DB class and the Student class.

So if you are not going to write a toy program, use none of them. Instead, use a different class like a StudentRepository for providing an API for loading and saving, assumed you are going to implement the CRUD code on your own. You might also consider to use an ORM framework, which can do the hard work for you (and the framework will typically enforce some decisions where the Load and Save operations have to be placed).

  • Thank you, I modified my answer, what about third approach? anyway I think here the point is about make distinct layers – Ahmad Dec 9 '14 at 19:02
  • Something also suggest to me to use StudentRepository instead of DB (don't know why! Maybe again single responsibility) but I still can't get why different repository for each class? if its just a data access layer then there are more bunch of static utility functions and can be together, not? just maybe then it would became so dirty and crowded class (sorry for my English) – Ahmad Dec 9 '14 at 19:30
  • 1
    @Ahmad: there a couple of reasons, and a couple of pros and cons to consider. This could fill a whole book. The reasoning behind this boils down to testability, maintainability and long-term evolvability of your programs, especially when they have a long-lasting life cycle. – Doc Brown Dec 9 '14 at 20:11
  • Has anybody worked on a project where there was a significant change in the DB technology? (without a massive rewrite?) I have not. If so, did following SRP, as suggested here to avoid being tied to that DB, help significantly? – user949300 Dec 10 '14 at 3:42
  • @user949300: I think I did not express myself clearly. The student class does not become tied to a specific DB technology, it becomes tied to a specific DB access technology, so it expects something like an SQL DB for persistence. Keeping the Student class completely unaware of the persistence mechanism does allow much easier reuse of the class in different contexts. For example, in a testing context, or in a context where the objects are stored in a file. And that is something I did indeed in the past very often. No need to change the whole DB stack of your system to gain benefit from this. – Doc Brown Dec 13 '14 at 13:27
3

There are quite a few patterns that can be used for data persistence. There is the Unit of Work pattern, there is Repository pattern, there are some additional patterns that can be used like the Remote Facade, and so on and on.

Most of those have their fans and their critics. Often it comes to picking what seems to suit the application best and sticking with it (with all its upsides and downsides, i.e. not using both patterns at the same time... unless you're really sure about that).

As a side note: in your example the RetrieveStudent, AddStudent should be static methods (because they are not instance-dependant).

Another way of having in-class save/load methods is:

class Animal{
    public string Name {get; set;}
    public void Save(){...}
    public static Animal Load(string name){....}
    public static string[] GetAllNames(){...} // if you just want to list them
    public static Animal[] GetAllAnimals(){...} // if you actually need to retrieve them all
}

Personally I'd use such approach only in a fairly small applications, possibly tools for personal use or where I can reliably predict that I won't have a more complicated use cases than just saving or loading objects.

Also personally, see the Unit of Work pattern. When you get to know it, it is really good in both small and large cases. And it is supported by a lot of frameworks/apis, to name EntityFramework or RavenDB for example.

  • 2
    About your side note: Although, conceptually, there is only one DB while the application is running, it doesn't mean you should make RetriveStudent and AddStudent methods static. Repositories are usually made with interfaces in order to allow the developers to switch implementations without affecting the rest of the application. This can even allow you to distribute your application with support for different databases, for instance. This same logic, of course, applies to many other areas besides persistence, as, for instance, UI. – MichelHenrich Dec 9 '14 at 16:57
  • You're definitely right, I've made a lot of assumptions based on the original question. – Gerino Dec 9 '14 at 17:06
  • thank you, I think best approach is using layers as mentioned in Repository pattern – Ahmad Dec 9 '14 at 19:04
1

If it's a very simple app where the object is more or less tied to the datastore and visa-versa (i.e. can be thought of as a property of the datastore), then having a .save() method for that class might make sense.

But I think that would be pretty exceptional.

Rather, it's usually better to let the class manage its data and its functionality (like a good OO citizen), and externalize the persistence mechanism to another class, or set of classes.

The other option is to use a persistence framework that defines persistence declaratively (like with annotations), but that's still externalizing the persistence.

-1
  • Define a method on that class that serializes the object, and returns a sequence of bytes that you can put in a database or file or whatever
  • Have a constructor for that object that takes such a sequence of bytes as input

About the RetrieveAllStudents() method, your feeling is right, it is indeed probably misplaced, because you might have multiple distinct lists of students. Why not simply keep the list(s) outside of the Student class ?

  • You know I have seen such trend in some PHP framework for example Laravel if you know it. The model is tied to database and can even return a series of objects. – Ahmad Dec 13 '14 at 8:20

protected by gnat Dec 13 '14 at 7:22

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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