C# Tech. interjú – Naiv refaktor

Az előző cikkben a Code Smell-ek tárgyalásánál kihagytuk a switch-t tartalmazó részt. Most megvizsgáljuk, mi a baj ezzel a kódrésszel:

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

var persons = new List<Person>();

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

Mi az, ami feltűnik? Az, hogy van két case ág, nagyon hasonló, de mégis helyenként eltérő programkóddal. Ha eltekintünk attól, hogy a switch az csúnya vagy szép, a részben duplikált kód akkor sem jó.

Mit csinál a két case ág? Lényegében ugyanazt:

  • létrehoz egy típusnak megfelelő parser-t.
  • jelzi a parser-nek, hogy készüljön fel a parse-olásra.
  • parse-olja az adatokat, amíg van.
  • jelzi a parser-nek, hogy többet nincs rá szükség.

Amit találtunk tehát az, hogy van két hasonló funkciójú osztály másféle interfésszel. Ez egy nevezetes probléma, a neve pedig a hosszú, de legalább kifejező “Alternative Classes with Different Interfaces”.

A parser-ek refaktorálása

Mivel a switch a parser-eket használja, és a parser-ek rosszul vannak tervezve, érdemes előbb a parser-eket rendbetenni, majd utána meglátjuk, milyen munka marad még a switch-en.

A szükséges parser műveletek

A case ágak kódja alapján a következőket műveletekre van szükség:

Létrehozás és inicializálás

