úterý 25. ledna 2011

Najdi chybu 01

Zadání

Vývojář implementuje práci s objednávkami. Potřebuje zformátovat popis objednávky pro další použití, např. odeslání emailem. K dispozici má třídy reprezentující osobu (Osoba) a objednávku (Objednavka). Rozmyslete si, co je v níže napsaném kódu špatně a proč:

public class Osoba
{
    public String Jmeno { get; set; }
    public String Prijmeni { get; set; }
    public String Titul { get; set; }
    public String VedeckaHodnost { get; set; }
}

public class Objednavka
{
    public Osoba Vystavil { get; set; }
    public Osoba Schvalil { get; set; }
    
    // ... další vlastnosti objednávky
}

   // ... někde v klientském kódu

    private String VratitPopisObjednavkyProEmail(Objednavka objednavka)
    {
        StringBuilder popisObjednavky = new StringBuilder();

        popisObjednavky.AppendFormat("Vystavil: {0}{1} {2}{3}\n", 
            (objednavka.Vystavil.Titul != null ? objednavka.Vystavil.Titul + " " : String.Empty),
            (objednavka.Vystavil.Jmeno != null ? objednavka.Vystavil.Jmeno : String.Empty),
            (objednavka.Vystavil.Prijmeni != null ? objednavka.Vystavil.Prijmeni : String.Empty),
            (objednavka.Vystavil.VedeckaHodnost != null ? ", " + objednavka.Vystavil.VedeckaHodnost  : String.Empty)
            );

        popisObjednavky.AppendFormat("Schválil: {0}{1} {2}{3}\n",
            (objednavka.Schvalil.Titul != null ? objednavka.Schvalil.Titul + " " : String.Empty),
            (objednavka.Schvalil.Jmeno != null ? objednavka.Schvalil.Jmeno : String.Empty),
            (objednavka.Schvalil.Prijmeni != null ? objednavka.Schvalil.Prijmeni : String.Empty),
            (objednavka.Schvalil.VedeckaHodnost != null ? ", " + objednavka.Schvalil.VedeckaHodnost : String.Empty)
            );

        return popisObjednavky.ToString();
    }


Řešení

Zřejmě došlo k porušení principu DRY (neopakování se). Zformátování celého jména osoby je na dvou místech v identickém tvaru a je předpoklad, že by se podobný kód mohl nadále množit. Důsledky tohoto častého programátorského nešvaru jsou:

  • Duplicitní kód. Problematické ladění. V naší ukázce je vidět, že pokud bude VedeckaHodnost == String.Empty, dojde k chybnému zformátování. Možná si toho autor na několika místech všimne a problém opraví. Ale nezapomene na některý výskyt?
  • Špatná čitelnost kódu. Informace o skutečném významu zformátování jména osoby je rozmělněna do čtyř řádků!
  • Snížení robustnosti kódu. Podařilo se Vám vyladit formátování jména osoby na všech místech aplikace. Jste spokojení. Pouze však do Té doby, než architekt systému vznese požadavek, že jméno osoby má být vždy zformátováno do tvaru Příjmení Jméno, Titul, Vědecká hodnost. To by Vám pak mohlo zkazit náladu ;-)
  • Vytvoření unit testů na správné formátování jména osoby je problematické.

Pokud si uvědomíte, že došlo současně k porušení pravidla zodpovědnosti třídy Osoba, začnete se blížit ke správnému řešení. Ano, třída Osoba by měla nabízet vlastnost CeleJmeno, v rámci které dojde k požadovanému zformátování. Dojde k úpravám, které nám kód pročistí, zlepší přehlednost a zbaví jej duplicit.

public class Osoba
{
    public String Jmeno { get; set; }
    public String Prijmeni { get; set; }
    public String Titul { get; set; }
    public String VedeckaHodnost { get; set; }

    public String CeleJmeno
    {
        get
        {
            return String.Format("{0}{1} {2}{3}",
                (String.IsNullOrEmpty(Titul) ? String.Empty : Titul + " "),
                Jmeno,
                Prijmeni,
                (String.IsNullOrEmpty(VedeckaHodnost) ? String.Empty : ", " + VedeckaHodnost)
                );
        }
    }
}

public class Objednavka
{
    public Osoba Vystavil { get; set; }
    public Osoba Schvalil { get; set; }
    
    // ... další vlastnosti objednávky
}

    // ... někde v klientském kódu

    private String VratitPopisObjednavkyProEmail(Objednavka objednavka)
    {
        StringBuilder popisObjednavky = new StringBuilder();

        popisObjednavky.AppendFormat("Vystavil: {0}\n", objednavka.Vystavil.CeleJmeno);
        popisObjednavky.AppendFormat("Schválil: {0}\n", objednavka.Schvalil.CeleJmeno); ;

        return popisObjednavky.ToString();
    }

4 komentáře:

  1. Navíc bych ještě popis objednávky vložil přímo jako property do třídy Objednávka.

    OdpovědětVymazat
  2. Petře, děkuji za pozorné přečtení a připomínku. Pokud by byl popis objednávky jednoznačně definován, pak by zřejmě měl být součástí třídy Objednavka. V tomto případě bylo formátování zamýšleno např. pro vložení do emailové komunikace. Pak ovšem název třídy SpravaObjednavek nebyl příliš vhodný. Upravil jsem.

    OdpovědětVymazat
  3. Nabízené řešení se mi nelíbí. Podle mě porušuje princip otevřenosti/uzavřenosti. V momentě, kdy bude potřeba osobní údaje formátovat jinak, nastane problém. Buď si klientská část bude muset údaje naformátovat sama nebo se bude muset přidat další metoda do třidy Osoba.

    Osobně bych do třídy "Osoba" vůbec, žádné formátování nevkládal. Spíše bych vytvořil rozhraní, např. IFormatovacOsoby, s jednou metodou 'String formatuj(Osoba)'. Jednotlivé implementace tohoto rozhraní by se pak staraly o formátování údajů pro různé výstupy.

    OdpovědětVymazat
  4. Jirko, děkuji za připomínku a vylepšený návrh. Vaše řešení je určitě robustnější. Příklad byl omezen na porušení DRY principu a na princip OCP se zapomnělo ;-)

    OdpovědětVymazat