Refaktorizovat, nebo ne?

[Tuxik]

Řeším takovej malej problém:

napsal jsem v PHP modul pro jeden systém. Původně mělo jít pouze o malý projekt pro vlastní potřebu, nicméně se mi to trochu rozrostlo a bude to k dispozici veřejně.

Teď řeším zásadní otázku, jak moc refaktorizovat kód.

Původní systém je napsaný dost volně, coding style v podstatě žádný, často se opakují hlavičky, načítání language stringů je řešeno jeden řádek = jeden string, načítání změněných/nakonfigurovaných/defaultních hodnot na několik řádků pro každou hodnotu atd. Z tohoto důvodu má jedna metoda zcela běžně 100,200 i více řádků. Když se pokusím o refaktorizaci, sice jsem schopný z toho udělat pěkný kód rozsekaný do menších metod, ale tento styl potom samozřejmě vůbec nekorespoduje s celým zbytkem systému a většina refaktorizace v podstatě řeší hlavně původní systém a ne můj modul.

Přepsat celý systém rozhodně není řešení (asi by bylo jednodušší si to napsat celý sám znovu a na to opravdu nemám ani čas, ani chuť), je to ne úplně málo používaný OSS.

Takže co s tím? Refaktorizovat modul podle sebe a zamotat hlavu ostatním, kterým se to dostane do ruky (já vím, když to udělám dobře, normální programátor by v tu chvíli měl zajásat, že je aspoň kousek napsanej pěkně), nebo dodržet původní bordel a smířit se s tím?

[Reklama]

[dustin]

Pokud je modul tvůj kód, nic ti přece nebrání mít stručné přehledné metody bez duplicit a správně pojmenované proměnné. Že je zbytek projektu nepřehledná špageta, s tím asi sotva něco uděláš.

[Kit]

Teď řeším zásadní otázku, jak moc refaktorizovat kód.

Zkusím odpovědět otázkou: Jak moc do toho v budoucnosti chceš zasahovat? Pokud vůbec, nemá to význam. Kdysi jsem napsal fungující špagetu o 700 řádcích, funguje dodnes. Za 5 let provozu nebylo potřeba v tom změnit ani čárku. Není tedy důvod to refaktorovat.

Původní systém je napsaný dost volně, coding style v podstatě žádný, často se opakují hlavičky, načítání language stringů je řešeno jeden řádek = jeden string, načítání změněných/nakonfigurovaných/defaultních hodnot na několik řádků pro každou hodnotu atd. Z tohoto důvodu má jedna metoda zcela běžně 100,200 i více řádků. Když se pokusím o refaktorizaci, sice jsem schopný z toho udělat pěkný kód rozsekaný do menších metod, ale tento styl potom samozřejmě vůbec nekorespoduje s celým zbytkem systému a většina refaktorizace v podstatě řeší hlavně původní systém a ne můj modul.

Nejde o rozsekání kódu, ale o dekompozici. Každá ucelená vlastnost by z něj měla být vyčleněna, pojmenována, parametrizována a uložena jako samostatná funkce či metoda. Nezáleží na tom, zda je volána 20× nebo pouze jednou.

Umělé rozsekání kódu je k ničemu.

Přepsat celý systém rozhodně není řešení (asi by bylo jednodušší si to napsat celý sám znovu a na to opravdu nemám ani čas, ani chuť), je to ne úplně málo používaný OSS.

Co je to za OSS, ať si to mohu alespoň prohlédnout? Pokud mě to zaujme, mohu to případně refaktorovat a opět vydat pod licencí OSS jako fork.

Takže co s tím? Refaktorizovat modul podle sebe a zamotat hlavu ostatním, kterým se to dostane do ruky (já vím, když to udělám dobře, normální programátor by v tu chvíli měl zajásat, že je aspoň kousek napsanej pěkně), nebo dodržet původní bordel a smířit se s tím?

Každé správné refaktorování se opírá o testy. Můžeš tedy začít tím, že napíšeš ty testy. Nic nepokazíš a pokud se na tom zastavíš, můžeš je následně využívat pro kontrolu funkčnosti aktualizací. Teprve pak má smysl začít refaktorovat.

[Tuxik]

No to ano, samozřejmě sem se snažil, nicméně v celém systému ve všech modulech je běžné mít v hlavní metodě naplácaných cca 50 řádků jen načtení hlavičky stránky s nějakýma standartníma klikátkama a error testama, následuje X řádků načítání jazyka a dalších serepetiček, na který z neznámýhoo důvodu nejsou standartní metody, i když se v celým systému opakují milionkrát. Za tím už následuje samotný užitečný kód, na kterým jsem si dal celkem záležet, ale co s tím začátkem? Prostě ho useknout do samostatné škaredé metody, ať moc nezavazí, nebo ho opravdu pěkně učesat?

