Pourquoi ma classe est-elle pire que la hiérarchie des classes dans le livre (débutant OOP)?

11

Je lis des objets PHP, des modèles et de la pratique . L'auteur essaie de modéliser une leçon dans un collège. L'objectif est d'afficher le type de leçon (conférence ou séminaire), et les frais pour la leçon selon qu'il s'agit d'une leçon horaire ou à prix fixe. La sortie doit donc être

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

lorsque l'entrée est la suivante:

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

J'ai écrit ceci:

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 />";
}

Mais selon le livre, je me trompe (je suis presque sûr que je le suis aussi). Au lieu de cela, l'auteur a donné une grande hiérarchie de classes comme solution. Dans un chapitre précédent, l'auteur a déclaré les «quatre panneaux indicateurs» suivants comme le moment où je devrais envisager de changer la structure de ma classe:

Le seul problème que je peux voir, ce sont les déclarations conditionnelles, et cela aussi de manière vague - alors pourquoi refactoriser cela? À votre avis, quels problèmes pourraient survenir à l'avenir que je n'ai pas prévus?

Mise à jour : j'ai oublié de mentionner - c'est la structure de classe que l'auteur a fournie comme solution - le modèle de stratégie :

Le modèle de stratégie

Aditya MP
la source
29
N'apprenez pas la programmation orientée objet à partir d'un livre PHP.
giorgiosironi
4
@giorgiosironi J'ai abandonné l'évaluation des langues. J'utilise PHP quotidiennement et c'est donc le plus rapide pour moi d'apprendre de nouveaux concepts. Il y a toujours des gens qui détestent un langage (Java: slidesha.re/91C4pX ) - certes, il est plus difficile de trouver les haineux Java que les dissers PHP, mais quand même. Il peut y avoir des problèmes avec l'OO de PHP, mais pour le moment je ne peux pas épargner le temps d'apprendre la syntaxe Java, la "voie Java" et l' OOP. En outre, la programmation de bureau m'est étrangère. Je suis une personne web complète. Mais avant de pouvoir accéder à JSP, vous devez connaître le Java de bureau (pas de citations, je me trompe?)
Aditya MP
Utilisez des constantes pour "taux fixe" / "taux horaire" et "séminaire" / "lecture" au lieu de chaînes.
Tom Marthenal

Réponses:

19

C'est drôle que le livre ne l'énonce pas clairement, mais la raison pour laquelle il favorise une hiérarchie de classe plutôt que si les instructions à l'intérieur de la même classe est probablement le principe Open / Closed Cette règle de conception logicielle largement connue stipule qu'une classe doit être fermée à modification mais ouvert à l'extension.

Dans votre exemple, l'ajout d'un nouveau type de leçon signifierait changer le code source de la classe Lesson, le rendant fragile et sujet à régression. Si vous aviez une classe de base et un dérivé pour chaque type de leçon, il vous suffirait simplement d'ajouter une autre sous-classe, qui est généralement considérée comme plus propre.

Vous pouvez mettre cette règle dans la section "Déclarations conditionnelles" si vous voulez, mais je considère que le "panneau" est un peu vague. Si les instructions ne sont généralement que des odeurs de code, des symptômes. Ils peuvent résulter d'une grande variété de mauvaises décisions de conception.

guillaume31
la source
3
Ce serait une bien meilleure idée d'utiliser une extension fonctionnelle plutôt que l'héritage.
DeadMG
6
Il existe certainement d'autres approches / meilleures au problème. Cependant, il s'agit clairement d'un livre sur la programmation orientée objet et le principe ouvert / fermé m'a simplement semblé être la manière classique d'aborder ce genre de dilemme de conception en OO.
guillaume31
Que diriez-vous de la meilleure façon d'aborder le problème ? Il est inutile d'enseigner une solution inférieure simplement parce que c'est OO.
DeadMG
16

Je suppose que ce que le livre vise à enseigner est d'éviter des choses comme:

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

Mon interprétation du livre est que vous devriez avoir trois classes:

  • RésuméLeçon
  • HourlyRateLesson
  • FixedRateLesson

Vous devez ensuite réimplémenter la costfonction sur les deux sous-classes.

Mon opinion personnelle

pour des cas simples comme celui-ci: ne le faites pas. Il est étrange que votre livre favorise une hiérarchie de classe plus profonde pour éviter de simples déclarations conditionnelles. De plus, avec votre spécification d'entrée, il Lessondevra s'agir d'une usine avec une répartition qui est une instruction conditionnelle. Ainsi, avec la solution livre, vous aurez:

  • 3 classes au lieu de 1
  • 1 niveau d'héritage au lieu de 0
  • le même nombre d'instructions conditionnelles

C'est plus de complexité sans avantage supplémentaire.

Simon Bergot
la source
8
Dans ce cas , oui, c'est trop de complexité. Mais dans un système plus grand, vous ajouterez plus de leçons, comme SemesterLesson, MasterOneWeekLessonetc. Une classe racine abstraite, ou mieux encore une interface, est définitivement la voie à suivre. Mais lorsque vous n'avez que deux cas, c'est à la discrétion de l'auteur.
Michael K
5
Je suis d'accord avec vous en général. Cependant, les exemples pédagogiques sont souvent artificiels et sur-architecturés afin d'illustrer un point. Vous ignorez d'autres considérations pendant un moment afin de ne pas confondre le problème en question et introduisez ces considérations plus tard.
Karl Bielefeldt
+1 Belle ventilation approfondie. Le commentaire, «C'est plus de complexité sans avantage supplémentaire» illustre parfaitement le concept de «coût d'opportunité» dans la programmation. Théorie! = Pratique.
Evan Plaice
7

