C# Tech. interjú – Code Smells

Ez a cikk a korábban bemutatott c# gyakorlati feladat megoldásának az első része. A feladatkiírás a programmal kapcsolatos észrevételek összegyűjtését a 4-ik feladatként adja meg, azonban az első feladathoz nem lehet hozzákezdeni, amíg a kód minimális szinten nincs rendbe téve. Emiatt a 4-ik ponttal kezdjük, annak is az első felével, ami a kisebb problémákat taglalja.

Ha valaki nem töltötte a forráskódot az előző cikk alapján, a lényeges rész e cikk végén megtalálható

Az első pillantás

Egy tapasztalt fejlesztőnek sokszor elég egy pillantás egy kódra, hogy megmondja, jó eséllyel problémák vannak vele. A probléma itt nem feltétlen hibás működést jelent, hanem a program szerkezetét érintő problémákat, amelyek hosszútávon megnehezítik vagy lehetetlenné teszik a program karbantartását. Hogyan lehetséges ez?

Code Smells

A titok nyitja, hogy a programozók nagyrészt ugyanazokat a hibákat követik el. Ezeket a tipikus hibákat az elmúlt években több cikk és könyv is kitárgyalta. A két legnépszerűbb könyv talán a Refactoring és a Clean Code. Később a szakma átvette a cikkekből/könyvekből a Code Smell kifejezést, illetve a jellemzőbb hibák szélesebb körben elfogadott neveket kaptak.

Mindig rossz a Code Smell?

A Code Smell csak egy figyelemfelkeltő jelenség, ami mögött jó eséllyel strukturális problémák állnak. Lehetnek azonban esetek, amikor a megoldás nem rossz. Az is lehet, hogy a megoldás nem a legelegánsabb ugyan, de a megoldandó feladat mérete nem indokolja az elegáns megoldásba fektetendő energiát.

A példakód problémái

A példakód számos helyen gyanús. Egy interjúzó helyzetét nehezíti, hogy néha nehéz eldönteni, hogy a kód felépítése azért rossz, mert ez csak egy egyszerű tesztfeladat, aminek nem kell minden részletében kidolgozottnak lenni, vagy pedig azért, hogy észre legyen véve. Emiatt érdemes minden furcsaságot/csúnyaságot megnevezni, hogy inkább az interjúztató magyarázkodjon.

MainWindow vs FileSystem

Azonnal a kód elején felfigyelhetünk két teljesen eltérő felelőségkörű osztály használatára:

LikeStatMixedConcerns

Mit jelent ez? Azt, hogy az alkalmazásban a megjelenítésért felelős kódba bele van gyúrva egy perzisztencia gyanús metódus. Gond ez? Nem feltétlenül, viszont csak kicsi kódokra működik kezelhető módon. Ezt az architekturális stílust Smart UI-nak hívjuk, amelyre mára mint Anti-Pattern tekintünk.

Az anti-pattern státusz ellenére a Smart UI kicsi alkalmazásoknál jó választás lehet – de ez nem jelenti azt, hogy a felelőségi körök valamiféle strukturálására ne kellene törekedni. A példában megoldás lehetne egy Model osztály létrehozása a “nem UI” kódok hordozására, esetleg használható lenne az Application osztály ugyanerre. Ez minimális refaktorálási igénnyel bír, nem bonyolítja az architektúrát, viszont nem eseménykezelőkbe nőnének bele a program funkciói.

Egy interjú során ennek a problémának a kiszúrása egyszerű feladatnak tűnhet, azonban ez általában csak egy ugródeszka az izgalmasabb beszélgetések megkezdéséhez. A Smart UI kapcsán például könnyen kerül a téma a következő kérdésekkel tarkított mederbe:

  • Milyen egyéb nevezetes architekturális modelleket ismer?
  • Mi a különbség az alkalmazás logika és az üzleti logika között? Miért szoktak törekedni ezek szeparációjára?
  • Mi a háromrétegű alkalmazások jellemző felépítése?
  • Milyen egyéb rétegeket szoktak még megnevezni összetettebb alkalmazásoknál?
  • Mit jelent a “cross cutting concern”? Mik tartoznak ide?
  • Melyek a jellemző problémák a cross cutting concern-ek implementálásánál (a SOLID elvek tükrében)?
  • Mi az aspektus orientált programozás lényege?
  • Hogyan kapcsolódik az aspektus orientált programozás a cross cutting concern-hez?
  • Mi az az interceptor? Hogyan viszonyul az aspektus orientált programozáshoz?
  • Hogyan viszonyul az MVC vagy az MVVM egy háromrétegű architektúrához?
  • Mi egy architektúrában a vertikális/horizontális felosztás szerepe/viszonya?
  • Mit tud a szerviz orientált architektúrákról?

