Clean code vs Philosophy of software design

Nedávno jsem narazil na diskuzi mezi Johnem Ousterhoutem (autorem A Philosophy of software design) a Robertem “Uncle Bob” Martinem (autorem Clean code).

Diskuze mě zaujala, jelikož oba autoři prezentují mnoho podivných argumentů a tímto článkem bych na ně rád reagoval. Nebudu rozebírat úplně všechno, co je v diskuzi napsáno, ale pouze několik zajímavých částí.

Diskuzi Boba a Johna není nutné číst pro pochopení tohoto článku.

Názory autorů

Začneme rychlým přehledem názorů obou autorů.

John Ousterhout:

• Preferuje delší metody
• Preferuje více komentářů v kódu
• Nemá rád dlouhé názvy metod a proměnných – preferuje kratší názvy v kombinaci s komentáři

Uncle Bob:

• Preferuje spoustu krátkých metod
• Vyhýbá se téměř všem komentářům v kódu, ale uznává, že komentáře jsou v některých situacích důležité
• Preferuje dlouhé názvy metod a proměnných

AddSongToLibrary

V jedné části diskuze Uncle Bob říká, že následující kód je velice dobrou abstrakcí a neví, jak by ho komentář mohl zlepšit:

addSongToLibrary(String title, String[] authors, int durationInSeconds);

John Ousterhout reaguje tím, že komentář je potřeba, a uvádí následující důvody:

1. Is there any expected format for an author string, such as “LastName, FirstName”?
2. Are the authors expected to be in alphabetical order? If not, is the order significant in some other way?
3. What happens if there is already a song in the library with the given title but different authors? Is it replaced with the new one, or will the library keep multiple songs with the same title?
4. How is the library stored (e.g. is it entirely in memory? saved on disk?)? If this information is documented somewhere else, such as the overall class documentation, then it need not be repeated here.

Podle mě nejsou komentáře vhodným řešením v (téměř) žádném z Johnem vyjmenovaných případů. Zkusme rozebrat jednotlivé body podrobněji.

1. Formát parametru “authors”

“Is there any expected format for an author string, such as “LastName, FirstName”?”.

Toto je opravdu validní připomínka. Správným řešením ale není použít komentář.

Správným řešením je jedna z následujících možností:

• Změnit metodu na addSongToLibrary(String title, String[] firstNamesOfAuthors, int durationInSeconds);
• Vytvořit objekt FullName a předat ho – addSongToLibrary(String title, FullName[] authors, int durationInSeconds); a nechat metodu, ať si vybere, co potřebuje.
• Vytvořit objekt Author a předat ho – addSongToLibrary(String title, Author[] authors, int durationInSeconds);
• Vytvořit objekt FirstName nebo SecondName a předat ho – addSongToLibrary(String title, FirstName[] authors, int durationInSeconds);

Všechny varianty jsou lepší než komentář, jelikož komentáře si programátor nemusí všimnout.

Proč si programátor nemusí všimnout komentáře, se můžete dočíst v mém článku o komentářích

2. Autoři v abecedním pořadí

“Are the authors expected to be in alphabetical order? If not, is the order significant in some other way?”.

Tohle je velice divný argument. Proč by metoda addSongToLibrary nemohla dělat řazení:

public void addSongToLibrary(
  String title,
  String[] authors,
  int durationInSeconds)
{
  var sortedAuthors = SortAuthors(authors);
  // ... do logic with sortedAuthors
}

Pokud z nějakého důvodu (performance) není možné, aby metoda addSongToLibrary seřadila autory po zavolání, tak můžeme vytvořit objekt AuthorsInAlphabeticalOrder a upravit metodu na addSongToLibrary(String title, AuthorsInAlphabeticalOrder authors, int durationInSeconds);

Každopádně komentář není dobrým řešením, jelikož komentáře si programátor nemusí všimnout.

3. Duplicitní songy

“What happens if there is already a song in the library with the given title but different authors? Is it replaced with the new one, or will the library keep multiple songs with the same title?

V tomto případě je velice důležité, jakou aplikaci píšeme. Řekněme, že píšeme:

Offline přehrávač hudby

Aplikace pro offline přehrávač hudby (Aimp, VLC) funguje následujícím způsobem:

• Uživatel si stáhne hudbu z internetu
• Po stažení si ji může přidat do knihovny naší aplikace
• Aplikace poté umožní poslechnout písničky v knihovně aplikace

