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.

One of the devs that works with me is following the Open/Closed Principle to add functionality to a Model by extending from our framework ORM.

class BaseModel extends ORM {
   ...
}

All our models are now inheriting BaseModel, and this model works well in the context of our application.

class Product extends BaseModel {
  ...
}

The problem comes when we tried to use the models as if they where extending directly from the ORM, use some of the most basic functionality:

$product = Product::factory();
$product->name = 'foo';
$product->price = 9.99;
$product->save();

This code should have worked well but it failed because now the factory method requires a list of parameters.

I think that as long as the models works well within the context of our product we are good; but I also have mixed feelings about not supporting some of the basic core functionality of the parent class.

Thoughts?

share|improve this question

migrated from codereview.stackexchange.com Apr 4 '13 at 16:20

This question came from our site for peer programmer code reviews.

3  
Is the Product class hiding the BaseModel::factory method? Has your coworker seen this: en.wikipedia.org/wiki/Liskov_substitution_principle ? –  Steve Evers Apr 4 '13 at 16:27
    
See also stackoverflow.com/tags/lsp/info –  Brian Apr 4 '13 at 16:31
4  
All functionality inherited by the base class should work as it should be expected. That's part of the principle of least surprise and important for integration with other components which are designed for working with the base class. –  Philipp Apr 4 '13 at 16:32
2  
1  
All SOLID principles must be applied at the same time. You simply cannot pick one and use only that. And L is for Liskov Subsitution Principle. –  Euphoric Apr 5 '13 at 16:09

3 Answers 3

up vote 4 down vote accepted

Any public accessible method is part of a contract on how things can be used. By requiring parameters on a descendent class' method, this contract is violated (see the already mentiond LSP).

Nevertheless, sometimes it is necessary to add parameters. In such case, the overriding method should still be able to use the original signature. In your example, this can be achieved by assigning reasonable default values to the parameters in BaseModel::factory().

share|improve this answer

In general, this really isn't a fundamental issue. Making child classes override functionality from their parents is a common and often-desirable trait of inheritance. It is needed especially if you want to do anything polymorphic.

But this case is a little different -- this is an issue of changing the inheritance chain of your classes. So really, it's the changes to the parent (BaseModel) that are the issue.

It sounds like your BaseModel class has changed (overridden) the static factory() method by adding additional required parameters. Assuming there is some good reason for the new parameter list, I think this is an OK change. It's just that these types of changes can have a real ripple effect to downstream classes.

But here's the real question: why are you relying on Product to inherit a static method? That seems a little strange to me, but it also allows for a potential fix. Just override factory() in your Product class so it doesn't require parameters, if possible. It can then call other factory() methods of the parent classes as needed.

share|improve this answer

As nibra already stated, you have a contract for this public accessible method and you're not allowed to break it.

A really simple rule is, for an extended class, you can extend the type of input you accept, but be more specific on the output, so you don't break the contract.

share|improve this answer

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.