11

I am reading PHP Objects, Patterns, and Practice. The author is trying to model a lesson in a college. The goal is to output the lesson type (lecture or seminar), and the charges for the lesson depending on whether it is an hourly or fixed price lesson. So the output should be

Lesson charge 20. Charge type: hourly rate. Lesson type: seminar.
Lesson charge 30. Charge type: fixed rate. Lesson type: lecture.

when the input is as follows:

$lessons[] = new Lesson('hourly rate', 4, 'seminar');
$lessons[] = new Lesson('fixed rate', null, 'lecture');

I wrote this:

class Lesson {
    private $chargeType;
    private $duration;
    private $lessonType;

    public function __construct($chargeType, $duration, $lessonType) {
        $this->chargeType = $chargeType;
        $this->duration = $duration;
        $this->lessonType = $lessonType;
    }

    public function getChargeType() {
        return $this->getChargeType;
    }

    public function getLessonType() {
        return $this->getLessonType;
    }

    public function cost() {
        if($this->chargeType == 'fixed rate') {
            return "30";
        } else {
            return $this->duration * 5;
        }
    }
}

$lessons[] = new Lesson('hourly rate', 4, 'seminar');
$lessons[] = new Lesson('fixed rate', null, 'lecture');

foreach($lessons as $lesson) {
    print "Lesson charge {$lesson->cost()}.";
    print " Charge type: {$lesson->getChargeType()}.";
    print " Lesson type: {$lesson->getLessonType()}.";
    print "<br />";
}

But according to the book, I am wrong (I am pretty sure I am, too). Instead, the author gave a large hierarchy of classes as the solution. In a previous chapter, the author stated the following 'four signposts' as the time when I should consider changing my class structure:

The only problem I can see is conditional statements, and that too in a vague manner - so why refactor this? What problems do you think might arise in the future that I have not foreseen?

Update: I forgot to mention - this is the class structure the author has provided as a solution - the strategy pattern:

The strategy pattern

3
  • 29
    Don't learn object-oriented programming from a PHP book. Apr 5, 2012 at 13:22
  • 4
    @giorgiosironi I've given up evaluating languages. I use PHP on a daily basis, and so it is the fastest for me to learn new concepts in it. There are always people who hate a language (Java: slidesha.re/91C4pX) - admittedly it's harder to find the Java haters than the PHP dissers, but still. There might be problems with PHP's OO, but for the moment I can't spare the time to learn Java syntax, the "Java way", and OOP. Besides, desktop programming is alien to me. I'm a complete web person. But before you can get to JSP, you allegedly need to know desktop Java (No citations, am I wrong?)
    – Aditya M P
    Apr 6, 2012 at 3:04
  • Use constants for "fixed rate"/"hourly rate" and "seminar"/"lecture" instead of strings. Apr 9, 2012 at 5:00

5 Answers 5

19

It's funny that the book doesn't state it clearly, but the reason why it favours a class hierarchy over if statements inside the same class is probably the Open/Closed principle This widely-known software design rule states that a class should be closed to modification but open for extension.

In your example, adding a new lesson type would mean changing the Lesson class's source code, making it fragile and regression prone. If you had a base class and one derivative for each lesson type, you would instead just have to add another subclass, which is generally considered cleaner.

You can put that rule under the "Conditional Statements" section if you will, however I consider that "signpost" to be a bit vague. If statements are generally just code smells, symptoms. They can result from a wide variety of bad design decisions.

3
  • 3
    It would be a vastly better idea to use a functional extension rather than inheritance.
    – DeadMG
    Apr 5, 2012 at 12:06
  • 6
    There are certainly other/better approaches to the problem. However, this is clearly a book about object-oriented programming and the open/closed principle just struck me as the classical way of approaching that kind of design dilemma in OO. Apr 5, 2012 at 12:36
  • How about the best way to approach the problem? There's no point teaching an inferior solution just because it's OO.
    – DeadMG
    Apr 7, 2012 at 12:44
16

I guess that what the book aims at teaching is to avoid things like:

public function cost() {
    if($this->chargeType == 'fixed rate') {
        return "30";
    } else {
        return $this->duration * 5;
    }
}

My interpretation of the book is that you should have three classes:

  • AbstractLesson
  • HourlyRateLesson
  • FixedRateLesson

You should then reimplement the cost function on both subclass.

My personal opinion

for simple cases like that: don't. It is strange that your book favors deeper class hierarchy to avoid simple conditional statements. What's more, with your input spec, Lesson will have to be a factory with a dispatch which is a conditional statement. So with the book solution you will have:

  • 3 classes instead of 1
  • 1 inheritance level instead of 0
  • the same number of conditional statements

That's more complexity with no added benefit.

3
  • 8
    In this case, yes, it's too much complexity. But in a larger system, you will be adding more lessons, like SemesterLesson, MasterOneWeekLesson, etc. An abstract root class, or better yet an interface, is definitely the way to go then. But when you have only two cases, it's up to the discretion of the author.
    – Michael K
    Apr 5, 2012 at 12:15
  • 5
    I agree with you in general. However, educational examples are often contrived and over-architected in order to illustrate a point. You ignore other considerations for a moment in order not to confuse the issue at hand, and introduce those considerations later. Apr 5, 2012 at 14:46
  • +1 Nice thorough breakdown. The comment, "That's more complexity with no added benefit" illustrates the concept of 'opportunity cost' in programming perfectly. Theory != Practicality. Apr 7, 2012 at 1:16