Ne szégyellje magát az, aki nem tudja az összes fenti kérdésre a választ – a programozók legnagyobb része megakad a három rétegű alkalmazásoknál. Éppen ezért nagyon pozitív, ha valaki a többiről is tud beszélni.

Privát GetFileList() mint első metódus

A MainWindow osztály definíciójában az első metódus a következő:

LikeStatBadMemberOrder

Egy osztály leírásánál többféle módon lehet csoportosítani és rendezni az osztály member-jeit, azonban mindegyik elterjedt csoportosításnak ugyanaz a célja: segítse a fejlesztőt az osztály megismerésében. Mire kíváncsi egy fejlesztő, amikor meg akar ismerni egy osztályt? Elsőként, hogy mire és hogyan lehet az osztályt használni. Ennek megfelelően sokan a programsorok rendezésénél az osztály publikus interfészét megvalósító metódusokat rakják előre, hiszen azokon keresztül lehet használni az osztályt, és várhatólag ezekben van implementálva a magas szintű logika. Legtöbb stílusnál a publikus metódusokat érthető módon megelőzik a konstruktorok, és sok esetben az osztály állapotát leíró változók.

Legtöbb stílusnál a privát metódusok a publikus metódusok alatt vannak, tekintve, hogy ezek implementációs részleteket rejtenek, ezeket elég később megismerni. Van azonban egy sorrend a privát metódusok között is: a programozó általában a nagyobb léptékű logikát leíró kódra kíváncsi előbb, és csak utána megy le a részletekig. Ennek megfelelően a sorrendben hamarabb következnek a nagyobb léptéket megvalósító metódusok. Van, hogy egy “fő metódust” közvetlenül követnek az őt kiszolgáló segéd metódusok, van hogy az összes segédmetódus az osztály aljára szorul – de semmi értelme egy segéd metódust az osztálydefiníció elejére tenni, mint ahova a GetFileList() került, így ez biztos, hogy rossz helyen van.

Button_Click() mint metódusnév

Ez egy nevesített Code Smell, mégpedig “Uncommunicative Name”. Egy metódus nevének a program érthetősége érdekében jól kell tükröznie, hogy mire használható a metódus.

Hosszú metódus

A Button_Click() metódus jó 80 sor hosszú. A hosszú metódus szintén egy tipikus és nevesített Code Smell. Vannak programozók, akik ökölszabályokat használnak a metódusok maximális hosszára. Itt elég eltérő számok szoktak megjelenni, valaki szerint a 20 sor egy jó felső korlát, valaki pedig csak akkor ír 5 sornál többet, ha nagyon-nagyon muszáj.

Mások nem a sorok számával fejezik ki, hogy minek szabad egy metódusba beleférni. Ők azt mondják, hogy egy metódus a feladat egy absztrakciós szintjével foglalkozzon. Ez általában biztosítja a metódusok széttördelését. Hogy kell ezt érteni? Tegyük fel, hogy már refaktoráltuk a kódot, és a Button_Click() közepénél levő ciklus saját metódusban foglal helyet:

static IEnumerable<PersonNameWithLikeCount> CalculateLikeCountForEach(IEnumerable<Person> persons)
{
    var counts = new List<PersonNameWithLikeCount>();

    foreach (var person in persons)
    {
        var counter = 0;

        foreach (var candidate in persons)
        {
            if (candidate.LikedPersons.Contains(person.Name))
            {
                counter++;
            } // if
        } // foreach

        counts.Add(
            new PersonNameWithLikeCount()
            {
                Name = person.Name,
                LikeCount = counter
            });
    } // foreach

    return counts;
} // CalculateLikeCountForEach()

