rarouš.weblog

Poslední dobou se mi velmi často stává, že narážím na hlášky typu “od devíti refaktoruju a furt to nemá konce,” “refaktoring mi zabere asi tři dny, udělej mi na to task do backlogu” nebo „refaktoroval jsem to a nějak to přestalo fungovat.“ Tohle všechno jsou lži, ať už plynoucí z neznalosti, co to je refaktoring nebo trendy nadužívání tohoto slova v nepatřičných situacích.

Co je to refaktoring?

Refaktoring je pojem, který je definován poměrně přesně. Je to krátká atomická operace nad kódem, která nemění jeho funkci, pouze strukturu. Z definice jasně vyplývá, že věty z úvodu jsou lži.

Refaktoring patří k základní hygieně čistého kódu. Měli byste ho používat neustále. Není to věc, kterou byste měli zahrnovat do plánů jako něco extra, podobně jako testy, jsou jejich nedílnou součástí. A sluší se je zahrnout do časových odhadů jednotlivých úkolů. Ne však jako samostatné úkoly!

Říkám refaktoring, myslím redesign

Refaktoring se stal trendy pojmem. Manažeři mu nerozumí a programátoři se sním furt ohánějí. „Proč ti to trvalo tak dlouho?“ „Ještě jsem musel udělat refaktoring tohodle a tamtoho.“ Jenže jak jsme si řekli v definici, refaktoring je krátká operace! Velmi krátká, pokud máte dobré nástroje, netrvá více než pár stisků kláves. Řádově trvá jednotky sekund.

Refaktoring má navíc tu vlastnost, že i když děláte velkou sérii – provádíte redesign – můžete kdykoli přestat a aplikace stále funguje. Ano, je to tak. Opět to vyplívá jasně z definice. Pokud ne, děláte jenom redesign.

Redesign

„Musíme to přepsat.“ Ta slova nemají manažeři rádi. Znamená to spoustu promarněných (z jejich pohledu – my přece víme, že se to musí udělat a že se to vrátí) hodin/dní (někdy i měsíců a let) práce. A tak se místo redesignování začalo „refaktrovat.“ Přestaňme si lhát. I když třeba během redesignu použijeme nějaký ten refaktoring, pořád je to redesign. Redesignovat můžeme přinejmenším dvěma způsoby:

1. Hurá redesign, kdy se původní kód zahodí a napíše se to znova a občas se něco z toho původního copy&pastene. Tohle se často dnes nazývá refactoring. A většinou z toho vypadne podobná sračka, jako byla před tím, jen bude mít jiný tvar.
2. Můžeme postupně refaktorovat a časem se dostat k lepšímu designu. Ale nezapomínejte, vždycky tenhle proces můžete přerušit a aplikace funguje. Stačí, že změny jsou good enough. Váš kód nikdy nebude dokonalý.

Blíží se Vánoce, čas stresu a shonu za nejdražšími Vánočními dárky, pro naše nejdražší. Čas, kdy se ve velkém mordují kapři (navíc na veřejnosti, no fůj!) a kácí se stromky. Ano, ty které potom ověsíme nejrůznějšími koulemi a řetězy, aby pak několik týdnů opadávalo jehličí a vůbec. O tomhle ale psát nechci. :)

Dneska se podíváme na jiný úkaz. A to na problematiku zanořování kódu, který ve výsledku vypadá jako stromečky. (Pokud jste takový kód ještě neviděli, což pochybuju, tak ukázku najdete třeba tady v sekci Dark Magic.)

V čem je problém?

Možná si říkáte, „co to ten Roubíček zase vymejšlí? Vždyť takovej kód je přece v pořádku,“ (tj. dělá, to, co má.) „A navíc nemusíme chodit do lesa, protože máme stromečky pořád na očích!“ To je fakt. Jenže, ruku na srdce, dokážete odhadnout na první pohled, co takový kód vlastně dělá? Moc asi ne. A proč? Protože pět úrovní zanoření (nepočítám namespace, class ani metodu) je už trochu moc.

