9

In my application design, I usually map objects to the important tables in the database. The objects then handle everything relating to that data (including linkage tables). So I for example have built an Activity object, with properties like name and due_date, methods like load() and save(), and also methods like getParent(), getContributors() and getTeam(), which return (arrays of) other objects. Is this 'bad' OOP because it violates the Single Responsibility Principle?

Rijk
  • 11,032
  • 3
  • 30
  • 45
  • 1
    How would you answer the question "What is the class responsible for?" – soulmerge Sep 14 '11 at 08:34
  • I thought about that, and that lead me to this question. I'd say: handling everything related to activities. But I'm not sure that is valid :) – Rijk Sep 14 '11 at 08:38
  • Thank you for interesting question. Not about "how to sort array", as many others :) – OZ_ Sep 14 '11 at 09:11

5 Answers5

5

It depends on the situation and the exact code you have: Your design might touch multiple responsibilities and still be a pretty nice OOP and maintainable.

Do you handle load() and save() in each class with similar code? Or do you delegate the task within load() and save() to other objects that are used for this functionality in several classes? That would be half-what following SRP and still be according to your design.

If not, your code really seems a bit smelly. To check whether it covers multiple responsibilities, ask yourself: what could cause changes to my class? In your situation, I would at least try to refactor the similar code in load() and save() in different classes to reach the situation described above, so that

  • maintainability is greatly improved,
  • you still do not need to change your clients' code.
DaveFar
  • 7,078
  • 4
  • 50
  • 90
  • 1
    Indeed. Extract the database interaction methods into another class, thus creating a data-access layer. Your business logic will then reside in the model, your DB interaction elsewhere. – cloakedninjas Sep 14 '11 at 08:54
  • I think code reuse of `load()` and `save()` it's good and important theme, but for another question. Model can work with DB as it necessary - with code reuse (ORM, Mapper), or without - depends on what is better in concrete case. In this question I see main theme as collaborating with other Models, dependencies and responsibility. – OZ_ Sep 14 '11 at 09:10
  • 1
    I think responsibilities, dependencies, code reuse, maintainability, they are all interrelated. You shouldn't just follow SRP because it's said to be good, but have in mind why that is so (ergo talk about maintainability and code reuse). – DaveFar Sep 14 '11 at 09:14
  • 2
    I think it is maintainable. There's very little duplication, and I do use a DB abstraction layer. In the `load()` method, I just run the (class-specific) query to fetch the data, do some pre-processing like escaping HTML characters in the text properties like `name`, and assign the data to the object's properties. – Rijk Sep 14 '11 at 09:33
  • If you don't have duplicate/similar code for the pre-processing, then I'd say you have good OOP and followed the SRP sufficiently. The SRP is also not a strict rule you have to follow 100%, it's rather a guideline. To get a feeling for this, you can listen to the debates with Uncle Bob about the SOLID design principles at hanselminutes and stackoverflow/stackexchange podcasts. – DaveFar Sep 14 '11 at 10:00
  • `The SRP is also not a strict rule you have to follow 100%, it's rather a guideline.` - no, it's a strict rule and this rule correlates with [Separation of concerns](http://en.wikipedia.org/wiki/Separation_of_concerns) - one of the most important rules in engineering. – OZ_ Sep 14 '11 at 10:11
  • Can you give a reason or reference why it is a strict rule? I have given references for SRP being a guideline, from Uncle Bob, the author of SRP. – DaveFar Sep 14 '11 at 10:33
  • @DaveBall aka user750378 if one of SOLID principles is violated - application is designed wrong. That's why these principles exists. And SRP, [by words](http://www.objectmentor.com/resources/articles/srp.pdf) of Martin (Uncle Bob), *"was described in the work of Tom DeMarco and Meilir Page-Jones"*. Also, in that document I can't find this is not a principle but just a *"guideline"*. But I can find *"The violation of SRP causes several nasty problems"*. – OZ_ Sep 14 '11 at 11:09
1

Well .. its hard to tell at this stage. You could pastbin the whole class , but ..

Yes , it looks like bad OOP. You have same class responsible for interaction with database and domain logic. This creates two, completely different reasons for class to change.

You might benefit from exploring DataMapper pattern.

