SOLID vs. static methods

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

Answer 1

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

Interface vis-a-vis SOLID

These are not mutually exclusive. The interface should express the nature of your business model ideally in business model terms. The SOLID principles is a Koan for maximizing Object Oriented code maintainability (and I mean "maintainability" in the broadest sense). The former supports the use and manipulation of your business model and the latter optimizes code maintenance.

Open/Closed Principle

"don't touch that!" is too simplistic an interpretation. And assuming we mean "the class" is arbitrary, and not necessarily right. Rather, OCP means that you have designed your code such that modifying it's behavior does not (should not) require you to directly modify existing, working code. Further, not touching the code in the first place is the ideal way to preserve the integrity of existing interfaces; this is a significant corollary of OCP in my view.

Finally I see OCP as an indicator of existing design quality. If I find myself cracking open classes (or methods) too often, and/or without really solid (ha, ha) reasons for doing so then this may be telling me I've got some bad design (and/or I don't know how to code OO).

Give it your best shot, we have a team of doctors standing by

If your requirements analysis tells you that you need to express the Product-Review relationship from both perspectives, then do so.

Therefore, Wolfgang, you may have a good reason for modifying those existing classes. Given the new requirements, if a Review is now a fundamental part of a Product, if every extension of Product needs Review, if doing so makes client code appropriately expressive, then integrate it into the Product.

Answer 2

SOLID are guidelines, so influence decision making rather than dictate it.

One thing to be aware of when using static methods is their impact on testability.

---

Testing forProduct(Product product) won't a problem.

Testing something that depends on it will be.

You won't have a seam for isolating the dependant Code-Under-Test (CUT) to use a mock, since when the application is running, the static methods necessarily exist.

Let's have a method called CUT() that calls forProduct()

If forProduct() is static you can't test CUT() as an atomic unit and all of your tests become integration tests that test unit logic.

A failure in a test for CUT, could be caused by a problem in CUT() or in forProduct() (or any of its dependent code) which removes the diagnosis benefits of unit tests.

See this excellent blog post for more detailed information: http://googletesting.blogspot.com/2008/12/static-methods-are-death-to-testability.html

---

This can lead to frustration with failing tests and abandonment of good practices and benefits that surround them.

Answer 3

If you think that the Product is the right place to find the product's Reviews, you can always give the Product a helping class to do the job for it. (You can tell because your business will never talk about a review except in terms of a product).

For instance, I would be tempted to inject something that played the role of a review retriever. I would probably give it the interface IRetrieveReviews. You can put this in the constructor of the product (Dependency Injection). If you want to change how the reviews are retrieved you can do it easily by injecting a different collaborator - a TwitterReviewRetriever or an AmazonReviewRetriever or MultipleSourceReviewRetriever or whatever else you need.

Both now have a single responsibility (being the go-to for all things product-related, and retrieving reviews, respectively), and in the future the product's behavior with respect to reviews can be modified without actually changing the product (you could extend it as a ProductWithReviews if you really wanted to be pedantic about your SOLID principles, but this would be good enough for me).

Answer 4

I would have a ProductsReview class. You say Review is new anyway. That doesn't mean it can just be anything. It still must have only one reason to change. If you change how you get the reviews for whatever reason you would have to change the Review class.

That isn't correct.

You are putting the static method in Review class because... why? Isn't that what you are struggling with? Isn't that the whole problem?

Then don't. Make a class that sole responsibility is getting the products reviews. You can then subclass it to ProductReviewsByStartRating whatever. Or subclass it to get reviews for a class of products.

Answer 5

I wouldn't put the 'Get Reviews For Product' functionality in either the Product class nor the Review class...

You have a place where you retrieve your Products, right? Something with GetProductById(int productId) and maybe GetProductsByCategory(int categoryId) and so on.

Likewise, you should have a place to retrieve your Reviews, with a GetReviewbyId(int reviewId) and maybe a GetReviewsForProduct(int productId).

When I want to change the way reviews are collected for a product, I have to change the Product class.

If you separate your data access from your domain classes, you won't need to alter either domain class when you change the way the reviews are collected.

Answer 6

Patterns and principles are guidelines, not rules written in the stone. In my opinion the question is not if it's better to follow SOLID principles or to keep a simpler interface. What you should ask yourself is what is more readable and comprehensible to most of people. Often this means that it must be as near as possible to the domain.

In this case I would prefer the solution (B) because for me the starting point is the Product, not the Review but imagine you're writing a software to manage reviews. In that case the center is the Review so solution (A) may be preferable.

When I have a lot of methods like this ("connections" between classes) I strip them all outside and I create one (or more) new static class to organize them. Usually you can see them as queries or kind of repository.

Answer 7

Your Product can just delegate to your static Review method, in which case you are providing a convenient interface in a natural location (Product.getReviews) but your implementation details are in Review.getForProduct.

SOLID are guidelines and should result in a simple, sensible interface. Alternatively, you could derive SOLID from simple, sensible interfaces. It's all about dependency management within code. The goal is to minimize the dependencies that create friction and create barriers to inevitable change.

Answer 8

I have a slightly different take on this than most of the other answers. I think that Product and Review are basically data transfer objects (DTOs). In my code I try to make my DTOs/Entities avoid having behaviors. They are just a nice API to store the current state of my model.

When you talk about OO and SOLID you're generally talking about an "object" that doesn't represent state (necessarily), but instead represents some kind of service that answers questions for you, or to which you can delegate some of your work. For instance:

interface IProductRepository
{
    void SaveNewProduct(IProduct product);
    IProduct GetProductById(ProductId productId);
    bool TryGetProductByName(string name, out IProduct product);
}

interface IProduct
{
    ProductId Id { get; }
    string Name { get; }
}

class ExistingProduct : IProduct
{
    public ProductId Id { get; private set; }
    public string Name { get; private set; }
}

Then your actual ProductRepository would return an ExistingProduct for the GetProductByProductId method, etc.

Now you're following the single responsibility principle (whatever inherits from IProduct is just holding on to state, and whatever inherits from IProductRepository is reponsible for knowing how to persist and rehydrate your data model).

If you change your database schema, you can change your repository implementation without changing your DTOs, etc.

So, in short, I guess I would choose neither of your options. :)

Answer 9

Static methods may be harder to test, but that doesn't mean you can't use them - you just need to set them up so that you don't need to test them.

Make both product.GetReviews and Review.ForProduct one line methods that are something like

new ReviewService().GetReviews(productID);

ReviewService contains all the more complex code and has an interface that is designed for testability, but is not exposed directly to the user.

If you must have 100% coverage, have your integration tests call the product/review class methods.

It might help if you think about public API design rather than class design - in that context a simple interface with logical grouping is all that matters - actual code structure matters only to developers.