A metódus nem tűnik túlzottan hosszúnak. Azonban a metóduson belül több absztrakciós szinten dolgozunk. Az egyik szint a következő: “minden személyre számold ki, hogy hányan kedvelik”. Ez a külső foreach. A következő szint az “Egy adott emberre számold ki, hogy hányan kedvelik”. Ez a belső foreach. Végül a harmadik absztrakciós szint az “Ellenőrizd, hogy X szemény kedveli-e Y-t”.

Ha ragaszkodunk hozzá, hogy egy metódus csak egy absztrakciós szinttel foglalkozzon, akkor a következő módon refaktoráljuk a kódot:

static IEnumerable<PersonNameWithLikeCount> CalculateLikeCountForEach(IEnumerable<Person> persons)
{
    var counts = new List<PersonNameWithLikeCount>();

    foreach (var person in persons)
    {
        var count = CalculateLikeCountFor(person, candidates: persons);

        counts.Add(
            new PersonNameWithLikeCount()
            {
                Name = person.Name,
                LikeCount = count
            });
    } // foreach

    return counts;
} // CalculateLikeCountForEach()

static int CalculateLikeCountFor(Person person, IEnumerable<Person> candidates)
{
    var count = 0;

    foreach (var candidate in candidates)
    {
        if (CandidateLikesPerson(candidate, person))
        {
            count++;
        } // if
    } // foreach

    return count;
} // CalculateLikeCountFor()

static bool CandidateLikesPerson(Person candidate, Person person)
{
    return candidate.LikedPersons.Contains(person.Name);
} // CandidateLikesPerson()

Ha csak azért nem szeretnénk kicsi metódusokra vagdalni a kódot, mert egy olyan algoritmusról van szó, ahol számít a sebesség, ez nagyon ritkán éri meg. A fenti kód kb 1.5-2 százalékkal lassabb, mint az, ami egybe van gyúrva. Egyrészt a jitter a nagyon kicsi metódusokat inline-olja, azaz valójában mégsem lesz metódushívás. A CandidateLikesPerson() metódus esélyes erre. Ha valakinek mégis számít az 1.5%, akkor 4.5-ös .NET esetében lehetőség van erőltetni az inlining-ot a MethodImplOptions egy új, AgressiveInlining értékével:

[MethodImplAttribute(MethodImplOptions.AggressiveInlining)] 
static int CalculateLikeCountFor(Person person, IEnumerable<Person> candidates)

Többször hallottam már olyan véleményt, hogy a paraméterek átadása metódushívésoknál lassítja a kódot. Ez nagyon sokszor nem igaz. A jitter olyan kódot fordít, ami 32 bites rendszer esetén az első kettő, 64 bites rendszerben pedig az első 4 paramétert regiszterben adja át. A fenti felépítésű kódnál, mivel a person/candidate változók a hívónál is és a hívottnál is használatban vannak, a jitter próbálja úgy szervezni a regiszterek használatát, hogy a hívó eleve például az ecx-ben (64 biten rcx-ben) tartsa a person referenciákat. Ez azért előnyös, mert az első paraméter metódushívásnál az ecx-ben utazik, így a metódushívás előkészítésében a paraméterekkel nem kell külön foglalkozni. Az 1.5 százalék lassulást egyébként az új stack frame létrehozása és törlése okozza metódushívásonként.

Tömörítés LINQ-val

Algoritmusokban szintén kevéssé szívesen használt eszköz a LINQ, illetve az alatta dolgozó Enumerable extension metódusok. LINQ-val tényleg könnyű rejtett komplexitást vinni a kódba. Ha valaki azonban átlátja mi fut le mögötte, akkor a teljesítménye nem mindig rosszabb, mint egy IEnumerable-n végiggyalogló foreach ciklus:

static int CalculateLikeCountFor(Person person, IEnumerable<Person> candidates)
{
    return candidates.Where(c => CandidateLikesPerson(c, person)).Count();
}

Tagoló kommentek

A példakódban egy metóduson belül többször található az alábbihoz hasonló komment:

LikeStatCommentAsSeparator

