5

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?

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

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

4

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

  • 1
    But Product::factory() is a static method, not an instance method so is not part of the contract for subtypes. (unless using something very odd like smalltalk) – Ian Jan 25 '16 at 13:23
2

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.

1

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.

1

The Liskov substitution principle restricts the behavior of instance methods in subclasses. It does not say anything about class methods like Product::factory.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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