Słów kilka o czytelności kodu

Z wielu stron można usłyszeć oczywiste twierdzenie, że powinniśmy pisać czytelny kod. Przyjmujemy to do wiadomości, uznajemy za oczywistą prawdę, po czym… siadamy i wracamy do tworzenia takiego samego bałaganu, jak przedtem.

Do popełnienia tego wpisu skłonił mnie post na Stack Overflow, zawierający z pozoru proste pytanie o obietnice (Promise) w JavaScripcie. Przytoczę tu całość kodu umieszczonego we wpisie:

exports.addBrand = function (user, data) {
    /** Mandatory data **/
    let name = "'" + data.brand.name + "'"
    let idAdvertiser = data.brand.idAdvertiser
    /** Optional data **/
    let isDefaultBillingEntity = data.brand.isDefaultBillingEntity === undefined ? null : data.brand.isDefaultBillingEntity
    if (name === undefined || idAdvertiser === undefined) return Promise.reject(new Error('A Brand name and an advertiser id should be provide'))
    return tools.execSQLQuery(query)
    .then(result => result[0])
    .catch(err => console.log(err))
var addBrand = (req, res) => {
  Brand.addBrand(req.user, req.body)
    .then(result => res.status(201).json({success: true, ui_info: 'Brand created.', ui_type: 'good', data: {brand: result}}))
    .catch(err => res.status(403).json({success: false, message: err}))
}

Sam w życiu bym nie napisał takiego potworka, a już szczególnie prosząc o pomoc na Stack Overflow, jednak notorycznie widzę podobne straszydła. Jednak wracając do postu, co było problemem? Otóż w ostatniej linii drugiego fragmentu pole message zawierało nieprawidłową wartość.

Moją reakcją było napisanie w komentarzu, że aby message miało wartość, w pierwszym fragmencie należy rzucić wyjątek w .catch(), a nie tylko go wylogować. Byłem pewien, że to rozwiąże problem biednego pytającego. O, ja naiwny! Okazało się, że intencja była zupełnie inna, jednak nie była oczywista z uwagi na brak czystego kodu.

A co to w ogóle jest ten „czysty kod”? Grady Booch następującymi słowami określił to, czym dla niego jest czysty kod (fragment zaczerpnięty z książki „Czysty Kod” Roberta C. Martina, tłumaczenie: Paweł Gonera):

Czysty kod jest prosty i bezpośredni. Czysty kod czyta się jak dobrze napisaną prozę. Czysty kod nigdy nie zaciemnia zamiarów projektanta; jest pełen trafnych abstrakcji i prostych ścieżek sterowania.

I zaiste, każdy z nas lubi, kiedy kod jest czytelny i zrozumiały bez wnikliwej analizy i dedukcyjnego dochodzenia, „co autor miał na myśli”. Sami jednak często piszemy kod, którego intencja wcale nie jest oczywista.

Wracając do postu ze Stack Overflow, co okazało się intencją pytającego? Otóż catch() z drugiego fragmentu kodu otrzymywał odrzuconą obietnicę z tej linijki:

if (name === undefined || idAdvertiser === undefined) return Promise.reject(new Error('A Brand name and an advertiser id should be provide'))

Kto zwrócił na to uwagę? Na pewno nie ja. Problem jednak okazał się trywialny: nieprawidłowa wartość wzięła się stąd, że zamiast err.message (a więc domyślnej właściwości każdego obiektu typu Error), pytający używał całego obiektu err jako wiadomości, co oczywiście skutkowało nieoczekiwaną wartością.

Co zatem jest nie tak z pytaniem, a w zasadzie z przytoczonym w nim kodem?

Po pierwsze: formatowanie. Źle sformatowany kod czyta się źle, łatwo pominąć ważne informacje. Jak zatem powinien wyglądać kod w przytoczonym pytaniu (pomijam już brak średników i wcięcia na dwie spacje, za które najchętniej obcinałbym palce tępym narzędziem)?

exports.addBrand = function (user, data) {
    /** Mandatory data **/
    let name = "'" + data.brand.name + "'"
    let idAdvertiser = data.brand.idAdvertiser

    /** Optional data **/
    let isDefaultBillingEntity = data.brand.isDefaultBillingEntity === undefined ?
      null :
      data.brand.isDefaultBillingEntity

    if (name === undefined || idAdvertiser === undefined) {
        return Promise.reject(new Error('A Brand name and an advertiser id should be provide'))
    }

    return tools.execSQLQuery(query)
        .then(result => result[0])
        .catch(err => console.log(err))
var addBrand = (req, res) => {
  Brand.addBrand(req.user, req.body)
    .then(result => res.status(201).json({success: true, ui_info: 'Brand created.', ui_type: 'good', data: {brand: result}}))
    .catch(err => res.status(403).json({success: false, message: err}))
}

Od razu lepiej. Widać już, że w funkcji addBrand dwukrotnie pojawia się return, łatwiej więc dojść do źródła problemu. Jednak dalej jesteśmy daleko od „dobrej prozy”, a także od jednej ze wskazówek do tworzenia pytań na Stack Overflow, proszącego o minimalny, kompletny i weryfikowalny przykład. Taki przykład powinien być pozbawiony zbędnego szumu. Tu zaś większość przytoczonego kodu nie ma żadnego znaczenia dla istoty problemu i tylko rozprasza starającego się pomóc czytelnika. Pytający zdawał sobie sprawę, że to linijka zawierająca return Promise.reject() jest ważna, zaznaczył to wszak w komentarzu. Usuńmy zatem wszystko to, co zbędne i co z powodzeniem mógł usunąć pytający:

exports.addBrand = function (user, data) {
    return Promise.reject(new Error('A Brand name and an advertiser id should be provide))
}
var addBrand = (req, res) => {
  Brand.addBrand(req.user, req.body)
    .catch(err => res.json({message: err}))
}

Stąd już niedaleka droga to minimalnego, kompletnego i weryfikowalnego przykładu, który jest tak oczywisty i tak czytelny, że bardziej się chyba nie da:

Promise.reject(new Error("foo"))
    .catch(err => console.log(err));

Pytanie zaś można skondensować do „Dlaczego ten kod nie wylogowuje wartości "foo"?”. Na tak postawione pytanie odpowiedzieć da się natychmiast, bez zbędnego angażowania szarych komórek. Dodatkowo, najprawdopodobniej samo przygotowanie takiego przykładu naprowadziłoby autora pytania na prawidłowe rozwiązanie.

Kończąc zatem apeluję do wszystkich: szanujcie czas swój i innych, piszcie kod taki, który dokładnie, bez zbędnego szumu ilustruje wasze intencje lub problemy. Będziecie zaskoczeni, jak dużo można w ten sposób zdziałać.


Napisz komentarz


Szukaj wpisów


Chmura tagów