Co myslíte, že by aplikace měla udělat, když uživatel podruhé přidá písničku se stejným title, authors, durationInSeconds?

V reálném světě může autor vydat více písniček se stejným názvem a délkou. Aplikace tedy musí uživateli umožnit podruhé přidat stejnou písničku. Možná uživatele upozorní a požádá o potvrzení, ale ve finále musí písničku umožnit přidat.

Když se tedy John ptá: “What happens if there is already a song in the library with the given title but different authors?”, tak je odpověď jasná – písnička se do knihovny přidá podruhé.

Je komentář potřeba?

Jelikož chování může programátor jednoduše odvodit z kontextu, tak komentář nepovažuji za nutný.

Pokud se vám zdá, že komentář je potřeba, a opravdu byste ho chtěli napsat, tak pamatujte, že musí obsahovat pouze informace, které se v budoucnu nezmění, protože každý napsaný komentář zvyšuje pravděpodobnost, že ho zapomenete aktualizovat.

Osobně bych napsal komentář takto:

///<remarks>
/// Author can release multiple songs with the same title and duration
/// therefore we have to allow adding the same song multiple times.
///</remarks>
addSongToLibrary(String title, String[] authors, int durationInSeconds);

a nezmiňoval bych proměnlivé informace. Například:

• Při přidání písničky s již existujícím title se uživateli zobrazí potvrzovací dialog.
• Druhá písnička se stejným názvem se přejmenuje na title (1)
• …

4. Jak se song ukládá

“How is the library stored (e.g. is it entirely in memory? saved on disk?)? If this information is documented somewhere else, such as the overall class documentation, then it need not be repeated here.”

Veřejné API

Pokud je metoda addSongToLibrary součástí veřejného API, tak není dobré sdělovat implementační detaily, které by se mohly v budoucnu změnit. Změna těchto detailů by mohla být breaking change.

Způsob uložení dat je implementační detail a není dobré ho zmiňovat v komentáři veřejné metody.

Interní API

Aplikace obvykle mají jednu „primární“ databázi, a pokud není určeno jinak, tak se data ukládají a načítají z této databáze. Pokud se pracuje s více databázemi, tak máme dvě „dobré“ možnosti:

• Programátor pozná, s čím pracuje, podle kontextu: např. máme modul „LegacyDataLoader“, který načítá data z legacy databáze a ukládá je do primární databáze. Programátor ví, že tento modul pracuje s oběma databázemi, a není potřeba to zmiňovat v komentáři.
• Pokud není ani z kontextu jasné, s čím metoda pracuje, a je to opravdu důležitá informace pro volajícího, tak tuto skutečnost uvedeme do názvu metody: addSongToFileSystemLibrary.

Komentář není dobrým řešením, jelikož komentáře si programátor nemusí všimnout.

Dlouhé názvy metod

V další části diskuze John Ousterhout naráží na dlouhé názvy metod v třídě PrimeGenerator která byla použita jako ukázka v knize Clean Code. Zde je relevantní text z diskuze:

V první řadě musím souhlasit s Johnem Ousterhoutem – název isLeastRelevantMultipleOfLargerPrimeFactor je opravdu nepochopitelný a čtenáři nic moc neřekne. Téměř všechny názvy metod ve třídě PrimeGenerator jsou nepochopitelné:

• smallestOddNthMultipleNotLessThanCandidate
• isMultipleOfNthPrimeFactor
• isNotMultipleOfAnyPreviousPrimeFactor
• …

To, že Bob zvolil špatné názvy, ale neznamená, že dlouhé názvy jsou obecně špatné. Oproti komentářům mají dvě hlavní výhody:

Výhody dlouhých názvů oproti krátkým názvům s komentáři

1. Název má programátor vždy přímo před sebou a pravděpodobně si ho přečte. Komentáře si nemusí všimnout.
2. Komentáře si musí programátor zapamatovat, a vždy, když vidí použití proměnné, tak si na její komentář musí vzpomenout. Oproti tomu název vždy vidí před sebou.

Extrémní příklad bodu 2 můžete vidět u matematických důkazů. Například následující důkaz Pythagorovy věty z Wikipedie obsahuje pouze jednopísmenné názvy proměnných a jejich definice na začátku důkazu. Čtenář si pak musí všechny definice proměnných zapamatovat a přiřadit k proměnným.

zdroj

Johnovy argumenty proti dlouhým názvům

John Ousterhout uvádí následující argumenty proti dlouhým názvům:

• John se snaží vždy přečíst celý název, a to ho zpomaluje – to je fér argument.
• Dlouhé názvy vyžadují hodně psaní – John asi ještě neobjevil zázraky moderních IDE, které vám po prvních pár znacích napoví zbytek názvu.
• Plýtvání horizontálního prostoru – John asi žije v devadesátkách a má 14” CRT monitor s rozlišením 640x480.
• Dlouhé názvy popisují chování hůře než komentáře – to je pravda, ale předpokládá to, že si programátor komentáře přečte a zapamatuje.
• Oba autoři začali v diskuzi názvy zkracovat, a to je podle Johna indikace, že dlouhé názvy jsou nepraktické – tohle není pravda. Zkracování je pouze indikací toho, že oba ví, o čem mluví, a nemusí opakovat celý kontext. Když řeknu v konverzaci „ten kamarád“, tak to znamená, že všichni víme, o kom mluvím, a nemusím opakovat jeho jméno. Neznamená to ale, že jména lidí jsou moc dlouhá, a měli bychom je zkracovat.

Paradox krátkých názvů

John začínal celou diskuzi s Bobem následující definicí:

Skoro bych tvrdil, že si John protiřečí – chce, aby programátor držel v hlavě co nejméně informací, a přitom ho nutí pamatovat si komentáře.

Jak dlouhý by měl název být?

Osobně nemám žádné obecné pravidlo, jak dlouhý by název měl být – správné pojmenování je obtížné umění. Z praxe ale vím, že lidé často píší příliš krátké a nepochopitelné názvy. Opačný problém, kdy někdo používá až příliš dlouhé názvy, jsem zatím snad ještě ani neviděl.

Prime Generator

Ústředním tématem diskuze je třída PrimeGenerator, jejíž implementace se objevila v knize Clean Code od Uncle Boba. John Ousterhout tento kód v průběhu diskuze napadá a říká, že je špatně napsaný z různých důvodů.

Později John i Uncle Bob představí svoji vlastní verzi třídy PrimeGenerator a diskutují o tom, která je lepší. Ukážu vám všechny 3 verze – dejte každé z nich pár minut a zkuste říct, jestli aspoň tušíte, co kód dělá a jak funguje.

Já osobně jsem nebyl schopen pochopit ani jednu z těchto verzí. Trvalo mi několik hodin, než jsem celý kód pochopil.

package literatePrimes;

import java.util.ArrayList;

public class PrimeGenerator {
    private static int[] primes;
    private static ArrayList<Integer> multiplesOfPrimeFactors;

    protected static int[] generate(int n) {
        primes = new int[n];
        multiplesOfPrimeFactors = new ArrayList<Integer>();
        set2AsFirstPrime();
        checkOddNumbersForSubsequentPrimes();
        return primes;
    }

    private static void set2AsFirstPrime() {
        primes[0] = 2;
        multiplesOfPrimeFactors.add(2);
    }

    private static void checkOddNumbersForSubsequentPrimes() {
        int primeIndex = 1;
        for (int candidate = 3; primeIndex < primes.length; candidate += 2) {
            if (isPrime(candidate))
                primes[primeIndex++] = candidate;
        }
    }

    private static boolean isPrime(int candidate) {
        if (isLeastRelevantMultipleOfLargerPrimeFactor(candidate)) {
            multiplesOfPrimeFactors.add(candidate);
            return false;
        }
        return isNotMultipleOfAnyPreviousPrimeFactor(candidate);
    }

    private static boolean isLeastRelevantMultipleOfLargerPrimeFactor(int candidate) {
        int nextLargerPrimeFactor = primes[multiplesOfPrimeFactors.size()];
        int leastRelevantMultiple = nextLargerPrimeFactor * nextLargerPrimeFactor;
        return candidate == leastRelevantMultiple;
    }

    private static boolean isNotMultipleOfAnyPreviousPrimeFactor(int candidate) {
        for (int n = 1; n < multiplesOfPrimeFactors.size(); n++) {
            if (isMultipleOfNthPrimeFactor(candidate, n))
                return false;
        }
        return true;
    }

    private static boolean isMultipleOfNthPrimeFactor(int candidate, int n) {
        return candidate == smallestOddNthMultipleNotLessThanCandidate(candidate, n);
    }

    private static int smallestOddNthMultipleNotLessThanCandidate(int candidate, int n) {
        int multiple = multiplesOfPrimeFactors.get(n);
        while (multiple < candidate)
            multiple += 2 * primes[n];
        multiplesOfPrimeFactors.set(n, multiple);
        return multiple;
    }
}