Vannak, akik szerint egy jó kód magáért beszél, ebből kifolyólag egy jó kódba nem kellenek kommentek, nem kell hozzá dokumentáció. Mások, például akik valami komplexebb algoritmust implementáltak, vagy egy többkötetes specifikációból szedték össze az igényeket, nem ennyire szigorúak. Azok sem, akik kénytelenek együtt élni valamelyik keretrendszer hibáival, és a workaround-ok okait kommentálják.

Egy dolog biztos, a programozók többnyire lusták, így ha egy programozó kommentelésre szánta el magát, akkor a kód azon részén valami bonyolultság van. Ez a bonyolultság lehet indokolt, de előfordulhat, hogy csak nem kellően átgondolt a kód.

A taglaló kommentek, amelyek a példaprogramban is szerepelnek, abból adódnak, hogy a metódus több funkciót valósít meg – ez pedig nem szerencsés. Ekkor a funkciók látják egymás munkaváltozóit, ezzel olyan mértékű összenövés történhet, amit később nehéz már refaktorálni. Ekkor kezdenek el a fejlesztők például copy-paste-elni, ha a nagy funkcióhalmaz közepéből kell valami. Amikor egy programozó úgy érzi, hogy a metódust kommentekkel – esetleg dupla soremeléssel – tagolni kell, akkor ott valószínű a metódus több metódusra kell tördelni.

switch

Talán ez a legcsúnyább része a kódnak. Egyrészt a switch egy ciklusban van, viszont a ciklus nem változtatja a switch feltételét – a sourceType mindig ugyanaz. Másrészt a case ágak apró különbségeket leszámítva egyformák. Harmadrészt objektum orientált környezetben a switch-ek gyakran (nem mindig) kiválthatóak valamelyik polimorfizmust támogató eszközzel – például öröklődéssel vagy interfészekkel. A kód ezen a részén akkora a baj, hogy legközelebb egy külön cikkben foglalkozunk vele.

Magic numbers

A “Show top 10 liked” komment körül a kódban látható egy kódolt 10-es érték. Ez az egyik legrégebben megnevezett rossz programozási szokás, és Magic Number névre hallgat. Több típusa van a kódba égetett számoknak, más más problémákkal. Az egyik esetben a kódot olvasó szimplán nem tudja, hogy miért pont annyi az érték, hogyan jött ki, és mi történik ha hozzányúl. Más esetekben a szám értékének a jelentése világos, de számtalan helyen szerepel a programban, így könnyen kifelejtődik egy, ha át kell írni. A példaprogram magic number-e is ehhez a kategóriához tartozik. Vajon ha azt szeretnénk, hogy a top 15 érték jelenjen meg, elég itt átírni a kódot, vagy máshol is bele kell nyúlni?

LikeStatMagicNumber

Catch everything

A példakódban a program stabilitását biztosítandó, a következő kódot látjuk:

LikeStatCatchAll

Az egyik szabálya a hibakezelésnek, hogy egy kód csak azokkal a hibákkal foglalkozzon, amelyekkel tud kezdeni valamit. Mivel nehéz elhinni, hogy egy kód minden hibával tud kezdeni valamit, mindig gyanús egy catch, ami minden hibát elkap és lenyel.

A gyanú alapja az, hogy a hibakezelés egyik feladata, hogy a hibás működésből a program állapotát egy konzisztens állapotba állítsa vissza. A visszaállás minden kódnál mást jelenthet, de általában valamiféle takarításról, illetve a félig elvégzett módosítások “visszacsinálásáról” van szó. Ezután a program egy stabil állapotban folytathatja a futását. Ha egy hiba természetéről nem tudunk semmit, akkor kevéssé hihető, hogy biztosítani tudjuk azt, hogy a program konzisztens állapotban működjön tovább. Ha pedig a program nincs konzisztens állapotban, elképzelhető, hogy a továbbfutása további károkat okoz.

Emiatt az ismeretlen hibákat vagy nem szabad elkapni, vagy tovább kell dobni. Ekkor az alkalmazás esetleg kifagy (ha csak egy felsőbb szint le nem kezeli a hibát). Remélhetőleg az ilyen esetekre még tesztelés során fény derül, és meg lehet vizsgálni, hogy adott hibából hogy lehet helyreállni. Ha nem ez a helyzet, lehet építeni logokra – feltéve, hogy a program ír log-ot végzetes hiba esetén.

