Jak psát kód: Nepřežeňte to s komentáři

<?php
// Obnoví index.
$this->getIndex()->refresh();
?>

v6ak:

Celkem souhlas, ale zrovna architektonická rozhodnutí mi sedí víc do samotného kódu než do commitů.

Honza:

Ahoj Jakube, nenapsal jsi už nebo nechceš napsat o tom kdy/kde používat znak '@' ?

Nemůžeme se s kolegy shodnout co je lepší, jestli

if (isset($data['var']) && $data['var']) {
  ...
}

nebo

if (@$data['var']) {
  ...
}

David Grudl:

Zavináč je lepší nepoužívat vůbec nikdy. Utají skutečný důvod chyby, je pomalý atd. V pozadí @$data['var'] nebo @fopen($file) se může vykonávat nějaký uživatelský kód, ve kterém může být překlep, ale nijak se o tom nedozvíme. Zavináčem chceme potlačit jednu konkrétní chybu, ale bohužel potláčíme úplně všechny včetně fatálních.

Honza:

Ale bez něj je to někdy strašně dlouhé, a dost špatně se to pak čte (jednoduché přiřazení pak vypadá jako něco šíleného)

$var = @$a['b']['c']['d'];

vs

$var = (isset($a['b']['c']['d'])) ? $a['b']['c']['d'] : null;

