Why is my class worse than the hierarchy of classes in the book (beginner OOP)?

Question

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:

• Code duplication
• The class who knew too much about its context
• The jack of all trades - classes that try to do many things
• Conditional statements

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:

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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

Answer 5

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.