Open - close principle doesn't encourage particular design - e.g. it doesn't say that it should be achieved through inheritance. Unfortunately, when you're into OOP, sometimes classes and inheritance and all that stuff cover the real solution. Imho, in example provided Image content type should not be hard-coded in particular classes. For me, content type is just a resource. For example, if you want to implement different language versions, you do it using resources, not through inheritance. You don't implement abstract Caption class and then EnglishCaption, FrenchCaption or PolishCaption. The same goes with content type of an image.
If you create base classes and mark methods as virtual (or you don't mark them as virtual because they are virtual by default like in Java), the point of doing it is to implement different behavior in particular classes. If you do it just to return different string and you don't implement any different logic, then maybe there is something wrong with the design.
Ok, that's the most important problem with the given example, but there also some more:
- abstract class is called AnImage, ImageBase or sth similar would be far better
- in every SendFileHeader in every derived class you are forced to write header("some type"), GetContentType as an abstract method and implementing SendFileHeader in abstract class as header(GetContentType()) would allow avoiding logic duplication
- and there is that SendToBrowserFunction that was better before the refactoring than after the refactoring, I'll describe it in more details below
Before the refactoring SendToBrowser looked like this:
public function SendToBrowser()
{
$strFileExtention = $this->GetFileExtention();
switch ( $strFileExtention )
{
case ‘gif’ :
header("Content-type: image/gif");
break;
case ‘jpg’ :
header("Content-type: image/jpg");
break;
}
$strFileContent = file_get_contents($this->fPath);
echo $strFileContent;
die();
}
and after the refactoring it looks like this:
public function SendToBrowser()
{
$this->SendFileHeader();
$strFileContent = file_get_contents($this->fPath);
echo $strFileContent;
die();
}
Before and after refactoring it has this nasty echo and even worse die. Yep, there is also that header function, but I guess changing these methods would be scheduled for further refactoring, because the author writes:
Our first implementation of Image class did not follow one other object orientated design principle. The Single Responsibility Principle.
Ok, let's get back to that switch that didn't follow open - close principle. Am I wrong or the switch logic looks for me like:
header("Content-type: image/" . $this->GetFileExtention());
What is more, the author states:
Why this class breaks the open-closed rule? If we need to send PNG image, we would need to change behaviour of SendToBrowser method and add new case to our switch statement.
Well, if we got rid of switch statement, the above code would also work for PNG. And if we came up to the conclusion that it doesn't work for some particular extension, I still think it should be implemented as a resource (some config file), not using inheritance.
Furthermore, GetFileExtension should be moved to File class, not completely removed.
I would also like to share some general thoughts. Imho, interfaces provide much better decoupling than abstract classes, especially in languages where you can inherit only one base class. Creating abstract class is more risky than creating interface. Also, too many abstract methods in abstract class lead to derived classes that have empty implementation of these methods. Creating interfaces is easier and more flexible.
Interfaces are also essential to loose coupling and technology that has been a real buzzword for some time now - dependency injection.
Eventually, I want to say that this post is not criticism of the author of the Image example. I'm quite sure that he understands open - close principle really well, but he just chose a wrong example. Or maybe I'm totally wrong and the cure is worse than evil? If so Hit me! Come on, hit me!