Jak moc? No hrozně moc. Fakt! Ono se to totiž dá krásně změřit, např. pomocí metriky Cyclomatic complexity.

Když se podíváme na tuto metriku u ukázkového příkladu, zjistíme, že metoda GetRouteData dosahuje hodnoty 17. Co to znamená? Když si přečtete odkazovaný popis metriky, dozvíte se také, že její hodnota určuje míru možného výskytu chyb. Čím větší je cyklomatická komplexita, tím více toho modul dělá a zvyšuje se riziko přehlédnutých chyb. Doporučuje se mít komplexitu do deseti a tady tuto hodnotu překračujeme takřka dvojnásobně.

It smells like refactoring time!

Refaktorovat, refaktorovat, refaktorovat

Jednoduše jsme pomocí metriky odhalili kandidáta, kde budeme kácet stromy. Když se podíváme pozorně na ukázkový příklad, můžeme si všimnout bloků kódu, které začínají komentářem. Budu se opakovat, ale nepište zbytečný kód! Tohle jsou jasné samostatné metody. Použijeme tedy refaktoring Extract method. Po extrahování čtyř metod, už se nám metoda vejde na jednu obrazovku a cyklomatická komplexita spadne na trojku. Takřka na jednu šestinu!

Huh. A to je teprve začátek.

Po prvním kole vypadá výsledek asi tak:

public override RouteData GetRouteData(HttpContextBase httpContext) {
  // Build regex
  domainRegex = CreateRegex(Domain);
  pathRegex = CreateRegex(Url);
  // Request information
  string requestDomain;
  string requestPath;
  RequestInformation(httpContext, out requestDomain, out requestPath);
  // Match domain and route
  Match domainMatch = domainRegex.Match(requestDomain);
  Match pathMatch = pathRegex.Match(requestPath);
  // Route data
  RouteData data = null;
  if (domainMatch.Success && pathMatch.Success) {
    data = new RouteData(this, RouteHandler);
    AddDefaultsFirst(data);
    IterateMatchingDomainGroups(domainMatch, data);
    IterateMatchingPathGroups(pathMatch, data);
  }
  return data;
}

Není to žádná nádhera, ale je mnohem přehlednější, než originál. A v řeči čísel:

Rozplétáme podmínky

Tak, snížili jsme komplexitu metody, ale tiše nám narostla komplexita třídy. Klidně můžeme pokračovat v cestě, to zvládneme později. Teď se zaměříme na ty velký šestky. Na první pohled cyklus se spoustou zanořených podmínek. Pryč s nimi! Teď přijde na řadu refaktoring Flatten Conditional a z vše objímajícího ifu uděláme jen guarda. Dále tu mám dva zanořený ify? Proč, když je můžeme logicky spojit operátorem &&? Směle do toho! Pro změnu použijeme refaktoring Combine Conditionals a vytvoříme jednu dlouhou a ošklivou podmínku…

Čas na refaktoring Extract method a podmínku pěkně smysluplně pojmenovat. Po pár magických kombinacích kláves se nám z týhle zrůdičky:

private static void IterateMatchingPathGroups(Match pathMatch, RouteData data) {
  for (int i = 1; i < pathMatch.Groups.Count; i++) {
    Group group = pathMatch.Groups[i];
    if (group.Success) {
      string key = pathRegex.GroupNameFromNumber(i);
      if (!string.IsNullOrEmpty(key) && !char.IsNumber(key, 0)) {
        if (!string.IsNullOrEmpty(group.Value)) {
          data.Values[key] = group.Value;
        }
      }
    }
  }
}

Stane opět poměrně snadno pochopitelný kód:

private static void IterateMatchingPathGroups(Match pathMatch, RouteData data) {
  for (int i = 1; i < pathMatch.Groups.Count; i++) {
    Group group = pathMatch.Groups[i];
    if (IsNotMatch(group))
      continue;

    string key = pathRegex.GroupNameFromNumber(i);
    if (IsInvalidKeyOrValue(group, key))
      continue;

    data.Values[key] = group.Value;
  }
}