L'exemple dans le livre est assez étrange. D'après votre question, la déclaration originale est:

L'objectif est d'afficher le type de leçon (conférence ou séminaire) et les frais de la leçon selon qu'il s'agit d'une leçon à l'heure ou à prix fixe.

ce qui, dans le cadre d'un chapitre sur la POO, signifierait que l'auteur souhaite probablement que vous ayez la structure suivante:

+ abstract Lesson
    - Lecture inherits Lesson
    - Seminar inherits Lesson

+ abstract Charge
    - HourlyCharge inherits Charge
    - FixedCharge inherits Charge

Mais vient ensuite l'entrée que vous avez citée:

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

c'est-à-dire quelque chose qui s'appelle du code typé chaîne et qui, honnêtement, dans ce contexte lorsque le lecteur est censé apprendre la POO, est horrible.

Cela signifie également que si j'ai raison sur l'intention de l'auteur du livre (c'est-à-dire la création des classes abstraites et héritées que j'ai énumérées ci-dessus), cela conduira à du code dupliqué et assez illisible et laid:

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.

Quant aux "poteaux indicateurs":

  • Duplication de code

    Votre code n'a pas de duplication. Le code que l'auteur attend de vous que vous écriviez le fait.

  • La classe qui en savait trop sur son contexte

    Votre classe passe simplement des chaînes de l'entrée à la sortie et ne sait rien du tout. En revanche, le code attendu peut être critiqué pour en savoir trop.

  • The Jack of All Trades - Des classes qui essaient de faire beaucoup de choses

    Encore une fois, vous ne faites que passer des chaînes, rien de plus.

  • Expressions conditionnelles

    Il y a une instruction conditionnelle dans votre code. Il est lisible et facile à comprendre.


Peut-être, en fait, l'auteur s'attendait à ce que vous écriviez un code beaucoup plus refactorisé que celui avec six classes ci-dessus. Par exemple, vous pouvez utiliser le modèle d'usine pour les leçons et les frais, 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`.

Dans ce cas, vous vous retrouverez avec neuf classes, qui seront un parfait exemple de surarchitecture .

Au lieu de cela, vous vous êtes retrouvé avec un code propre et facile à comprendre, qui est dix fois plus court.

Arseni Mourzenko
la source
Wow, vos suppositions sont précises! Regardez l'image que j'ai téléchargée - l'auteur a recommandé exactement les mêmes choses.
Aditya MP
J'aimerais tellement accepter cette réponse, mais j'aime aussi la réponse de @ ian31 :( Oh, qu'est-ce que je fais!
Aditya MP
5

Tout d'abord, vous pouvez changer les "nombres magiques" comme 30 et 5 dans la fonction cost () en variables plus significatives :)

Et pour être honnête, je pense que vous ne devriez pas vous en préoccuper. Lorsque vous devrez changer de classe, vous le changerez. Essayez d'abord de créer de la valeur, puis passez à la refactorisation.

Et pourquoi pensez-vous que vous vous trompez? Ce cours n'est-il pas "assez bon" pour vous? Pourquoi vous inquiétez-vous pour un livre;)

Franc Michal
la source
1
Merci de répondre! En effet, le refactoring viendra plus tard. Cependant, j'ai écrit ce code pour apprendre, essayer de résoudre le problème présenté dans le livre à ma manière et voir comment je me trompe ...
Aditya MP
2
Mais vous le saurez plus tard. Quand cette classe sera utilisée dans d'autres parties du système et que vous devrez ajouter quelque chose de nouveau. Il n'y a pas de solution miracle :) Quelque chose peut être bon ou mauvais, cela dépend du système, des exigences, etc. Pendant l'apprentissage de la POO, vous devez beaucoup échouer: P Ensuite, avec le temps, vous remarquerez des problèmes et des schémas courants. Tbh cet exemple est trop simple pour faire quelques conseils. Ohh je recommande vraiment ce poste de Joel :) Les astronautes de l'architecture prennent le relais de joelonsoftware.com/items/2008/05/01.html
Michal Franc
@adityamenon Alors votre réponse est "le refactoring viendra plus tard". N'ajoutez les autres classes qu'une fois qu'elles sont nécessaires pour simplifier le code - généralement, c'est à ce moment que le troisième cas d'utilisation similaire arrive. Voir la réponse de Simon.
Izkata
3

Il n'est absolument pas nécessaire d'implémenter des classes supplémentaires. Vous avez un petit MAGIC_NUMBERproblème dans votre cost()fonction, mais c'est tout. Tout le reste est une sur-ingénierie massive. Cependant, il est malheureusement courant de donner de très mauvais conseils - Circle hérite de Shape, par exemple. Il n'est certainement pas efficace de dériver une classe pour l'héritage simple d'une fonction. Vous pouvez utiliser une approche fonctionnelle pour le personnaliser à la place.

DeadMG
la source
1
C'est intéressant. Pouvez-vous expliquer comment vous feriez cela avec un exemple?
guillaume31
+1, je suis d'accord, aucune raison de sur-ingénierie / compliquer un si petit peu de fonctionnalité.
GrandmasterB
@ ian31: Faites quoi, spécifiquement?
DeadMG
@DeadMG "utilise une approche fonctionnelle", "utilise une extension fonctionnelle plutôt que l'héritage"
guillaume31