Osobně jsem na pochybách, jestli s vyjímkou tohoto jednoho modulu chci mít s tím systémem ještě někdy něco společného. Pokud se rozhodnu že ano, celkem by se mi asi vyplatilo to učísnout sám pro sebe...

Spíš by mě zajímal názor na to umístění něčeho pěkného do takového bordelu. Pokud je kolem bordel, se kterým už jsou ostatní zvyklí pracovat, jestli to nebude spíš na škodu zavést do toho řád pouze na jednom místě.

[Kit]

Pokud je modul tvůj kód, nic ti přece nebrání mít stručné přehledné metody bez duplicit a správně pojmenované proměnné. Že je zbytek projektu nepřehledná špageta, s tím asi sotva něco uděláš.

Je zbytečné zdržovat se hledáním duplicit. Časem se najdou samy, protože budeš mít tendenci stejnou funkcionalitu pojmenovat stejně. Je nutné refaktorovat každý logický celek bez ohledu na počet použití.

[Reklama]

[Tuxik]

Nejde o rozsekání kódu, ale o dekompozici. Každá ucelená vlastnost by z něj měla být vyčleněna, pojmenována, parametrizována a uložena jako samostatná funkce či metoda. Nezáleží na tom, zda je volána 20× nebo pouze jednou.
Umělé rozsekání kódu je k ničemu.

No a o tom to je, pokud je tam 50 řádků v podstatě lineárního programu, kterej dohromady řeší hlavičku, maximálně to vyhodím do nějakýho getHeader, odsunu to někam na konec, ať mi to nekazí zbytek a bude :-D
něco podmínek tam je, ale vyhazovat zvlášť něco jako

Kód: [Vybrat]

$user = ( $this->user->logged ? $this->user->name : 'guest' );mi přijde kontraproduktivní...

Co je to za OSS, ať si to mohu alespoň prohlédnout? Pokud mě to zaujme, mohu to případně refaktorovat a opět vydat pod licencí OSS jako fork.

OpenCart, přeju hodně štěstí, vthnul jsem se na to původně s velkým elánem... elán mě přešel zhruba v okamžiku, kdy jsem si v sublimu otevřel první soubor "pro inspiraci" :-D

Každé správné refaktorování se opírá o testy. Můžeš tedy začít tím, že napíšeš ty testy. Nic nepokazíš a pokud se na tom zastavíš, můžeš je následně využívat pro kontrolu funkčnosti aktualizací. Teprve pak má smysl začít refaktorovat.

nemám testy na všechno, ale něco jsem si napsal, protože jsem se v tom bordelu celkem ztrácel. Bez toho bych byl asi v yadeli po každé druhé úpravě

[Kit]

No to ano, samozřejmě sem se snažil, nicméně v celém systému ve všech modulech je běžné mít v hlavní metodě naplácaných cca 50 řádků jen načtení hlavičky stránky s nějakýma standartníma klikátkama a error testama, následuje X řádků načítání jazyka a dalších serepetiček, na který z neznámýhoo důvodu nejsou standartní metody, i když se v celým systému opakují milionkrát. Za tím už následuje samotný užitečný kód, na kterým jsem si dal celkem záležet, ale co s tím začátkem? Prostě ho useknout do samostatné škaredé metody, ať moc nezavazí, nebo ho opravdu pěkně učesat?

Pokud se držím své oblíbené architektury MVC, tak:

• načtení hlavičky stránky patří do View
• error testy patří do Modelu
• načítání jazyka? V PHP je spousta zajímavých standardních metod. To by mě zajímalo
• co je to užitečný kód? Patří do Controlleru, Modelu nebo do View. V hlavním modulu nemá co pohledávat

Hlavní modul mám vždy ořezaný na kost. Pouze v něm injektuji databázi do modelu a model podstrčím tovární metodě, aby vyrobila potřebný controller nebo view. A pak je jen echem odprezentuji.

Osobně jsem na pochybách, jestli s vyjímkou tohoto jednoho modulu chci mít s tím systémem ještě někdy něco společného. Pokud se rozhodnu že ano, celkem by se mi asi vyplatilo to učísnout sám pro sebe...

Zcela jistě je to osobní rozhodnutí.