Ezzel el is értünk az egyik lehetőségre, amikor “legális” minden exception-t elkapni. Ha a program azért kap el minden exception-t, hogy logoljon, vagy biztonságba helyezze azokat az adatokat, amiket tud, akkor az rendben van, ugyanakkor az exception-t ilyenkor tovább kell dobni.

Másik legális eset, ha kontextust szeretnénk adni a hibához. Tegyük fel, hogy valahol a mélyben történt egy UnauthorizedAccessException. Aki kapott már ilyet, az tudja, hogy nem túl sok információ található egy ilyen exception-ben a hibáról. Ugyanakkor, ha egy felsőbb szinten tudjuk, hogy az XY file feldolgozása volt folyamatban Z művelethez, akkor valami saját exception osztályt használva ezt felruházhatjuk olyan adatokkal, ami kontextust ad a hibának, és így segíthet kideríteni az okot. Ekkor az eredeti exception-t el kell tenni az újonnan létrehozott exception InnerException property-jébe, és az új exception-t kell továbbdobni.

Néha – főleg szervizeknél – előfordulhat, hogy előre látható, hogy a kód leállása nagyobb problémát okoz, ekkor esetleg megérheti bevállalni annak a kockázatát, hogy az ismeretlen hibát is lenyeli az alkalmazás. Ilyenkor viszont gondosan kell eljárni, előfordulhat, hogy az alkalmazás sorozatban generálja ugyanazt a hibát számtalanszor – elkezdve darálni a log bejegyzéseket, és túráztatni a hibát okozó komponenst. Ha adott más eszköz, ezekben az esetekben is megéri engedni meghalni az alkalmazást, és az operációs rendszerre (service recovery) vagy az alkalmazásszerverre bízni az újraélesztés.

A forráskód

Ha valaki nem töltötte le a kódot az előző cikk leírása alapján, az alábbi sorokat kellett véleményezni:

public partial class MainWindow : Window
{
    private IEnumerable<string> GetFileList()
    {
        return FileSystem.GetFiles(
                    this.GetDatabasePath(),
                    this.GetSelectedSourceType());
    } // GetFileList()

    private void Button_Click(object sender, RoutedEventArgs e)
    {
        var sw = Stopwatch.StartNew();

        try
        {
            var filesToProcess = this.GetFileList();
            var sourceType = this.GetSelectedSourceType();

            var persons = new List<Person>();

            // Fetch all persons from every file
            foreach (var file in filesToProcess)
            {
                switch (sourceType)
                {
                    case "bin":
                        {
                            var parser = new BinaryParser();
                            parser.Open(file);

                            while (!parser.HasReachedEnd)
                            {
                                persons.Add(parser.GetPerson());
                            } // while

                            parser.Close();
                        } // case "bin"
                        break;

                    case "xml":
                        {
                            var parser = new XmlParser(file);
                            parser.StartParse();

                            Person parsedPerson;
                            while ((parsedPerson = parser.GetNextPerson()) != null)
                            {
                                persons.Add(parsedPerson);
                            } // while

                            parser.FinishParse();
                        } // case "xml"
                        break;
                } // switch
            } // foreach

            // Calculate like counts
            var counters = new List<PersonNameWithLikeCount>();

            foreach (var person in persons)
            {
                var counter = 0;

                foreach (var candidate in persons)
                {
                    if (candidate.LikedPersons.Contains(person.Name))
                    {
                        counter++;
                    } // if
                } // foreach

                counters.Add(
                    new PersonNameWithLikeCount()
                    { 
                        Name = person.Name, 
                        LikeCount = counter 
                    });
            } // foreach

            // Show top 10 liked
            var firstTen = (from p in counters
                            orderby p.LikeCount descending
                            select p).Take(10);

            UpdateScreen(firstTen, sw.ElapsedMilliseconds);
        }
        catch (Exception)
        {
        } // catch
    } // Button_Click()    
} // class MainWindow
  1. C# Tech. interjú – Naiv refaktor - pro C# tology - devPortal

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: