Question

Here's a problem I frequently run into: Let there be a web shop project that has a Product class. I want to add a feature which allows users to post reviews to a product. So I have a Review class which references a product. Now I need a method that lists all reviews to a product. There's two possibilities:

(A)

public class Product {
  ...
  public Collection<Review> getReviews() {...}
}

(B)

public class Review {
  ...
  static public Collection<Review> forProduct( Product product ) {...}
}

From looking at the code, I'd choose (A): It's not static and it doesn't need a parameter. However, I sense that (A) violates the Single Responsibility Principle (SRP) and the Open-Closed Principle (OCP) whereas (B) doesn't:

  • (SRP) When I want to change the way reviews are collected for a product, I have to change the Product class. But there should be only one reason why to change the Product class. And that's certainly not the reviews. If I pack every feature that has something to do with products in Product, it'll soon be clattered.

  • (OCP) I have to change the Product class to extend it with this feature. I think this violates the 'Closed for change' part of the principle. Before I got the customer's request for implementing the reviews, I considered Product as finished, and "closed" it.

What is more important: following the SOLID principles, or having a simpler interface?

Or am I doing something wrong here altogether?

Result

Wow, thanks for all of your great answers! It's hard to pick one as official answer.

Let me summarize the main arguments from the answers:

  • pro (A): OCP is not a law and readability of the code matters as well.
  • pro (A): the entity relationship should be navigable. Both classes may know about this relationship.
  • pro (A)+(B): do both and delegate in (A) to (B) so Product is less likely to be changed again.
  • pro (C): put finder methods into third class (service) where it's not static.
  • contra (B): impedes mocking in tests.

A few additional things my colleges at work contributed:

  • pro (B): our ORM framework can automatically generate the code for (B).
  • pro (A): for technical reasons of our ORM framework, it will be necessary to change the "closed" entity in some cases, independently from where the finder goes to. So I won't always be able to stick to SOLID, anyway.
  • contra (C): to much fuss ;-)

Conclusion

I'm using both (A)+(B) with delegation for my current project. In a service-oriented environment, however, I'll go with (C).

No correct solution

Licensed under: CC-BY-SA with attribution
scroll top