Rozhodl jsem se sepsat několik prohřešků, které mi vadí při procházení cizího kódu. Určitě by se jich našlo ale mnohem více. Záměrně se zaměřuji na věci, které by měli být jasné i junior vývojářům s trochou soudnosti při pomyšlení na to, že se v jejich kódu někdy bude muset orientovat i někdo jiný. Popřípadě oni sami v době, kdy už stihli zapomenou, jak jejich kód funguje.
1. Absence komentářů
Na začátek chci zmínit, že nepatřím mezi zastánce pravidel “okomentované musí být vše” - obvykle omezené na vše viditelné mimo specifickou třídu (public/internal). Pak nacházíme zbytečné komentáře typu:
/// <summary>
/// Gets or sets if is locked.
/// </summary>
public bool IsLocked { get; set; }
Mnohem užitečnější a přehlednější je komentovat kód, jehož fungování nebo smysl chceme zdokumentovat. Je potřeba se vždy zamyslet na tím, jaké třídy/metody/vlastnosti/části kódu by mělo smysl nějak zdokumentovat. Prostě přemýšlejte nad tím, co by vám mohlo být nejasné, když se na kód podíváte třeba za půl roku.
Nejčastěji mi chybí obecné komentáře popisující smysl tříd, upozornění na požadavky při jejich používání, obhájení důvodů použití nestandardních konstrukcí a takový evergreen – odkazy na zdroje, podle kterých byl kód implementovaný.
Zrovna nedávno jsem se dostal ke kódu, kde byl naimplementovaný jeden autentifikační mechanismus poměrně neobvyklým způsobem z důvodu chyby na straně služby, kterou kód konzumoval. Vše absolutně bez komentářů, takže na první (vlastně ani druhý) pohled nevíte, zda se jedná o chybu nebo ne. Přitom by stačil komentář: “implementace technologie XYZ podle specifikace ABC; bylo změněno pořadí parametrů kvůli chybné implementaci na serveru 123 (o chybě ví, ale neplánují opravu)”.
2. Reinventing the wheel
Pokud se učíte programovat není určitě na škodu si znovu napsat něco, co už vymyslel někdo před vámi. Pokud ale píšete běžnou aplikaci, buďte líní! Pokud už implementace existuje (a jste schopni ověřit, že funguje), použijte ji. Jeden dotaz do vyhledávače zabere méně času, než implementovat něco celé od znova. A to mluvíme i o jednoduchých funkcích jako encodování a decodování URL adres, parsování query string řetězců a podobně. Obvykle je totiž vlastní implementace neotestovaná a vystavujete se nebezpečí problémům – například bezpečnostních děr. Největší chyba je implementovat něco, co již v .NET Frameworku existuje. V takovém případě to obvykle dotyčný udělá špatně – pokud si nenašel, že požadovaná funkce již existuje, pravděpodobně si ani nepřečetl specifikaci toho, co píše. Jak něco takového dopadá netřeba dodávat.
3. Bordel v souborech projektu
Nedávno jsem zahlédnul v jednom z projektů tuto třídu:
S největší pravděpodobností vývojář použil omylem funkci “Generate class for Hashtable<T>” a vznikl zbytečný soubor. Opravdu je tolik práce tu třídu smazat? Toto vypovídá buď o tom, že vývojář neví co dělá (neví, že třídu vytvořil) a nebo nemá respekt k ostatním vývojářům, co se na projektu budou podílet a v kódu se budou muset orientovat.
Soubor ale většinou nevznikne náhodou. Obvykle je to zastaralá třída, dříve používaná. Platí ale stejné pravidlo – třídu smažte, je uložená v source control systému a nenechte, aby mátla ostatní. Je to jako mít všude po bytě rozházený bordel a zvát si návštěvy.
4. Nevhodné použití dědičnosti
Používat správně a efektivně OOP principy není jednoduché. Existuje mnoho situací kdy neexistuje jedno ideální řešení. Obvykle je důležité najít nějaký optimální kompromis mezi příliš velkou a žádnou abstrakcí. Je potřeba objekty správně navrhnout podle jejich závislostí a zodpovědností. Obvykle se objektový návrh v průběhu času na projektu mění při přidávání nových funkcí a refaktorizaci. Jinými slovy, pokud je potřeba rychle změny implementovat, kód může začít značně smrdět.
Co mi ale vždycky vadilo, je používání principů OOP způsobem, který kód komplikuje víc, než kdyby byl napsán jako procedurální.
Klasickým příkladem je zavádění dědičnosti na místech, kde je to nevhodné. Následující kus kódu demonstruje, jak pomocí dědičnosti byla přidána objektu zaměstnance schopnost být zobrazován v nějakém seznamu.
public abstract class DataGridRow
{
// ...
public Color BackgroundColor { get; set; }
}
public class Employee : DataGridRow
{
// ...
public string FirstName { get; set; }
public string LastName { get; set; }
public decimal ComputeSalary(DateTime month);
}Dotyčný vývojář právě ušetřil jednu třídu. A to pouze za cenu nesmyslného provázání kódu a zkomplikováním rozšiřitelnosti. Bravo!
5. Špatné pojmenování
Když ve vašem projektu existují 2 třídy, které chcete pojmenovat stejně, tak je někde něco špatně a není nejlepším řešením vymyslet synonymum původního názvu. Zkuste se místo toho zamyslet, co je skutečným smyslem obou tříd a vymyslete pevnou a jednoznačnou terminologii, které se při vývoji striktně držte. Komentáře jsou v takovém případě samozřejmostí.
Stejně je tomu při pojmenování proměnných. Pokud máte 3 různé vstupy, které nějak zpracováváte, nepojmenujte je jako “request1”, “request2” a “request3”.
Opět jeden příklad:
.GroupJoin(existEmployee
, e => e.UserPrincipalName
, e2 => e2.Account
, (e, e2) => new { e = e, e2 = e2 })
//.Where(ee => ee.e.UserPrincipalName == "[email protected]");
.Where(ee => ee.e2.Count() == 0);6. Nekonzistentní kultura kódu
V kultuře psaní kódu jsem celkem tolerantní. Má to však určité hranice a pokud nedokážete udržet konzistenci kódu ani v rámci jedné třídy, je někde něco špatně.
Viz následující ukázka – žádné komentáře, chybějící a zbytečné mezery a konce řádků, překlepy. A to nemluvím o problémech samotného kódu jako vlastnoruční parsování query string řetězců nebo nepoužité proměnné.
class OnlineRequset:Request
{
long? purchaseId;
.......
public override string Email
{
get
{
//ID=542424&PURCHASE_DATE=01%2f02%2f2008
var match = Regex.Match(Query, EmailPattern);
if (!match.Success)
throw new Exception(string.Format("Requset query is not valid: {0}", Query));
return match.Groups["Email"].Value;
}
set
{
var match = Regex.Match(Query, EmailPattern);
if (!match.Success)
throw new Exception(string.Format("Requset query is not valid: {0}", Query));
Query = Query.Replace(string.Format("EMAIL={0}&STREET", match.Groups["Email"].Value), string.Format("EMAIL={0}&STREET", value));
}
}
public string Query { get; private set; }
private static readonly Regex _queryRx = new Regex(@"^ID=(?<Id>[0-9]+)\&DATE=(?<Date>[0-9f%]+)\&NO=(?<No>[0-9]+)", RegexOptions.IgnoreCase | RegexOptions.Compiled);
private string EmailPattern = @"EMAIL=(?<Email>.*)&STREET";
public OnlineRequset(string qs)
{
// TODO: Complete member initialization
Query = qs;
}
}7. Když se jeden typ rozhodnutí vleče napříč celou aplikací
Tohle mě opravdu dokáže hodně vytočit. Představte si, že máte upravit oprávnění v nějaké aplikaci. Nová skupina uživatelů by měla mít přístup k některým informacím o některých zákaznících. Začnete zkoumat, jak je řešené oprávnění a zjistíte, že se zobrazování řeší v prezenční vrstvě formou “zobrazit tlačítko, pokud je uživatel ve skupině X nebo Y a zároveň v Z a nebo je jeho jméno A nebo B a nebo není C nebo D”. Podobný typ rozhodování je ale také přímo v hlavním kódu aplikace, v reportech a dokonce v databázi. Takže každá úprava bude trvat neúnosně dlouho a navíc budete mít téměř jistotu, že s každým zásahem na něco zapomenete. Naprosto ideální kombinace, pokud není logika oprávnění nikde zdokumentovaná. Jedinou cestou ven je kompletně takové zvěrstvo vyoperovat ven a vyřešit vše z jednoho místa. A také vyhodit původního autora aplikace.