Spíš by mě zajímal názor na to umístění něčeho pěkného do takového bordelu. Pokud je kolem bordel, se kterým už jsou ostatní zvyklí pracovat, jestli to nebude spíš na škodu zavést do toho řád pouze na jednom místě.

To záleží na situaci. Je možné, že se ostatním zalíbí elegance tvé nové verze a předělají v tom stylu i zbytek aplikace. Nebo taky ne

[Kit]

Nejde o rozsekání kódu, ale o dekompozici. Každá ucelená vlastnost by z něj měla být vyčleněna, pojmenována, parametrizována a uložena jako samostatná funkce či metoda. Nezáleží na tom, zda je volána 20× nebo pouze jednou.
Umělé rozsekání kódu je k ničemu.

No a o tom to je, pokud je tam 50 řádků v podstatě lineárního programu, kterej dohromady řeší hlavičku, maximálně to vyhodím do nějakýho getHeader, odsunu to někam na konec, ať mi to nekazí zbytek a bude :-D
něco podmínek tam je, ale vyhazovat zvlášť něco jako

Kód: [Vybrat]

$user = ( $this->user->logged ? $this->user->name : 'guest' );mi přijde kontraproduktivní...

U jednořádokvých zápisů je to vždy na vážkách, ale v tomto případě bych nekompromisně udělal metodu getUser(), i když má jen jeden řádek.

Kód: [Vybrat]

function getUser() {
    return $this->user->logged ? $this->user->name : 'guest';
}

Disclaimer: Mé hejtování getterů/setterů teď nechme stranou, dostaneme se k tomu pozděij.

Co je to za OSS, ať si to mohu alespoň prohlédnout? Pokud mě to zaujme, mohu to případně refaktorovat a opět vydat pod licencí OSS jako fork.

OpenCart, přeju hodně štěstí, vthnul jsem se na to původně s velkým elánem... elán mě přešel zhruba v okamžiku, kdy jsem si v sublimu otevřel první soubor "pro inspiraci" :-D

OK, podívám se na to. Volného času mám teď dost.

[Kit]

Co je to za OSS, ať si to mohu alespoň prohlédnout? Pokud mě to zaujme, mohu to případně refaktorovat a opět vydat pod licencí OSS jako fork.

OpenCart, přeju hodně štěstí, vthnul jsem se na to původně s velkým elánem... elán mě přešel zhruba v okamžiku, kdy jsem si v sublimu otevřel první soubor "pro inspiraci" :-D

OK, podívám se na to. Volného času mám teď dost.

Aha. 150k řádek. Tak do toho se mi tedy nechce. Jedině že bych to zkrátil na 5-10k řádek při zachování funkčnosti. Jenže i tak by to byla práce na měsíc.

[Tuxik]

Popravdě z toho celýho mám velmi smíšené pocity... celkem pěkně a kvalitně jsem implementoval API na jedné přepravní služby, která projevila zájem o další spolupráci, dokonce se jim i ten kód líbí, nicméně já si pořád připadám tak nějak blbě, protože jsem si do svýho celkem pěknýho kódu přenesl část cizího bordelu.

Jinak celej OpenCart mi přijde jako slepenec práce několika lidí, kteří vůbec neměli tušení, co že vlastně dělají, každej si napsal svůj kousek podle zadání a někdo to potom bezmyšlenkovitě poslepoval dohromady. Výsledkem je nepochopitelnej mix OOP, PP a čehosi  mezi tím.

Co se týče architektury, je to celkem logicky rozdělený na controller, model, view a language. Systém language je celkem zvláštní, ale budiž, model vypadá většinou celkem úhledně, view jsou tpl šablony, tomu není moc co vytknout, jenom v controlleru člověk kolikrát narazí na neuvěřitelný věci.

Co se týče hlavičky jako takové, ta je celkem v pohodě, ale za ní následují breadcrumbs, nějaký další mačkátka, pokud se používají a to je už lepený v controlleru pokaždá znova, aneb ideální kaditáti na osamostatnění. Další zajímavá věc je načítání dat pro formuláře, ukázka:

Kód: [Vybrat]

if (isset($this->request->post['weight_sort_order'])) {
$data['weight_sort_order'] = $this->request->post['weight_sort_order'];
} else {
$data['weight_sort_order'] = $this->config->get('weight_sort_order');
}
v některých případech je místo else ještě elseif a až potom else, kde se přiřadí třeba $blabla=''; jako default, když není nic jinýho. Z toho by šlo určitě udělat krásnou funkci a tu pak používat na jednom řádku místo 5-7

