Refactoring if-else logic per riflettere i principi OOP

Quel codice

var human = new Human();
human.IsFather = false;
human.IsMother = false;
human.IsSon = true;
human.Isdaughter = false;

var newHuman = HumanHelper.GetHuman(human);
Console.WriteLine(newHuman.WhatAmI);

Potrebbe essere semplificato in

var human = new Son();

Console.WriteLine(newHuman.WhatAmI);

So che sembra stupido, ma se si assegnano valori booleani codificati in modo fisso, si potrebbe piuttosto instanziare direttamente il tipo reale.

Se hai semplificato troppo il tuo codice reale per renderlo comprensibile a noi, ti suggerisco di aggiungere un enum che rappresenta i diversi ruoli

enum Role {
    FATHER,
    MOTHER,
    SON,
    DAUGHTER
};

E crea una fabbrica responsabile della creazione umana

public static Human create(Role role) {
    switch(role) {
        case Role.FATHER:
        case Role.MOTHER:
            return new Parent();
        case Role.SON:
            return new Son();
        case Role.DAUGHTER:
            return new Daughter();
    }
}

Pertanto, puoi sbarazzarti di questi brutti IsXXX in Human e trasformarlo in un'interfaccia

interface Human {
    string WhatAmI(); //pay attention that this method seems redundant with ToString()
    // other methods ?
}

Con classi derivate

public class Parent : Human
{
    public string WhatAmI() {
        return "parent";
    }
}

...

La cosa più importante da capire qui è che quando un codebase è ingombrato con if/else è sempre perché abbiamo fatto alcune scelte progettuali iniziali come "aggiungi un metodo IsSon , IsFather , eccetera." o ogni volta che decidiamo di passare un "flag" come parametro del metodo: write(bool shouldOverride) genera sempre un codice simile a

public void write(shouldOverride) {
    if(shouldOverride) {
        //some behaviour
    }
    else {
        //some other behaviour
    }
}

Alcune osservazioni:

1. La classe Human ha una doppia responsabilità, poiché è entrambe a specifica per il tipo di oggetti che si desidera creare e una base classe per gli oggetti effettivamente creati. Questi sono in realtà separati responsabilità e coinvolge proprietà separate. Il primo Human dovrebbe essere una classe separata chiamata qualcosa di simile %codice%. (Le proprietà HumanSpecification appartengono a IsXXX ma HumanSpecification appartiene a Human)

   E WhatAmI dovrebbe essere Human , ma questo non è possibile fintanto che lo usi anche come oggetto della specifica.

2. Le specifiche dovrebbero consentire solo configurazioni valide. Un set dei booleani non hanno ragione per esprimere un'opzione esclusiva. Per esempio. è possibile impostare sia abstract che IsMother , che non è possibile senso. Utilizza invece IsFather per le opzioni esclusive. Non è ovvio se un essere umano può essere padre e figlio allo stesso tempo, ma il la specifica lo consente.

3. La proprietà enum sembra superfluo e potenzialmente pericolosa perché potrebbe indurre i clienti a utilizzare effettivamente questa proprietà. Per scopi di debug devi utilizzare WhatAmI .

Se implementi queste tre modifiche avrai un codice molto più semplice e meno soggetto a errori.

La parte più confusa è la logica all'interno del metodo factory, in cui i flag sono combinati con un'impostazione di configurazione per produrre un'istanza.

Reverse engineering the logic form the code:

kindOfHuman |IsMother |IsFather |IsDaughter |IsSon |Result
parent      |T        |F        |*          |*     |Parent
parent      |F        |T        |*          |*     |Parent
parent      |T        |T        |*          |*     |Parent
parent      |F        |F        |*          |*     |!ERROR!
child       |T        |T        |*          |*     |!ERROR!
child       |T        |F        |*          |*     |!ERROR!
child       |F        |T        |*          |*     |!ERROR!
child       |F        |F        |T          |*     |Daughter
child       |F        |F        |F          |T     |Son
child       |F        |F        |F          |F     |!ERROR!

(dove ToString() significa qualsiasi valore di verità.) Noterai subito una logica strana: come se * sia vero , il risultato potrebbe essere ancora IsSon !

Guardando più da vicino la logica sembra che le regole in pratica distinguano solo tre casi nell'input, quindi l'input potrebbe essere semplificato in un enum con le opzioni PARENT, DAUGHTER, SON. Questo semplifica molto le regole:

kindOfHuman |spec     |Result
parent      |PARENT   |Parent
parent      |DAUGHTER |!ERROR
parent      |SON      |!ERROR
child       |PARENT   |!ERROR
child       |DAUGHTER |Daughter
child       |SON      |Son