7

The example in the book is pretty strange. From your question, the original statement is:

The goal is to output the Lesson Type (Lecture or Seminar), and the Charges for the lesson depending on whether it is a hourly or fixed price lesson.

which, in the context of a chapter on OOP, would mean that the author probably wants you to have following structure:

+ abstract Lesson
    - Lecture inherits Lesson
    - Seminar inherits Lesson

+ abstract Charge
    - HourlyCharge inherits Charge
    - FixedCharge inherits Charge

But then comes the input you quoted:

$lessons[] = new Lesson('hourly rate', 4, 'seminar');
$lessons[] = new Lesson('fixed rate', null, 'lecture');

i.e. something which is called stringly typed code, and which, honestly, in this context when the reader is intended to learn OOP, is horrible.

This also means that if I'm right about the intention of the book author (i.e. creating those abstract and inherited classes I listed above), this will lead to duplicate and pretty unreadable and ugly code:

const string $HourlyRate = 'hourly rate';
const string $FixedRate = 'fixed rate';

// [...]

Charge $charge;
switch ($chargeType)
{
    case $HourlyRate:
        $charge = new HourlyCharge();
        break;

    case $FixedRate:
        $charge = new FixedCharge();
        break;

    default:
        throw new ParserException('The value of chargeType is unexpected.');
}

// Imagine the similar code for lecture/seminar, and the similar code for both on output.

As for the "signposts":

  • Code Duplication

    Your code doesn't have duplication. The code the author expects you to write does.

  • The Class Who Knew Too Much About His Context

    Your class just passes strings from the input to the output, and doesn't know anything at all. The expected code on the other hand can be criticized for knowing too much.

  • The Jack of All Trades - Classes that try to do many things

    Again, you are just passing strings, nothing more.

  • Conditional Statements

    There is one conditional statement in your code. It is readable and easy to understand.


Maybe, in fact, the author expected you to write a much more refactored code than the one with six classes above. For example you could use factory pattern for lessons and charges, etc.

+ abstract Lesson
    - Lecture inherits Lesson
    - Seminar inherits Lesson

+ abstract Charge
    - HourlyCharge inherits Charge
    - FixedCharge inherits Charge

+ LessonFactory

+ ChargeFactory

+ Parser // Uses factories to transform the input into `Lesson`.

In this case, you'll end up with nine classes, which will be a perfect example of an over-architecture.

Instead, you ended up with an easy to understand, clean code, which is ten times shorter.

2
  • Wow, your guesses are precise! Look at the image I uploaded - the author recommended the exact same things.
    – Aditya M P
    Apr 5, 2012 at 15:44
  • I would so love to accept this answer, but I also like @ian31's answer :( Oh, what do I do!
    – Aditya M P
    Apr 6, 2012 at 3:07
5

First of all you can change "magic numbers" like 30 and 5 in cost() function to more meaningfull variables :)

And to be honest, I think that you should not worry about this. When you will need to change the class then you will change it. Try to create value first then move onto refactorization.

And why are you thinking that you are wrong ? Isn't this class "good enough" for you ? Why are you worried about some book ;)

3
  • 1
    Thanks for answering! Indeed, refactoring will come later. However, I wrote this code for learning, trying to solve the problem presented in the book in my own way and seeing how I go wrong...
    – Aditya M P
    Apr 5, 2012 at 11:30
  • 2
    But you will know this later. When this class will be used in other parts of the system and you will have to add something new. There is no "silver bullet" solution :) Something can be good or bad it depends on the system, requirements etc. While learning OOP you have to fail a lot :P Then with time you will notice some common problems and patterns. Tbh this example is too simple to make some advice. Ohh i really recommend this Post from Joel :) Architecture astronauts take over joelonsoftware.com/items/2008/05/01.html Apr 5, 2012 at 11:33
  • @adityamenon Then your answer is "refactoring will come later". Only add the other classes once they're needed to make the code simpler - usually, that's when the 3rd similar use case comes along. See Simon's answer.
    – Izkata
    Apr 5, 2012 at 14:00
3

There's absolutely no need whatsoever to implement any additional classes. You have a bit of a MAGIC_NUMBER problem in your cost() function, but that's it. Anything else is a massive over-engineering. However, it's unfortunately common for very bad advice to be given out- Circle inherits Shape, for example. It's definitely not efficient in any way to derive a class for the simple inheritance of one function. You could use a functional approach to customize it instead.

4
  • 1
    This is interesting. Can you explain how you would do that with an example ? Apr 5, 2012 at 12:30
  • +1, I agree, no reason to over-engineer/complicate such a small bit of functionality. Apr 5, 2012 at 21:43
  • @ian31: Do what, specifically?
    – DeadMG
    Apr 7, 2012 at 12:44
  • @DeadMG "use a functional approach", "use a functional extension rather than inheritance" Apr 10, 2012 at 9:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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