7 věcí, které mě rozčilují v cizím kódu

Tomáš Jecha       18. 1. 2013       C#, Offtopic       5891 zobrazení

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)”.

Undocumented code

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.

Reinventing the wheel

3. Bordel v souborech projektu

Nedávno jsem zahlédnul v jednom z projektů tuto třídu:

class Hashtable<T>
{
}

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.

Messy room

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!

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 == "test@test.com");
                .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.

7

 

hodnocení článku

2 bodů / 2 hlasů       Hodnotit mohou jen registrované uživatelé.

 

Nový příspěvek

 

Diskuse: 7 věcí, které mě rozčilují v cizím kódu

Nepsal jste zrovna vy tento článek: http://vbnet.cz/clanek--26-kreslici_tabu...

V tom kódu teda ale je komentářů.

Když vám to vadí, proč to děláte?

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět

V ukázkovém kódu, který slouží pro demonstraci, je žádoucí komentovat i triviální věci, dělám to taky.

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět

No dobře vypadá to ale jako kdyby jste z nás dělali neschopné.

Kdybychom něco nepochopili klikneme buď F1 nebo se podíváme do MSDN Library.

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět

Existuje řada způsobů jak získat informace. Pokud vás vysloveně uráží, že články obsahují příliš mnoho komentářů, tak je nečtěte.

nahlásit spamnahlásit spam 4 / 4 odpovědětodpovědět

Okomentované výukové články šetří dobudoucna čas všem zúčastněným. Jak těm co je čtou, tak i těm co je píšou, protože se na to někdo určitě zeptá. Je lepší to mít napsáno zrovna.

Určitě se přikláním mít tam více popisů a číst je zbytečně, než informace odkudkoliv lovit. Opravdu je to mnohonásobně kratší.

(zrovna jsem mrknul na tu kreslící tabuli a velmi chválím - ty hrubky jako přehlížím, dělám je občas taky :) )

nahlásit spamnahlásit spam 0 odpovědětodpovědět

Když pominu, že se jedná o výukový článek, u kterého je na místě důkladně vysvětlit, co kód dělá, zeptám se na jednu věc. Máte snad pocit, že komentáře ve článku jakkoliv splňují podstatu toho, o čem jsem tu psal, že mi vadí? Pokud na komentáře čas je, komentuje. Pokud není, je lepší čas věnovat komentováním důležitých částí, než bezmyšlenkovitému popisování zbytečností jen proto, že musíte.

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět

řekl bych, že je něco jiného kód v článku, který je psaný pro poučení ostatních (a tedy každý prd okomentuju) a v produkčním systému.

nahlásit spamnahlásit spam 1 / 1 odpovědětodpovědět
                       
Nadpis:
Antispam: Komu se občas házejí perly?
Příspěvek bude publikován pod identitou   anonym.

Nyní zakládáte pod článkem nové diskusní vlákno.
Pokud chcete reagovat na jiný příspěvek, klikněte na tlačítko "Odpovědět" u některého diskusního příspěvku.

Nyní odpovídáte na příspěvek pod článkem. Nebo chcete raději založit nové vlákno?

 

  • Administrátoři si vyhrazují právo komentáře upravovat či mazat bez udání důvodu.
    Mazány budou zejména komentáře obsahující vulgarity nebo porušující pravidla publikování.
  • Pokud nejste zaregistrováni, Vaše IP adresa bude zveřejněna. Pokud s tímto nesouhlasíte, příspěvek neodesílejte.

přihlásit pomocí externího účtu

přihlásit pomocí jména a hesla

Uživatel:  
Heslo:  

zapomenuté heslo

 

založit nový uživatelský účet

zaregistrujte se

 
zavřít

Nahlásit spam

Opravdu chcete tento příspěvek nahlásit pro porušování pravidel fóra?

Nahlásit Zrušit

Chyba

zavřít

feedback