Navíc, když se teď na ten kód podíváme, zjistíme, že se ty původní šestky (z kterých už jsou čtyřky) liší jen v tom, že používají každá jinou členskou proměnou, naštěstí stejného typu. Tak jdeme zase refaktorovat. :) Nejprve si tedy tuto proměnou uložíme lokálně na začátku metody a ze zbytku extrahujeme metodu novou s jedním parametrem navíc.

U druhý čtyřky už to dělat nemusíme, páč výsledný extrakt z té první je plně vyhovující našim potřebám a tak ho zavoláme jen s jiným parametrem. Ze dvou čtyřek máme jednu a dvě jedničky. A komplexita třídy se nám vrátila k původní hodnotě. Ano, máme tu teď spoustu malých metod, které ale mají jasný účel. A když se podíváme blíž, zjistíme, že můžeme úplně v klidu některé z nich extrahovat no úplně nové třídy. Ale to už je na jinou pohádku.

Stromečky jsou totiž fuč. :)

PS. Zmíněné refaktoringy a vizualizace metrik jsou součásti skvělého produktu Refactor Pro! nebo samotné refactoringy v neplacené verzi Code Rush Xpress.

Jedním ze základních pravidel psaní přehledného kódu je nepsat zbytečný kód. Pojďme si ukázat pár příkladů zbytečného kódu, kterému je zahodno se raději vyhnout.

Komentáře

Programátoři jsou často velice kreativní lidé, ale svou kreativitu si vybíjí nesprávným způsobem. Tak se může stát, že během své kariéry narazíte na skvosty typu:

1. zakomentované kusy kódu
2. označení dočasného kódu v kometáři
3. vylévání srdíčka nad vlastní blbostí

K bodu prvnímu. Když něco takového potkáte, rovnou to smažte, nesnažte se nad tím přemýšlet. Jestli někdy ten kód budete potřebovat, najdete ho ve vašem VCS. Pokud se přistihnete, že máte označený blok kódu a chystáte se zmáčknout magickou kombinaci kláves Ctrl+K, C, ušetřete si práci a rovnou zmáčkněte Delete. ;)

K bodu druhému. Nejprve zjistěte, kdo to napsal a dejtemu rovnou pohlavek nebo seberte prémie. Nejlépe oboje, u někoho lépe fungují podmíněné reflexy spojené s fyzickou bolestí, někdo si vzpomene, že mu nezbyly peníze na tu skvělou chlastačku a už to nebude chtít opakovat. Zjišťovat, co na koho platí je moc drahé. ;)

Pro představu, našli jsme takovýto kód:

//Testovaci verze
if (Request.Cookies.AllKeys.Contains("ticket")) {
  InvitedUser iuser = _invitedUsersRepository.FindByHash(Request.Cookies["ticket"].Value);
  if (iuser != null) {
    iuser.Valid = false;
    _invitedUsersRepository.Save(iuser);
  }
}
// --
;

Autora jsme ztrestali. První krok máme za sebou, co dál? Tohle asi hned smazat nepůjde. Prvním možným krokem, je nadefinovat build symbol a tyto bloky obalit preprocesorovou podmínkou:

#if TEST_VERSION
if (Request.Cookies.AllKeys.Contains("ticket")) {
  InvitedUser iuser = _invitedUsersRepository.FindByHash(Request.Cookies["ticket"].Value);
  if (iuser != null) {
    iuser.Valid = false;
    _invitedUsersRepository.Save(iuser);
  }
}
#endif
;

Sice jsme se nezbavili špatného kódu, ale aspoň pujde dočasná featura vypnout z jednoho místa a kód hovoří trochu jasněji.

Pokud napíšete blok kódu a pocítíte potřebu uvodit ho komentářem, nebo takových bloků máte zasebou víc, je čas refaktorovat! Místo komentářů vytvořte metody s popisnými názvy (ekvivalent komentáře). Komentáře mají tu vlastnost, že rychle ztrácejí význam a zůstávají v místech, kde už není co komentovat.

