CODEWITHSUNDEEP
phpoopsingle-responsibility-principle
Rijk
Rijk

Reputation: 11301

Single Responsibility Principle in OOP

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?

Upvotes: 9

Views: 2387

Answers (5)

Gordon
Gordon

Reputation: 317217

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.

Upvotes: 1

user680786
user680786

Reputation:

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).

Upvotes: 0

Matjaz Muhic
Matjaz Muhic

Reputation: 5598

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..

Upvotes: 1

DaveFar
DaveFar

Reputation: 7467

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.

Upvotes: 5

tereško
tereško

Reputation: 58454

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.

Upvotes: 1

Related Questions