Kód: [Vybrat]

$cosi=getValue('nazevGet','nazevConfig','default');
Tohle celý je třeba 20x za sebou při načítání formuláře a ve výsledku když to projde funkcí validate(); tak se to odešle jako array do nějakých systámových funkcí na uložení nebo kdoví co.

Jak to tu píšu a přemýšlím nad tím, tak refaktorovat se bude, protože tohle je prostě napřesdržku.

Děkuji za rady

PS pro Kit: na 5-10k řádek by se to asi dostávalo blbě

[Tuxik]

Ještě co se týče přepsání jednořádkovýho ifu na jednořádkovou metodu (která má tři řádky) mi přijde opravdu spíš zbytečný. Pokud je ten if nějakej složitější a v podstatě ho pojmenovat metodou zjednoduší čtení, proč ne, ale zrovna v tomto případě mi to opravdu přijde kontraproduktivní. Respektive... metoda getUser měla být už v core systému a neměla se opakovat v každým druhým skriptu (jenom upozorňuju, že ten jednořádkovej if není vytaženej z opencartu, byl to jenom narychlo napsanej příklad), ale v podstatě podobně jednoduchý věci se tam podle mě opakují celkem často a vyčlenit je globálně by nebylo špatný. O tom byl v podstatě z velké části můj původní dotaz, jestli tyhle věci řešit po svým, když je nikdo nevyřešil systémově, nebo jestli to prostě udělat stejně, jak všude jinde.

[Kit]

Kód: [Vybrat]

if (isset($this->request->post['weight_sort_order'])) {
$data['weight_sort_order'] = $this->request->post['weight_sort_order'];
} else {
$data['weight_sort_order'] = $this->config->get('weight_sort_order');
}


Tohle bych refaktoroval například takto:

Kód: [Vybrat]

$data['weight_sort_order'] = $this->post->getSafe('weight_sort_order');a ve třídě Post bych dopsal metodu:

Kód: [Vybrat]

function getSafe($key) {
    return isset($this->post[$key]) ? $this->post[$key] : $this->config[$key];
}

Pokud v POST nebude požadovaný parametr, vezme se defaultní hodnota z konfigu. Věřím, že tohle bude mnohem užitečnější, než předchozí zápis, protože to můžeš použít na jakoukoli proměnnou.

v některých případech je místo else ještě elseif a až potom else, kde se přiřadí třeba $blabla=''; jako default, když není nic jinýho. Z toho by šlo určitě udělat krásnou funkci a tu pak používat na jednom řádku místo 5-7

Kód: [Vybrat]

$cosi=getValue('nazevGet','nazevConfig','default');
Tohle celý je třeba 20x za sebou při načítání formuláře a ve výsledku když to projde funkcí validate(); tak se to odešle jako array do nějakých systámových funkcí na uložení nebo kdoví co.

Všech else se systematicky zbavuji, tady bych tím začal. Tvůj příklad bych ještě zredukoval:

Kód: [Vybrat]

$cosi=getValue('nazevGet');
protože při absenci proměnné v GET oslovená metoda bude vědět, zda má hledat alternativní data v $config a pod jakým názvem. Metoda v $config zase bude vědět, jaká data má dosadit v případě absence v konfigu.

PS pro Kit: na 5-10k řádek by se to asi dostávalo blbě

No tak to bude o 2-3 řádky delší. No a co?

[Kit]

Ještě co se týče přepsání jednořádkovýho ifu na jednořádkovou metodu (která má tři řádky) mi přijde opravdu spíš zbytečný.

Než vymyslíš, zda to je či není zbytečné, můžeš to mít už napsané.

Nejde o to, jestli přepisem ušetříš nebo vyplýtváš řádky. Jde o to, že deleguješ pravomoci. Nikdo kromě třídy User nebude pak kecat do toho, jak se má jmenovat nepřihlášený uživatel. Pokud někdy budeš chtít přejmenovat 'guest' na 'anonym', půjdeš to opravit pouze do třídy User a nikde jinde to hledat nebudeš.

Pokud je ten if nějakej složitější a v podstatě ho pojmenovat metodou zjednoduší čtení, proč ne, ale zrovna v tomto případě mi to opravdu přijde kontraproduktivní. Respektive... metoda getUser měla být už v core systému a neměla se opakovat v každým druhým skriptu (jenom upozorňuju, že ten jednořádkovej if není vytaženej z opencartu, byl to jenom narychlo napsanej příklad), ale v podstatě podobně jednoduchý věci se tam podle mě opakují celkem často a vyčlenit je globálně by nebylo špatný. O tom byl v podstatě z velké části můj původní dotaz, jestli tyhle věci řešit po svým, když je nikdo nevyřešil systémově, nebo jestli to prostě udělat stejně, jak všude jinde.