Pravidlo: Když ucítíte potřebu napsat v kódu komentář, zamyslete se, zda nenajdete lepší způsob jak tuto myšlenku vyjádřit.

Poznejte svůj jazyk

Za programátora se považuje každý, kdo se v nějakém jazyce naučí pár příkazů. Někdo si vystačí s málem a pak mu nezbývá než prasit. Základem je poznat jazyk, ve kterém se chystáme sdělovat své myšlenkové pochody. Zdrojový kód se mnohem častěji čte než píše, proto by měl obsahovat co nejméně zbytečností, které odvádějí pozornost.

Začněme třeba u JavaScriptu. Často se setkávám s kódem, který napovídá, že autor má hrubé znalosti C a jemu podobných jazyků, ale o JavaScriptu moc potuchy nemá.

Podmíněné přiřazení

Jednou z typických ukázek může být validace vstupních parametrů a nastavení výchozí hodnoty, pokud přišlo null. Nejhorší případ:

function example(options) {
  if (options == null) {
    this.options = { color: 'red' };
  }
  else {
    this.options = options;
  }
}

O něco chytřejší programátor použije ternární podmíněný operátor:

function example(options) {
  this.options = (options != null) ? options : { color: 'red' };
}

Vývojář znalý JavaScriptu však odbourá zbytečné ruchy a vznikne logický zápis:

function example(options) {
  this.options = options || { color: 'red' };
}

Pojďme teď dál k mému oblíbenému C#. Předchozí příklad tam nejde řešit operátorem || protože C# je silně typový a null != false. Ovšem C# 2.0 přinesl s Nullable<T> hodnotovými typy i operátor ??. Ten slouží jako zkrácená varianta ternárního podmíněného operátoru. Díky němu můžeme následující kód:

public void Example(Options options) {
  _options = (options != null) ? options : new Options { Color = Color.Red };
}

Pěkně odšumět:

public void Example(Options options) {
  _options = options ?? new Options { Color = Color.Red };
}

Pokud pracujete s Nullable hodnotovými typy, zkraťte i následující zá­pis:

public void Example(int? pageIndex) {
  int page = 1 + (pageIndex.HasValue ? pageIndex.Value : 0);
}

Na:

public void Example(int? pageIndex) {
  int page = 1 + (pageIndex ?? 0);
}

Generické parametry

Dalším, velice častým šumem, je explicitní uvádění generických parametrů, tam kde to není potřeba. Za příklad si vezměme třeba generickou metodu Assert.Equal<T>(T expected, T actual) z frameworku xUnit.net. Můžeme ji volat takto:

Assert.Equal<string>("test", null);

Jenže kompilátor není žádný hlupák a dokáže správně dosadit typ generického parametru podle typu předávaného parametru. Proto je možné napsat, mnohem přehlednější zápis:

Assert.Equal("test", null);

FxCop vám přímo radí: Pište generické metody tak, aby se typ generického parametru dal implicitně odvodit.

Zbytečné volání ToString

Často se také můžete setkat s voláním metody ToString tam, kde to není pořeba. Příklad:

double rating = 3.8;
string output = "Your rating is: " + rating.ToString();

Tohle lze napsat bez šumu:

double rating = 3.8;
string output = "Your rating is: " + rating;

Teď si jistě, říkáte, že jsem asi blbec, jak můžu po silně typovém C# chtít, aby sečetl string a double. Nuže můžu. Krom toho, že je C# silně typový, podporuje i přetěžování operátorů! A designéři třídy String přidali přetížení operátoru + na kterýkoliv Object, na kterém zavolají metodu ToString za vás. :)

Zbytečné explicitní přetypování

Občas se bohužel můžeme v kódu potkat s věcmi, nad kterými zůstává rozum stát. Někdo například už pochopil dědičnost. Pochopil, že třída List<T> implementuje rozhraní IList<T> a to dědí z ICollection<T>, které je potomkem obecného rozhranní IEnumerable<T>. Jenže pak přijde na praktické použití a je zle:

public class UserDetailViewData {
  public IEnumerable<User> Friends { get; set; }
}

public class UsersController : Controller {
  public ActionResult Detail(userName) {
    IList<User> friends = GetFriendsOfUser(userName);
    return View(new UserDetailViewData {
      Friends = (IEnumerable<User>)friends
    });
  }
}

Takhle vytržené z kontextu to nemusí vypadat až tak zle, ale originál mě stál málem ukroucení hlavy.

Roztahaná inicializace

Novinkou C# 3.0 jsou inicializátory. Zvažte následující kód:

var user = new User();
user.Name = "Josef Novák";
user.Gender = Gender.Male;

Přepsat na:

var user = new User {
  Name = "Josef Novák",
  Gender = Gender.Male,
};

Vyhnete se tím, zbytečnému opakování user..

Závorky navíc

Častý šumem v kódy bývají také závorky, které nemají žádný význam, zejména u inicilizátorů s bezparametrickým konstrukotrem new User() { Name = "Josef Novák" } a u anonymních bezparametrických delegátů delegate() { return true; }. Tyto závorky klidně smažte a u delegátů zvažte kompresi na lambda výraz: ()=> true.

Na druhou stranu existuje spousta případů, kdy vhodně vložené závorky zvyšují čitelnost.

Závěr

Když říkám, že je lepší psát némě kódu, rozhodně tím nemám na mysli kriptické zápisy alá regexy. To, co ušetříte na zbytečném kódu, klidně investujte do popisných názvů proměnných a metod. Důležitá je čitelnost. Se spoustou výše zmiňovaných úprav vám mohou pomoci nástroje, např. CodeRush Express, který je zcela zdarma.

Rozhodně jsem nepokryl všechny možnosti, kde psát zbytečný kód navíc. Jistě taky některé znáte, podělte se o ně v komentářích. :)

Přinejmenším na veřejnosti. Mnohdy se v různých APIs můžete setkat s tím, že různé objekty veřejně vystavují metody/vlasnosti, které mají návratový typ List<T>, v lepším případě IList<T>. Krásná ukázka nerozvážného návrhu.

Proč je špatné vracet List<T>

Když vracíte List<T>, znamená to, že říkáte všechno o vnitřní implementaci a zavíráte si vrátka pro její možnou změnu. Generický List sice neni nerozšiřitelný (sealed), ale ani nebyl při návrhu moc k rozšiřování zamýšlen. Navíc tim porušujute jeden ze základních principů OOP – a to zapouzdření. Dále tim umožňujete konzumentovi vaši kolekci přímo měnit.

Ukázka naivního/lenošného přístupu

Pro ukázku mějme třídu reprezentující uživatele User, kterému můžeme přiřadit uživatelské role Role. Naivní přístup:

public class User {
  public List<Role> Roles { get; private set; }
}

public class UserConsumerCode {
  public void SomeMethod {
    var user = new User();
    user.Roles.Add(Role.Admin);

    // ...

    user.Roles.Remove(Role.Reviewer);

    // ...

    var isAdmin = user.Roles.Contains(Role.Admin);
  }
}

Jak vidíte, klient má nad kolekcí rolí uživatele plnou moc. Může si přidávat, odebírat, přetřizovat jak se mu zlíbí. A my nemáme nejmenší šanci tyto celkem důležité operace jakkoli ovlivnit, nebo u nich vědět. Kdybychom změnili typ kolekce Roles na IList<Role>, tak sice můžeme změnit vnitřní implementaci a místo List<T> použít třeba LinkedList<T>, ale stále zveřejňujeme příliš mnoho. Jedinou výhodou tohoto řešení je, že jsme nemuseli napsat moc kódu a máme hodně funkcionality.

Jak zlepšit náš kód?

Rovnou přeskočím použití ICollection<T>, a přejdeme k tomu, jak by to mělo vypadat. Zachováme si zapouzdření, uzavřeme se vůči nechtěné manipulaci z vnějšku a otevřeme se snadným vnitřním změnám a rozšiřitelnosti (Open/closed principle).

public class User {
  private readonly ICollection<Role> _roles;

  public User() {
    _roles = new List<Role>();
  }

  public IEnumerable<Role> Roles {
    get { return _roles.ToArray(); }
  }

  public void AddRole(Role role) {
    _roles.Add(role);
  }

  public void RemoveRole(Role role) {
    _role.Remove(role);
  }

  public bool IsInRole(Role role) {
    return _roles.Contains(role);
  }
}

Pěkně jsme obohatili a zpřehlednili naše API (zápis user.IsInRole(Role.Admin) říká mnohem více, než user.Roles.Contains(Role.Admin), navíc neporušuje Demeterův zákon), skryli jsme implementační detaily a zároveň nechali otevřená vrátka pro snadnou výměnu vnitřní implementace… Dalším krokem by měly být kontrakty manipulačních metod, aby byly bezpečné. Dále pak můžeme přidat vyvolávání událostí, abychom o manipulaci věděli. Míst pro rozšíření je tu spousta.

Hloupá otázka na závěr

„Ale, jak teď zjistim, kolik rolí má uživatel přiřazených, když IEnumerable nemá vlastnost Count ani Length?“ using System.Linq; ;)

Zastávám názor, že šablona by měla být co nejvíce přehledná a obsahovat co nejméně programového kódu. Pojďme se podívat na některé kousky zasmrádlého kódu a jak se s nimi vypořádat.

Html.ActionLink

Html.ActionLink je pro mne jedním z prvních míst, kde zvednout ukazováček a říct: „takhle ne.“ Již ve spotu o routingu jsem psal, že procházení routovací tabulky při hledání akce je výpočetně náročné a pokud vaše tabulka obsahuje mnoho řádků, může být zdrojem nepříjemného zdržení. Mějme například helper odkaz na detail zboží v jehož URL obsahuje informace o kategorii zboží, jeho id a název:

<%= Html.ActionLink(Model.Name, "Detail", "Goods", new { id = Model.Id, name = Model.UrlName, category = Model.Category.UrlName }) %>

Tohle rozhodně není přehledné, ani výkonné. Začněme tedy refaktorovat. Nelíbí se mi, že tag odkazu je generovaný a že pořádně netuším, jestli třeba nejsou parametry ve špatném pořadí. A co potom takový kodér? Co když bude chtít přidat nějakou třídu, aby mohl tento konkrétní odkaz lépe nastylovat nebo mu dát nějaký sémantický význam? Přejděme na Url.Action!

<a href="<%= Url.Action("Detail", "Goods", new { id = Model.Id, name = Model.UrlName, category = Model.Category.UrlName }) %>"><%= Html.Encode(Model.Name) %></a>

No moc jsme si nepomohli. Sice teď máme větší kontrolu nad generovaným kódem, ale kód je o dost delší a pořád pěkně „smrdí.“ Zbavme se teď kouzelných řetězců, ty mohou být zdrojem špatně dohledatelných chyb!

<a href="<%= Url.RouteUrl(RouteTo.CommodityDetail, new { id = Model.Id, name = Model.UrlName, category = Model.Category.UrlName }) %>"><%= Html.Encode(Model.Name) %></a>

Zbavili jsme se možnosti udělat chybu v názvu akce nebo řadiče a navíc teď probíhá vyhledávání routy v tabulce podle klíče, tudíž mnohem efektivněji. Ale pořád mi tu něco smrdí. Ano je to ta anonymní třída, i tady je velká šance na vnesení chyby. Ačkoli to tak na první pohled nevypadá, názvy vlastností této třídy nejsou nic jiného než kouzelné řetězce, jen bez uvozovek. Na pozadí se totiž překládají jako klíče ve slovníku. Lék na to je jednoduchý. Vytvoříme si vlastní helper!

<a href="<%= Url.CommodityDetail(Model) %>"><%= Html.Encode(Model.Name) %></a>

Zápis se rázem zpřehlednil, snížila se možnost zanesení chyby a množství opakovaného kódu.