L'output sarà lo stesso ma la logica è semplificata molto! Poiché ci sono solo tre uscite valide, credo che il più chiaro e leggibile sia un se, come:

   if (spec == PARENT && kindOfHuman == "parent") {
       return new Parent();
   } else if (spec == DAUGHTER && kindOfHuman == "child") {
      return new Daughter();
   } else if (spec == SON && kindOfHuman == "child") {
      return new Son();
   }
   throw new ArgumentException("Should not get here");

Penso che sia abbastanza leggibile, ma se prevedi di aggiungere o rimuovere spesso regole in futuro, potresti prendere in considerazione un approccio più basato sui dati, ad esempio configurando regole come:

   var rules = new[]{ 
      Rule("parent", PARENT, ()=>new Parent()), 
      Rule("child", DAUGHTER, ()=>new Daughter()), 
      Rule("child", Son, ()=>new Son()),
   }

Ma YAGNI.

refactor a code based on if/else if statements to follow closely the OOP principles

Se non ti dispiace vorrei mostrare come la doppia spedizione (o il modello di visitatore se preferisci) possa fornire alcune opzioni qui. Non userò Human come oggetto di valore di bool. In realtà non utilizzerò affatto if .

Scusate per ottenere java su tutto il c # partito qui (era tutto quello che è capitato di avere a portata di mano e io non pubblicare il codice non ho ancora testato). Confido che sarai in grado di seguirlo. Lo stesso trucco funziona bene in c #.

public class Human {
    Gender gender;
    Role role;

    public Human(Gender gender, Role role) {
        this.gender = gender;
        this.role = role;
    }

    @Override
    public String toString() {
        return whatAmI();
    }

    String whatAmI() {
        return gender.whatAmI(role);
    }

    static interface Gender {
        String whatAmI(Role role);
    }

    static interface Role {
        public String whatAmI(Male male);
        public String whatAmI(Female female);
    }

    static class Male implements Gender {
        @Override
        public String whatAmI(Role role) {
            return role.whatAmI(this);
        }
    }

    static class Female implements Gender {
        @Override
        public String whatAmI(Role role) {
            return role.whatAmI(this);
        }
    }

    static class Parent implements Role{
        @Override
        public String whatAmI(Male male) {
            return "Parent";
        }

        @Override
        public String whatAmI(Female female) {
            return "Parent";
        }
    }

    static class Child implements Role{
        @Override
        public String whatAmI(Male male) {
            return "Son";
        }

        @Override
        public String whatAmI(Female female) {
            return "Daughter";
        }        }

    public static void main (String args[]){
        System.out.println(new Human(new Female(), new Parent()).whatAmI());
        System.out.println(new Human(new Male(), new Parent()).whatAmI());
        System.out.println(new Human(new Female(), new Child()).whatAmI());
        System.out.println(new Human(new Male(), new Child()).whatAmI());
   }
}

Uscite

Parent
Parent
Daughter
Son

È un po 'troppo progettato. Capisce se sei un padre o una madre e poi continua a dimenticarsene. Mi sono ritrovato a imitare il tuo comportamento.

Credo che questo mostri un modo in cui puoi refactoring if-else-if code con i principi OOP. Sono stati rifattorizzati proprio fuori dall'esistenza. Non che questo approccio non abbia i suoi difetti. Sesso e ruolo sanno troppo l'uno dell'altro per i miei gusti. Ma è bello dire semplicemente all'umano cosa essere, piuttosto che chiedere.

Quindi hai un oggetto Human , e da quell'oggetto vuoi ottenere un oggetto di una classe derivata usando le proprietà dell'oggetto della classe base. Perché non hai appena creato un Son in primo luogo?

Inoltre, in quelle classi derivate dovresti sovrascrivere le proprietà IsMother e IsFather e Isdaughter e IsSon , o impostarle nel costruttore, altrimenti hai un Son il cui IsSon restituisce falso .

Se vuoi che un metodo statico restituisca l'oggetto desiderato, ecco cosa dovresti fare:

public static class HumanHelper
{
    enum Gender { Male, Female };

    private static string kindOfHuman = ConfigurationManager.AppSettings["kindOfHuman"];

    public static Human GetGuman(Gender gender)
    {
        if (kindOfHuman == "parent")
        {
            return new Parent();
        }
        else if (kindOfHuman == "child")
        {
            if (gender == Gender.Female)
            {
                return new Daughter();
            }
            else if (gender == Gender.Male)
            {
                return new Son();
            }
        }
        throw new ArgumentException("Should not get here");
    }
}

In tal modo, puoi rimuovere alcuni if s.