package literatePrimes;

import java.util.ArrayList;

public class PrimeGenerator2 {

    /**
     * Computes the first prime numbers; the return value contains the
     * computed primes, in increasing order of size.
     * @param n
     *      How many prime numbers to compute.
     */
    public static int[] generate(int n) {
        int[] primes = new int[n];

        // Used to test efficiently (without division) whether a candidate
        // is a multiple of a previously-encountered prime number. Each entry
        // here contains an odd multiple of the corresponding entry in
        // primes. Entries increase monotonically.
        int[] multiples = new int[n];

        // Index of the last value in multiples that we need to consider
        // when testing candidates (all elements after this are greater
        // than our current candidate, so they don't need to be considered).
        int lastMultiple = 0;

        // Number of valid entries in primes.
        int primesFound = 1;

        primes[0] = 2;
        multiples[0] = 4;

        // Each iteration through this loop considers one candidate; skip
        // the even numbers, since they can't be prime.
        candidates: for (int candidate = 3; primesFound < n; candidate += 2) {
            if (candidate >= multiples[lastMultiple]) {
                lastMultiple++;
            }

            // Each iteration of this loop tests the candidate against one
            // potential prime factor. Skip the first factor (2) since we
            // only consider odd candidates.
            for (int i = 1; i <= lastMultiple; i++) {
                while (multiples[i] < candidate) {
                    multiples[i] += 2*primes[i];
                }
                if (multiples[i] == candidate) {
                    continue candidates;
                }
            }
            primes[primesFound] = candidate;

            // Start with the prime's square here, rather than 3x the prime.
            // This saves time and is safe because all of the intervening
            // multiples will be detected by smaller prime numbers. As an
            // example, consider the prime 7: the value in multiples will
            // start at 49; 21 will be ruled out as a multiple of 3, and
            // 35 will be ruled out as a multiple of 5, so 49 is the first
            // multiple that won't be ruled out by a smaller prime.
            multiples[primesFound] = candidate*candidate;
            primesFound++;
        }
        return primes;
    }
}

package literatePrimes;

public class PrimeGenerator3 {
    private static int[] primes;
    private static int[] primeMultiples;
    private static int lastRelevantMultiple;
    private static int primesFound;
    private static int candidate;

    // Lovely little algorithm that finds primes by predicting
    // the next composite number and skipping over it. That prediction
    // consists of a set of prime multiples that are continuously
    // increased to keep pace with the candidate.

    public static int[] generateFirstNPrimes(int n) {
        initializeTheGenerator(n);

        for (candidate = 5; primesFound < n; candidate += 2)
            if (candidateIsPrime())
                registerTheCandidateAsPrime();

        return primes;
    }

    private static void initializeTheGenerator(int n) {
        primes = new int[n];
        primeMultiples = new int[n];
        lastRelevantMultiple = 1;

        // prime the pump. (Sorry, couldn't resist.)
        primesFound = 2;
        primes[0] = 2;
        primes[1] = 3;

        primeMultiples[0] = -1;// irrelevant
        primeMultiples[1] = 9;
    }

    private static boolean candidateIsPrime() {
        if (candidate >= primeMultiples[lastRelevantMultiple])
            lastRelevantMultiple++;

        for (int i = 1; i <= lastRelevantMultiple; i++) {
            while (primeMultiples[i] < candidate)
                primeMultiples[i] += 2 * primes[i];
            if (primeMultiples[i] == candidate)
                return false;
        }
        return true;
    }

    private static void registerTheCandidateAsPrime() {
        primes[primesFound] = candidate;
        primeMultiples[primesFound] = candidate * candidate;
        primesFound++;
    }
}

Problémy PrimeGeneratoru

Hlavní problém

Největší problém implementací je, že popisují relativně složitý matematický algoritmus a ani jeden z autorů se nesnaží čtenáři vysvětlit matematiku nutnou k pochopení algoritmu.

Entangled metody a komentáře

John říká, že první verze kódu (z Clean Code) je špatná, protože obsahuje „Entangled“ metody – abychom pochopili, jak funguje metoda A, tak musíme vědět, jak funguje metoda B atd.

John má pravdu. Na druhou stranu ale v jeho refaktorované verzi prezentuje „Entangled“ komentáře – abychom pochopili jeho druhý komentář:

// Index of the last value in multiples that we need to consider
// when testing candidates (all elements after this are greater
// than our current candidate, so they don't need to be considered).
int lastMultiple = 0;

tak musíme vědět, jak celý algoritmus funguje, což se nedozvíme, dokud ho celý nepřečteme.

Zapomínání kontextu

Dále John kritizuje následující cyklus s tím, že programátor zapomene kontext cyklu, když začne studovat metodu isPrime:

private static void checkOddNumbersForSubsequentPrimes() {
    int primeIndex = 1;
    for (int candidate = 3; primeIndex < primes.length; candidate += 2) {
        if (isPrime(candidate))
            primes[primeIndex++] = candidate;
    }
}

John má opět pravdu.

Na druhou stranu v jeho verzi jsem měl velice podobný problém – začal jsem zapomínat, co znamenají názvy i, n a lastMultiple. Komentáře mi samozřejmě nepomohly, protože jsem je zapomněl. V jedné verzi se tedy ztrácíte ve změti metod a ve druhé zase ve změti náhodně pojmenovaných proměnných.

Nedostatek komentářů

John dále kritizuje postoj Uncle Boba ke komentářům, a říká, že jich Bob píše málo, a opět má pravdu. Mimo jiné cituje z Clean Code od Uncle Boba:

Bob sám říká, že když něco nejde vyjádřit kódem, tak je komentář nutný. I přes to ale odmítá napsat komentáře v PrimeGeneratoru. Máte pocit, že Bobův kód dostatečně vyjádřil to, že algoritmus popisuje inkrementální variaci Eratosthenova síta? Já určitě ne…

Místo toho aby vysvětlil něco tak zásadního radši napíše třeba tento komentář:

primeMultiples[0] = -1; // irrelevant

Který je naprosto nic neříkající, jelikož nevíte, proč je to irrelevantní – osobně jsem si myslel, že je to proto, že se primeMultiples[0] později na něco nastaví a -1 je pouze nedůležitá počáteční hodnota. Později jsem ale zjistil, že optimalizace použitá v tomto algoritmu úplně přeskakuje primeMultiples[0] a vůbec tuto proměnnou nepoužije.

Komentáře Johna Ousterhouta

Abych ale nekritizoval jen Boba, pojďme se podívat na Johna Ousterhouta.

John píše více komentářů, ale často jsou zvláštní:

// Number of valid entries in primes.
int primesFound = 1;

Tento komentář říká, že primesFound je počet validních záznamů v poli primes. primes je pole nalezených prvočísel. Nemám tušení, co v tomto komentáři znamená slovo „valid“ – Johnova implementace žádná nevalidní prvočísla do pole primes nepřidává. Co vůbec jsou nevalidní prvočísla?

Dále tento komentář nevysvětluje, proč je primesFound inicializováno na 1. Po chvíli přemýšlení vám to dojde, ale i tak je zvláštní, že John tento důvod nekomentuje. (Důvod je, že první prvočíslo je vždy 2, takže už na začátku máme jedno nalezené prvočíslo – dvojku.)

Občas také zapomene okomentovat kritické části kódu. Prakticky první logika, na kterou v jeho verzi narazíte, je:

if (candidate >= multiples[lastMultiple]) {
    lastMultiple++;
}

John se rozhodl tento kus kódu nekomentovat, i přes to, že je to velice specifická optimalizace, která není úplně intuitivní.

Hnidopich

Možná si říkáte, že řeším zbytečné detaily a že žádný kód není perfektní. V běžné situaci bych s vámi souhlasil, ale tohle je situace, kdy rozebíráme ukázkový příklad z knihy o tom, jak správně strukturovat kód. A nejen to – probíráme druhý a třetí refaktoring této verze. A ani po třech kolech refaktoringu nemůžu říct, že autoři vyprodukovali dobrý kód.

Moje verze PrimeGeneratoru

Abych pouze nekritizoval, tak jsem vytvořil moji verzi PrimeGeneratoru. Můžete si o ní přečíst v dalším článku.

Závěr

Osobně jsem zklamaný z obou autorů. Zdá se mi, že se oba až fanaticky drží svých pravidel a nedokáží se na problém podívat komplexně. Diskuze metod a komentářů toto skvěle vystihuje:

• Bob je zarytý odpůrce komentářů, a tak i při psaní složitého algoritmu, jako je PrimeGenerator, odmítá napsat komentář.
• John zase nemá rád dlouhé názvy a mnoho metod, a tak zarytě píše matoucí názvy a jeden dlouhý kus kódu.