Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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

share|improve this question
2  
As long as it's not a static variable everything is cool. Static methods a simple to test, and simple to trace. –  Coder Mar 15 '12 at 15:22
2  
Why not just have a ProductsReviews class? Then Product and Review stay the same. Or maybe I misunderstand. –  ElGringoGrande Mar 15 '12 at 16:54
2  
@Coder "Static methods are simple to test", really? They can't be mocked, see: googletesting.blogspot.com/2008/12/… for more details. –  StuperUser Mar 15 '12 at 17:11
    
@ElGringoGrande: Review is a new class I'd change anyway. Product should stay unchanged. –  Wolfgang Mar 15 '12 at 17:12
2  
Testing Abs() isn't the problem, testing something that depends on it is. You don't have a seam for isolating the dependant Code-Under-Test (CUT) to use a mock. This means you can't test it as an atomic unit and all of your tests become integration tests that test unit logic. A failure in a test could be in CUT or in Abs() (or its dependent code) and removes the diagnosis benefits of unit tests. –  StuperUser Mar 15 '12 at 17:51
show 5 more comments

9 Answers

up vote 5 down vote accepted

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.

share|improve this answer
1  
+1 for noting that OCP is more of an indicator of good quality, not a fast rule for any code. If you find that it is difficult or impossible to properly follow the OCP, then that's a sign your model needs to be refactored to allow for more flexible reuse. –  CodexArcanum Mar 16 '12 at 5:34
    
I chose the example of Product and Review to point out that Product is a core entity of the project whereas Review is a mere add-on. So Product is already existing, finished, and Review comes later and should be introduced without opening up existing code (incl. Product). –  Wolfgang Mar 16 '12 at 8:15
add comment

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.

share|improve this answer
1  
That's a very good point. In general, not for me though. ;-) I'm not so strict about writing unit tests that cover more than one class. They all go down to the database, that's okay for me. So I won't mock the business object or it's finder. –  Wolfgang Mar 15 '12 at 18:30
    
+1, thanks for setting the red flag for tests! I agree with @Wolfgang, usually I'm not so strict too but when I need that kind of tests I really hate static methods. Usually if a static method interacts too much with its parameters or if interacts with any other static method I prefer to make it an instance method. –  Adriano Mar 15 '12 at 18:42
    
Doesn't this depend entirely on the language being used? The OP used a Java example, but never mentioned a language in the question, nor is one specified in the tags. –  Izkata Mar 16 '12 at 3:01
    
@Izkata SOLID principles are for Object Oriented design, the unit testing principle of isolation and how static affects it is an issue across every OO language/framework that I know of. –  StuperUser Mar 16 '12 at 10:17
1  
@Izkata JS is dynamically typed, so doesn't have static, you simulate it with a closure and the singleton pattern. Reading up on Python (especially stackoverflow.com/questions/893015/…), you have to inherit and extend. Overriding isn't mocking; looks like you still don't have a seam for testing the code as an atomic unit. –  StuperUser Mar 16 '12 at 16:27
show 2 more comments

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.

share|improve this answer
    
This is how I would handle it, and I'm confused (and worried about whether my own ideas are proper) by the lack of this answer until your post. Clearly neither the class representing a report nor the representation of a product should be responsible for actually retrieving the other. Some kind of data providing services should be handling this. –  CodexArcanum Mar 16 '12 at 5:36
    
@CodexArcanum Well, you're not alone. :-) –  Eric King Mar 16 '12 at 14:49
add comment

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

share|improve this answer
    
Sounds like the DAO pattern that is very common in service/component oriented software. I like this idea because it expresses that retrieving objects is not the responsibility of these objects. However, since I prefer an object oriented way over the service oriented. –  Wolfgang Mar 15 '12 at 17:57
1  
Using an interface like IRetrieveReviews stops it being service-oriented - it doesn't determine what gets the reviews, or how, or when. Maybe it's a service with lots of methods for things like this. Maybe it's a class that does that one thing. Maybe it's a repository, or does an HTTP request to a server. You don't know. You shouldn't know. That's the point. –  Lunivore Mar 15 '12 at 18:06
    
Yeah, so it would be an implementation of the strategy pattern. Which would be an argument for a third class. (A) and (B) wouldn't support this. The finder will definitely use an ORM, so there's no reason for replacing the algorithm. Sorry if I wasn't clear about this in my question. –  Wolfgang Mar 15 '12 at 18:25
add comment

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.

share|improve this answer
    
I was also thinking about this idea of the semantics of both ends and which one is "stronger" so it would claim the finder. That's why I came up with (B). It would also help to create a hierarchy of "abstraction" of the business classes. Product was a mere basic object where Review is a higher one. So, Review may reference Product but not the other way round. This way, cycles in references between the business classes can be avoided which would be another problem on my list solved. –  Wolfgang Mar 15 '12 at 18:20
    
I think for a small set of classes it could be nice even to provide BOTH methods (A) and (B). Too many times UI isn't well known at the moment of writing this logic. –  Adriano Mar 15 '12 at 18:37
add comment

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.

share|improve this answer
    
I don't agree that having the method on Review would violate SRP. On Product, it would but not on Review. My problem there was that it is static. If I moved the method to some third class it would still be static and have the product parameter. –  Wolfgang Mar 15 '12 at 18:01
    
So you are saying that the only responsibility for the Review class, the only reason for it to change, is if forProduct static method needed to change? There is no other functionality in Review class? –  ElGringoGrande Mar 15 '12 at 19:24
    
There's a lot of stuff on Review. But I think that a forProduct(Product) would fit nicely to it. Finding Reviews depends a lot on the attributes and structure of Review (which unique identifiers, which scope changing attributes?). The Product, however, does not know and should not know about Reviews. –  Wolfgang Mar 16 '12 at 7:56
add comment

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.

share|improve this answer
    
I'm not sure I would want this. This way, I have the method on both places which is good because other developers don't need to look in both classes to see where I put it. On the other hand, I would have both disadvantages of the static method and the change of the "closed" Product class. I'm not sure the delegation helps out of the friction problem. If there was ever a reason to change the finder, it'll most certainly result in the change of the signature and hence a change in Product again. –  Wolfgang Mar 15 '12 at 18:09
    
The key of OCP is that you can replace implementations without changing your code. In practice this is closely tied to Liskov Substitution. One implementation must be substitutable for another. You might have DatabaseReviews and SoapReviews as different classes both implementing IGetReviews.getReviews and getReviewsForProducts. Then your system is Open for extension (changing how reviews are gotten) and Closed for modification (dependencies on IGetReviews don't break). That's what I mean by dependency management. In your case, you're going to have to modify your code to change its behavior. –  pfries Mar 15 '12 at 22:31
    
Isn't that the Dependency Inversion Principle (DIP) that you're describing? –  Wolfgang Mar 16 '12 at 7:47
    
They're related principles. Your substitution point is achieved by DIP. –  pfries Mar 16 '12 at 20:56
add comment

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.

share|improve this answer
add comment

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

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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