tereško
  • 58,060
  • 25
  • 98
  • 150
  • I'll have a look at the DataMapper pattern, thanks. But why is it so bad that the class can do a query to 'fill itself'? I see how it can make testing harder, but what is wrong with it from a design standpoint? – Rijk Sep 14 '11 at 09:47
  • @Rijk can a Car add an Engine to itself ? This is less of a "design" issue and more of "common sense" thing. – tereško Sep 14 '11 at 10:28
  • wrong analogy about car and engine. Model can work with DB without DataMapper and can work with him - nothing wrong in both cases. It's only means, if we can make code reuse in this case with DataMapper or not. – OZ_ Sep 14 '11 at 11:32
  • @OZ_ , noone said anything about anything about *models*. But in this example the model would be *Automobile Plant* ( plant - another word for factory .. to avoid confusion ). – tereško Sep 14 '11 at 11:35
  • @tereško, sorry, I still can't get your idea. Maybe you can explain it more detailed? – OZ_ Sep 14 '11 at 11:38
  • @OZ_ ask me in chat or read [this comment](http://stackoverflow.com/questions/5863870/mvc-how-should-a-model-be-structured/5864000#5864000) (only replace 'book' with 'car') .. and i repeat: the question had nothing to do with models. – tereško Sep 14 '11 at 11:50
1

Maybe I'll just kick in the dark with this (cause I'm not an expert) but:

Methods load() and save() inside domain objects is called Active Record (Another description). This is not bad (altough I dislike it) because people that will maybe work after or with you will have less problems figuring out how to persist those objects.

About other methods. It's not that bad if it's in objects domain and it represents objects behaviour. If designed well it can be very good. Domain driven design encourages using rich domain model which is opposite of anemic domain model. An anemic domain model has domain objects that only have properties and getters and setters. So as long as it's in domain of your object, putting additional methods in it is not considered bad.

This is as far as I understand those concept from the books and articles I've read..

Hope it helps..

Matjaz Muhic
  • 5,328
  • 2
  • 16
  • 34
  • Thanks! Could you maybe share some links to interesting books and articles? – Rijk Sep 14 '11 at 09:35
  • [Best DDD Book](http://domaindrivendesign.org/books/evans_2003), [Anemic domain model](http://martinfowler.com/bliki/AnemicDomainModel.html), [Domain model](http://martinfowler.com/eaaCatalog/domainModel.html), ... – Matjaz Muhic Sep 14 '11 at 09:43
1

What you describe is an ActiveRecord and it's well known that it violates SRP. Also, ActiveRecord only works well when the table rows match the object closely. Once Impedance Mismatch gets too big, it will make changes to the system more difficult later.

It's not necessarily bad OOP, but it is a form of Technical Debt because of the lack of separation between persistence logic and domain logic. Violating any of the SOLID principles will usually lead to hard to change code, fragile code, non-reusable code.

A few of those debts are not an issue. It's when those debt accumulate interest, e.g. when they start to ripple into other design decisions. In other words, when you notice that it gets more difficult to change the system, try to pay back some debts, e.g. refactor to a more maintainable solution.

Gordon
  • 312,688
  • 75
  • 539
  • 559
0

I think it's important to stop thinking that Model should be only layer between logic and database. Model can work with database and with other models, all logic should be in Models.
I think there is two ways:

  1. your Model could return array of ID's in getContributors() method, and you could create new object (Factory maybe), which will convert these ID's to objects.
  2. your Model could return array of objects, but without using new keyword, but through the Factory or Dependencies Container (I prefer DC).
OZ_
  • 12,492
  • 7
  • 50
  • 68
  • Where the hell did you see models now ?!? An noone here sees models are layers between logic and database. Some people just want to put the logic and sql in one place , hence the activerecord (anti)pattern. – tereško Sep 14 '11 at 11:45
  • @tereško, let will be logic and sql in one place - nothing wrong with it. And don't be so nervous. – OZ_ Sep 14 '11 at 11:52
  • which would mean that, that class would have to change either if DB structure changes or the logic of class .. thus breaking SRP ( which is what the topic was actually about ). – tereško Sep 14 '11 at 12:05
  • DB is place where data of object stored, so DB structure should reflect it and should be changed only when object changes. – OZ_ Sep 14 '11 at 12:29