Ten druhý zápis taky není "to ono" =(
Nevíte jak to napsat pěkně?

David Grudl:

Bohužel je nic kratšího není, byť se to už přes deset let řeší :-( Viz třeba https://wiki.php.net/rfc/ifsetor

Jirka:

Typický příklad idiocie vývojářů PHP. Teď už píšu radši v Ruby :)

v6ak:

Tak je možné si na to napsat funkci (specificky pro pole a ArrayAccess), pak to bude něco jako: <?php $var = arrayItemOrNull($a, 'b', 'c', 'd'); ?>

Přiznám se, že bez frameworku na nějakých jednoduchých místech zavináč občas použiju (např. @intval($_GET['foo'])). Ale moc to nedoporučuju. U složitějších případů to může snadno zakrýt třeba fatal error:

<?php $f = @fopen(getFlieName()); ?>

V tomto případě to zakrylo překlep v názvu funkce. Nicméně to může jít i hlouběji. Podobný problém může být někde uvnitř funkce getFileName nebo ještě hlouběji (funkce getFileName volá funkci a, která volá funkci b, která volá funkci c, která volá funkci d, která způsobí fatal error). Dejme tomu, že nebude přímo v fopen. Ale může být v handleru stream wrapperu - pokud by getFileName vrátila např. safe:///tmp/x.lock a handler streamu safe by způsobil chybu...

A případ s fatal error je možná ještě celkem nevinný (toho si programátor prostě hned všimne, byť bez debuggeru možná bude zuřivě ladit a půlit interval). PHP má ale i jiné úrovně chyb, např. notice nebo warning. V lepším případě si pak programátor nevšimne jednoho warningu a bude mu divné, proč to nefunguje. V horším případě to pustí do produkce.

trestná smradlavice:

Ráda se poučím. V čem je @ pomalý? Co je jeho 'úzké hrdlo'?
Jaký kód se může vykonávat za vrácením hodnoty možná neexistující proměnné (nemluvím o vlastnosti objektu)?
Souhlasím s praktikou psát PHPko tak, aby nevypisovalo E_WARNING ani E_NOTICE, ale třeba na kód @$pocet[$i]++; pro umlčení E_NOTICE v prvním případě, kdy index neexistuje, pohlížím jako na tolerovatelný přístup.

David Grudl:

Občas to taky použiju, buď z lenosti https://github.com/nette/tester/blob/73c4f855ac66a06acf727d1970cfe13c6da8f7ac/….type.phpt#L14, nebo protože není jiná možnost https://github.com/nette/nette/blob/master/…/FileStorage.php#L154 - jen člověk musí znát rizika.

Je to "pomalé", protože třeba @$pocet[$i]++ se rozloží do

<?php
$old = error_reporting(0);
$pocet[$i]++
error_reporting($old);
?>

Jaký kód se může vykonávat za vrácením hodnoty proměnné? No právě jsem měl na mysli objekty. Pokud jistě nevím, že $pocet je pole, tak těžko odhadovat, co $pocet["$i"]++ vlastně všechno může vykonávat. Taktéž může být $i objekt, u kterého se pak volá toString() atd.

v6ak:

On ten rozklad bude ještě složitější, uvážíme-li výjimky. Například pokud je $pocet objekt implementující ArrayAccess, pak výjimka nastat skutečně může. V takovém případě ale též musíme zavolat error_reporting($old).

Takže musíme mít spíš něco jako:
<?php
$old = error_reporting(0);
try{
    $pocet[$i]++
} finally {
    error_reporting($old);
}
?>

Jakub Vrána :

Používám na to jednoduchou uživatelskou funkci idx(): https://github.com/facebook/libphutil/blob/…/utils.php#L25. V tomhle konkrétním případě by se dalo použít i !empty($data['var']).

trestná smradlavice:

Hm.. nemělo by tam, na řádku 45 být ! před array_key_exists() ? Respektive, nechápu, proč by tam řádky 45-47 musely vůbec být.

v6ak:

Mějme pole:

<?php
$a = array(
    'x' => null
);
?>

Mějme následující volání funkce:
<?php
$result = idx($a, 'x', 'default');
?>

1. Co čekáš v $result?
2. Co tam bude?
3. Co by tam bylo, kdybychom zmíněné řádky 44-47 odstranili? Nápověda: isset($array[$key]) se chová v některých případech jinak než array_key_exists($key, $array).

Jakub Vrána :

Původně funkce byla přehlednější: https://github.com/facebook/libphutil/blob/…/utils.php#L55. Mikrooptimalizovali jsme ji, protože se používá na mnoha místech a volá se třeba i v cyklech s mnoha průchody. Dělá ale pořád totéž.

Michal:

S tímto článkem nemůžu souhlasit. Záleží totiž na situaci. Navíc nebyl uveden důvod, čemu vlastně nadbytečné komentáře škodí. Ale postupně:

Kód, který je implementací nějakého algoritmu (prohledávání grafu, řazení,...), komentáře vhodné jsou. Nesouhlasím s tím, že bychom měli algoritmus štěpit na mrtě malých metod. Kód by se stal špatně čitelným.

V primitivním get/set kódu si často bez komentářů vystačíme. Je to kód, kde jen vytahujeme a přiřazujeme potřebné hodnoty tak, aby vše fungovalo (typicky tvorba formuláře, přiřazování proměnných do šablony,...). Ale klidně bychom si s nimi mohli vypomoct, když zhodnotíme, že to bude mít nějaký přínos.

Nicméně si nemyslím, že nadbytečné a nicneříkající komentáře jsou problém. Pomocí komentářů a bílého místa lze kód vizuálně dobře strukturovat a zpřehledňovat. Například:

<?php

// prepare template
$template->var = "foo"
... another 20 lines of code

// prepare control
$control->foo = "bar"
... another 20 lines of code

?>

Takovéto komentáře vítám. Každý snadno vidí, že se jen přiřazují hodnoty, ale díky komentářům to je mnohem přehlednější.

Vadí vám, že komentáře pak překáží? To je nelze ignorovat? Díky syntax coloringu nemám problém se dívat na kód, jako by tam inline komentáře nebyly. Vidíte pak na monitoru moc dokumentačních komentářů a málo kódu? Co vyzkoušet code folding?

Bavil bych se spíše o tom, jak napsat vhodný komentář místo "prázdného". V příkladu z článku bych komentář nahradil důvodem proč se index refreshuje. Místo komentáře "// přiřazování hodnot formuláři" je lepší použít "// příprava formuláře", protože to obecně vypovídá o tom, co chceme udělat. Komentáře s implementačními detaily smysl moc nedávají pokud komentovaný kód není vyloženě produktem nějaké optimalizace. Ale opět - nevadí mi, když tam jsou.

A nakonec: uvádět komentáře ke kódu do commit zpráv? No to mi opravdu nepřijde rozumné, je to nesmysl. Při první úpravě nebo reorganizaci zpráva zapadne. V commit messages píšeme, co jsme commitli, ne dokumentaci toho, co jsme commitovali. Historií repozitáře procházíme podle toho, co hledáme, ne, jaké implementační detaily jsme použili. Nevím, proč bych měl pro pochopení kódu používat blame.

v6ak:

Ke štěpení do malých metod: IMHO je to celkem vhodné, nesmí se to samozřejmě přehnat. Zkus si ale pročíst třeba zdroják OpenCartu (zejména některé controllery) a uvidíš, k čemu je to dobré.

Otázka je, co jsou "nadbytečné a nicneříkající komentáře". Já si pod tím představím komentář, který mi nic neřekne, ale jen přitahuje pozornost. A to je na tom to špatné.

Jestli mít komentáře jako "prepare something"? To asi záleží na kontextu, ale mám pocit, že takový kód může velmi snadno porušit princip jedné obrazovky. Možná bude lepší ty komentáře tam mít než nemít, ale ještě lepší by to bylo udělat jinak:

<?php
$template = $this->prepareTemplate($x);
$control = $this->prepareControl($y);
?>

Michal:

Ano, pro dlouhé bloky kódu nadepsané komentářem je vhodné vydělení do samostatné metody. Nebo kód používaný na více místech. Pokud se ale jedná o pár řádků, často bohatě stačí provést oddělení právě komentáři.

Je to především o zkušenostech a citech programátora. Shrnul bych to jako "Dokumentujte kód, ale nepište komentář jen abyste napsali komentář."

Jakub Vrána :

Hlavní vada nadbytečných komentářů je v tom, že zdržují čtení. Pokud zkoumám nějaký cizí kód, třeba v něm hledám chybu, tak musím číst především kód (protože v něm je ta chyba) a čekám, že v komentářích bude nějaká užitečná informace. Samozřejmě je můžu skrýt nebo ignorovat, ale tím přehlídnu i komentáře, kde něco užitečného je.

Smysl přesunutí některých informací do commit zpráv je v tom, že jde potom jít do mnohem větších detailů a podrobně rozebrat všechna rozhodnutí a implementační detaily. V komentářích pak zůstane jen to nejdůležitější, co čtení kódu zjednodušuje a ne zdržuje.

Pimp:

Dobry den pan Vrana,

viem ze to sem nepatri, ale ako to vyzera s dalsim vyvojom NotORM. Je to perfektna kniznica, ale ma aj par nedostatkov:

-chybajuce JOINy ked nemam urobene constrainty medzi dvomi tabulkami tak tak ich neviem spojit su situacie kedy constrainty urobit nemozem (napr. v stlpci povolim hodnotu null) ale tabulky spojit musim.

-tiez je problem v nazvoch columnsov (pri spojovani tabuliek) preco sa musi pouzivat konvencia tabulka_id a nemozem si nadefinovat vlastnu napriklad tabulka_uid?najlepsie by bolo keby NotORM umoznovalo aj klasicke joiny.

-tiez mi vadi ze not ORM nepodporuje multiple insert (viem da sa to obist pomocou call_user_func_array ale ked niekto takyto kod cita rozhodne nebude vediet "kde je sever".

-preco NotORM neuma nieco ako metodu toArray pre NotORM_Row a NotORM_Result, ale musim volat iterator_to_array?

Jakub Vrána :

Fakt to sem nepatří, nicméně…

1. Hodnota NULL se s cizími klíči nijak nevylučuje, sloupec ji klidně může obsahovat, i když je nad ním vytvořen cizí klíč. Pro spojení tabulek lze kromě NotORM_Structure_Discovery použít i NotORM_Structure_Convention, které s cizími klíči vůbec nepracuje (a navíc je výchozí).

2. Vytvořením objektu NotORM_Structure_Convention lze nadefinovat odlišnou konvenci. Rozšíření této třídy přináší ještě bohatší možnosti. Implementace rozhraní NotORM_Structure potom úplnou svobodu. Navíc lze použít třídu NotORM_Structure_Discovery, kterou názvy sloupců vůbec nezajímají.

3. Metoda $result->multiInsert(array $rows) jako zkratka za call_user_func_array(array($result, 'insert'), $rows) by asi šla, to fakt moc čitelné není.

4. $result->toArray() by existovat mohlo, ale významně lepší proti standardnímu iterator_to_array($result) mi to nepřijde. $row->toArray() je odkaz na tabulku `toArray`.

Tabulky lze připojit pomocí tečkové notace, ale je to potřeba jen tehdy, když potřebuji data z jedné tabulky vyfiltrovat nebo seřadit podle hodnot v jiné tabulce.