Vybral jsi nádherné příklady, na kterých se dá demonstrovat, že i zdánlivě zbytečné refaktorování některých jednoduchých konstrukcí může mít smysl.

Máš pravdu, v OpenCart je plno zbytečných lkíčových slov else a elseif. Byly by mezi prvními oběťmi mého refaktorování, protože jen zatemňují a zpomalují programy. Nejsou k ničemu užitečné, jsou to relikty z dob strukturovaného programování. V některých modernějších programovacích jazycích je už nenajdeš.

[Karel]

Ještě co se týče přepsání jednořádkovýho ifu na jednořádkovou metodu (která má tři řádky) mi přijde opravdu spíš zbytečný. Pokud je ten if nějakej složitější a v podstatě ho pojmenovat metodou zjednoduší čtení, proč ne, ale zrovna v tomto případě mi to opravdu přijde kontraproduktivní. Respektive... metoda getUser měla být už v core systému a neměla se opakovat v každým druhým skriptu (jenom upozorňuju, že ten jednořádkovej if není vytaženej z opencartu, byl to jenom narychlo napsanej příklad), ale v podstatě podobně jednoduchý věci se tam podle mě opakují celkem často a vyčlenit je globálně by nebylo špatný. O tom byl v podstatě z velké části můj původní dotaz, jestli tyhle věci řešit po svým, když je nikdo nevyřešil systémově, nebo jestli to prostě udělat stejně, jak všude jinde.

U toho ifu je důležité, zda obsahuje business logiku. A zrovna tenhle if ji obsahuje, protože definuje, jaké username se použije, pokud uživatel není přihlášen. Pokud proměnná user je statická a její hodnota se naplní jen jednou, pak není co řešit a zůstává to jako if. Pokud je to ale operace, která se vyskytuje v kódu vícekrát, pak jednoznačně patří do samostatné funkce. Účel poznáte ve chvíli, kdy budete chtít logiku změnit, kupříkladu místo 'guest' použít 'anonymouse'.

Zjednodušeně řečeno je moje doporučení: pokud ta část kódu dělá nějaké rozhodnutí, které se dělá shodně na více místech, tak ji má smysl vyčlenit jako samostatnou funkci tak, aby ve zdrojovém kódu byla ta část kódu zapsána jen jednou. A to i když se jedná o jediný řádek.

[Tuxik]

Co se týká dopisování čehokoliv do stávajícího "bordelu", není to tak jednoduché. Systém je dobré čas od času aktualizovat a průšvih je na světě, co se přepsalo, musí se přepsat znova. Existuje na to sice "suprovej" modul vqmod, kterej dělá podle šílenýho XML zápisu rewrite php souborů se změnama, ale tomu se vyhýbám, jak to jen jde (používám ho, protože je to bohužel nejrozumější způsob úpravy core files, ale opravdu jen pokud neexistuje žádná jiná možnost). Pokud se mi to hodí, raději si dopíšu vlastní metody s tím, že volají ty původní, jenom třeba trochu jinak. Proto raději udělám vlastní metodu se třema parametrama, než přepisovat core soubory tak, aby mi stačil pouze jeden parametr.

A ještě jedna otázečka... napadá někoho, jak zredukovat tenhle bordel, nebo ho mám prostě jenom vyhodit do vlastní metody getLanguageStrings a nechat to být?

Kód: [Vybrat]

        $data['heading_title'] = $this->language->get('heading_title');
        $data['text_edit']    = $this->language->get('text_edit');
        $data['text_enabled'] = $this->language->get('text_enabled');
        $data['text_disabled'] = $this->language->get('text_disabled');
        $data['text_content_top'] = $this->language->get('text_content_top');
        $data['text_content_bottom'] = $this->language->get('text_content_bottom');     
        $data['text_column_left'] = $this->language->get('text_column_left');
        $data['text_column_right'] = $this->language->get('text_column_right');
        $data['entry_code'] = $this->language->get('entry_code');

je to jen kousek kódu, někde je to 50 i víc takových úžasných řádků. Jenom upozorňuju, že $this->language->get chci v nějaké formě zachovat a nepřepisovat, protože je to opět jedna z core funkcí, která se prostě používá všude a nechci zavádět další na to samý.