Ha megnézzük a BinaryParser és az XmlParser kódját, látjuk, hogy létrehozás után mind a két osztálynak van egy plusz inicializációs lépése, az egyiknek az Open(), a másiknak a StartParse(). Ezen kívül a BinaryParser default konstruktorral dolgozik (azaz neki a C# fordító készít egy paraméter nélküli konstruktort), amíg az XmlParser egy paraméteres konstruktort használ, amiben azonban nem csinál lényeges műveletet.

internal class BinaryParser
{
   internal void Open(string path) ...

internal class XmlParser
{
   internal XmlParser(string path)
   {
       this.path = path;
   } // XmlParser()

   internal void StartParse() ...
Kell-e két fázis?

Felmerül a kérdés, hogy miért nem tudja a konstruktor használható állapotba hozni a parser-eket, azaz miért van szükség kétfázisú inicializációra? Ebből egyenesen adódik a kérdés, hogy egyáltalán mikor van szükség kétfázisú inicializációra?

Van néhány osztály a .NET-ben, ami kétfázisú inicializálást használ. A WCF szerviz hosztok például ilyenek. De ott rengeteg beállítási lehetőség van a hoszt létrehozása és megnyitása között, emiatt érthető az explicit Open() metódus.

Más esetekben szükség lehet lazy inicializálásra, mert az objektum a teljes készültségében túl sok, vagy drága erőforrást használ.

Valószínű található még néhány eset, ahol indokolt a kétfázisú inicializáció, de a parser-eket egyelőre egyszerű körülmények közé szánjuk. A “Keep it simple” jegyében így az Open/StartParse metódusokat megszüntetjük.

Lezárás

Mind a két parser implementáció rendelkezik olyan metódussal, amin keresztül megtudhatja, hogy nincs tovább szükség a munkájára. Ezt mind a két parser ki is használja, hiszen file-okon dolgoznak, így azokat le tudják zárni.

Van a .NET-ben szabványos módszer arra, hogy egy példánynak kifejezzük, hogy nincs már rá szükség. Ez az IDisposable interfészen keresztül elérhető Dispose() metódus. Némelyik osztály ezt egy Close() metódussal csomagolja be, jellemzően azok, amelyek használatához hagyományok miatt hozzátapadt a close fogalma – ilyenek a file-ok, hálózati kapcsolatok. Máskor a szimmetriára való törekvés jegyében kap a Dispose() új nevet, pl Open/Close vagy Connect/Disconnect. A mi osztályunk nem ilyen (bár belül file-okra támaszkodik), ezért a Dispose tökéletesen megteszi. Másrészt a programban várhatólag egy “using” konstrukcióban fogjuk használni, emiatt a Close() felesleges is lenne.

Adatok olvasása

Egy új person példány beolvasását mind a két esetben egy GetPerson() jellegű metódus végzi. Különbség van azonban abban, hogy hogyan értesít arról a parser, hogy nincs több parse-olható adat. Az egyik esetben a GetXX metódus null-t ad vissza, a másik esetben külön le kell kérdezni, hogy a parser elérte már az adatok végét.

Ezt a két működési stílust most egyesíteni kell, így meg kell vizsgálnunk, hogy melyik a használhatóbb. Tegyük fel, hogy azt a megoldást választjuk, ahol van HasReachedEnd jellegű property. A gond ezzel a megoldással, hogy nem lehet kizárni, hogy az osztály használója ilyen esetekben is tovább hívogatja a GetPerson() metódust. Emiatt erre a helyzetre mindenképpen fel kell készülni, például egy exception eldobásával, vagy akár egy null visszaadásával. Ha pedig a null visszaadását választjuk, lényegében feleslegessé válik a HasReachedEnd, hiszen ugyanazt a dolgot kétféleképpen fejezhetjük ki. Így mégis inkább a HasReachedEnd megszüntetése mellett döntünk.

Az interfész

A fentiek alapján az interfész elég egyszerű: az inicializálást nem tudjuk interfészen előírni, mivel a konstruktort választottuk az inicializáció egyetlen helyének. A lezárást adja az IDisposable, ebből kell örökölni. Ezen felül egyetlen művelet kell, amelyik beolvassa a következő person példányt. Mivel az interfész nem tud más olvasni, illetve a visszatérési érték típusából egyértelmű, hogy mit olvas, nem feltétlenül kell xxxPerson() jellegű név (mint GetNextPerson() vagy NextPerson()). Használhatunk szimpla Next()-et, vagy GetNext()-et, kinek mi az izlése:

internal interface IParser : IDisposable
{        
    Person GetNext();
}

Maga az IParser név egyébként elég általános, emiatt nem biztos, hogy hosszabb távon szerencsés. Másik oldalról, amíg csak Person-okat parse-olunk, felesleges túltölteni az interfészek/osztályok neveit Person-okkal (mint IPersonParser). Ez azért sem lenne jó, mert nem tudhatjuk, milyen irányba fejlődik a parser (mindig csak Person-t kell majd parse-olni egy IPersonParser-nak?)

A refaktorált parser-ek

A kicsi parser-eket nem különösebben nehéz átalakítani, hogy a fent megadott interfésszel rendelkezzenek. Az eredeti kódból copy-paste-elgetéssel 1-2 perc alatt megoldható. Az XML kódja a könnyebb, ugyanis ott a tényleges parse-olást a .NET keretrendszer végzi a teljes Person struktúrára. Ráadásul, ha nem sikerül neki tovább olvasni a stream-et, pont úgy ad vissza számunkra null-t, ahogy nekünk kell. A GetNext() implementáció így csak egy továbbhívás:

internal class XmlParser : IParser
{
    private static readonly string ItemTitle = "Person";

    private string path;
    private XmlReader xmlReader;
    private XmlSerializer xmlSerializer;

    public XmlParser(string path)
    {
        this.path = path;

        this.xmlReader = XmlReader.Create(this.path);
        this.xmlReader.MoveToContent();
        this.xmlReader.ReadToDescendant(XmlParser.ItemTitle);

        this.xmlSerializer = new XmlSerializer(typeof(Person));
    } // XmlParser()

    public void Dispose()
    {
        this.xmlReader.Close();
    }

    public Person GetNext()
    {
        return this.xmlSerializer.Deserialize(this.xmlReader) as Person;
    }
} // class XmlParser

A bináris parser esetében saját formát használtunk, emiatt a kód nagyobb és összetettebb:

internal class BinaryParser : IParser
{
    private FileStream fileStream;
    private BinaryReader binaryReader;

    internal BinaryParser(string path)
    {
        this.fileStream = File.OpenRead(path);
        this.binaryReader = new BinaryReader(fileStream);
    } // BinaryParser()

    public void Dispose()
    {
        this.binaryReader.Close();
    }

    public Person GetNext()
    {
        if (this.HasReachedEnd)
        {
            return null;
        } // if

        return GetPerson();
    } // Next()

    private Person GetPerson()
    {
        var result = new Person
        {
            LikedPersons = new List<string>()
        };

        result.Name = this.binaryReader.ReadString();

        var contactCount = this.binaryReader.ReadInt32();
        for (var i = 0; i < contactCount; i++)
        {
            result.LikedPersons.Add(
                this.binaryReader.ReadString());
        } // for i

        return result;
    } // GetPerson()

    private bool HasReachedEnd
    {
        get
        {
            return this.binaryReader.PeekChar() < 0;
        } // get
    } // HasReachedEnd
} // class BinaryParser 

A két osztály így még nem teljes, például le kellene kezelni a Dispose utáni hívásokat – most implementáció specifikus hibát dob az alsó réteg.

Vissza a switch-hez

Most, hogy a két parser ugyanolyan interfésszel rendelkezik, elvileg meg lehet szüntetni a kód duplikálást. Ebben az esetben a switch-re már csak azért van szükség, hogy a megfelelő típusú parser-t előállítsa. Első lépésben a következő formára tudjuk hozni a kódot:

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

var persons = new List<Person>();

foreach (var file in filesToProcess)
{
    IParser parser;

    switch (sourceType)
    {
        case "bin": parser = new BinaryParser(file); break;
        case "xml": parser = new XmlParser(file); break;

        default:
            throw new NotSupportedException(
                string.Format("Not supported format: {0}", sourceType));
    } // switch

    using (parser)
    {
        Person person;

        while ((person = parser.GetNext()) != null)
        {
            persons.Add(person);
        } // while()
    } // using
} // foreach

A kód így jobb, bár a switch nem tűnt el. Azonban a switch szerepe most már tiszta: az az egyetlen dolga, hogy a formátum információ alapján létrehozzon egy megfelelő parser-t. Ismerősen cseng ez a mondat? Valami alapján létrehozni valamit – ez a parameterized factory method tudása, amely az eredeti factory method egy variánsa. Bár a GoF factory method eredetileg öröklődésen és virtuális factory metóduson alapult, sokszor öröklődés nélkül, egy switch (vagy if) szerkezettel oldják meg – talán ez az a helyzet, ahol legkevesebben néznek ferde szemmel a switch-re.

Amit kaptunk tehát, az egy factory method belseje, csak a program eredeti ömlesztett formája miatt rossz helyen szerepel. Annyi a dolgunk, hogy kiszervezzük egy külön osztályba:

internal static class ParserFactory
{
    internal static IParser Create(string path, string formatType)
    {
        switch (formatType)
        {
            case "bin": return new BinaryParser(path);
            case "xml": return new XmlParser(path);

            default:
                throw new NotSupportedException(
                    string.Format("Not supported format: {0}", formatType));
        } // switch
    } // Create()
} // class ParserFactory

így az új ciklus:

foreach (var file in filesToProcess)
{
    using (var parser = ParserFactory.Create(file, sourceType))
    {
        Person person;

        while ((person = parser.GetNext()) != null)
        {
            persons.Add(person);
        } // while()
    } // using
} // foreach

A parser és a Single Responsibility

A most levezetett megoldásunk sokkal jobb, mint az eredeti, de még mindig vannak hiányosságai. A problémát az okozza, hogy a parser-ek több dolgot csinálnak. Ez a több dolog esetleg nem tűnik fel, és azért nem, mert nagyon gyakran vétett összefonódásról van szó: az osztály maga kezeli (pl maga nyitja meg) az adatforrását, és utána azt adatforrásból érkező adatokat értelmezi.

Az adatforrás kezelése alkalmazás logika, az adat értelmezése üzleti logika, így a több felelőség ráadásul külön nemű felelőség is. Ez azért probléma, mert az üzleti logika és az alkalmazás logika jellemzően külön változik.

Bár a KISS/YAGNI elvek miatt nem szabadna azon gondolkodnunk, hogy milyen jövőbeli változásokra hogyan tudunk felkészülni, most emlékeztetőül a Single Responsibility értelmére, mégis nézzünk egy példát: ha az alkalmazás a jövőben nem csak file-okból, hanem hálózatról vagy egyéb adatforrásról is akar adatot olvasni, akkor a parser-eket egyenként át kell irogatni. Az egyenként átírogatás érzékelteti, hogy több osztályt kell átírni, a több osztály átírása pedig azért kell, mert egy adott dolgot nem egy adott osztály csinál. Az adatforrás megnyitása egy külön felelőség, ezt most belekentük az összes parser-ba. A következő cikkben ezek alapján refaktoráljuk tovább a kódot.

  1. C# Tech. interjú – SOLID 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: