… czyli jak Marek funkcję upiększał. Jeśli po przeczytaniu tytułu i jego dokończenia w pierwszym zdaniu zacząłeś czytać również dalszą treść to znak, że albo wiesz czym jest refatoryzacja i chcesz zobaczyć co mogłem o niej napisać i co dziwnego popełniłem, albo jest to dla Ciebie nowe słowo i chcesz poznać jego znaczenie patrząc na moje przykłady. Niezależnie od powodu dla którego czytasz ten wpis mam zamiar dostarczyć Ci przynajmniej jednej nowej informacji pokazując jak sam podchodzę do tego tematu.Uważam, że dopiero spojrzenie na coś z wielu perspektyw daje dobry pogląd na sytuację, nawet jeśli jakieś przykłady i podejścia okazują się niewłaściwie. Jednak nie mając punktu odniesienia nie możemy określić czy coś jest wartościowe czy nie. Dlatego mam nadzieję, że przeczytanie tego tekstu wniesie coś nowego do Twojego postrzegania tematu refaktoryzacji. Jednak nie jest to jedyny powód dla którego ten wpis powstał. Inny powód to konieczność wyczyszczenia kodu w jednym z moich projektów, który piszę dla klienta i mała motywacja do tego, więc tworzenie tego wpisu jest niejako moim motywatorem. Tak, będę omawiał swoje techniki na przykładzie na prawdę działającego i znajdującego się w, jakby nie patrzeć, komercyjnej aplikacji kawałku kodu. Treść tutaj zawartą kieruję raczej do osób mniej zaznajomionych z tematem, ponieważ po pierwsze sam nie uważam się za osobę, która może doradzać prawdziwym zawodowcom, a po drugie takie osoby mają już na pewno wypracowane własne techniki, które się w ich przypadku sprawdzają.

Wstęp

Od razu może wspomnę, że swoją wiedzę opieram przede wszystkim na książce „Czysty kod. Podręcznik dobrego programisty” autorstwa Roberta C. Martina, jak również na doświadczeniu zdobytym w pracy, gdzie też korzystam z narzędzie Resharper, które kontroluje m.in. jakość pisanego kodu, a więc niejako zmusza do trzymania porządku wyświetlając co chwila żółte i czerwone ikonki kiedy coś mu się nie spodoba. Jednak oczywiście poza tymi dwoma źródłami będę posługiwał się czymś równie istotnym – doświadczeniem z własnych, nawet niewielkich programów. Bo wiadomo, że człowiek uczy się najlepiej na własnych błędach, a nic tak nie zmusza to zastanowienia się nad jakością swojej pracy jak konieczność wprowadzania w niej kolejnych zmian i usprawnień :D

W tym tekście skupię się na jednym z plików, a właściwie nawet na jego fragmencie, ponieważ cały plik ma (sic!) ~2000 linii, a uważam, że jednak wpisy na blogu nie powinny dorównywać książkom :D Jak widać już sama objętość tego pliku zmusza do zastanowienia się nad sensem życia, a to dopiero początek. To co za chwilę pokażę, jest fragmentem kontrolera w aplikacji webowej pisanej w technologii .NET i języku C#. Pewnie zapytacie „skoro wiesz jak pisać dobry kod to dlaczego stworzyłeś tego potwora?!” Powód jest prosty – mało czasu, dużo zmian. Projekt był trochę nieprzemyślany pod względem struktury, wszystko „projektowałem” na bieżąco, „byle działało”. I to jest właśnie powód dla którego praca pod dużą presją czasu daje na dłuższą metę gorsze rezultaty, bo co z tego, że jakaś wersja produktu powstanie w połowę krótszym czasie, skoro dodanie potem czegokolwiek lub zmiana w logice spowoduje, że zespół (albo jak w moim przypadku jedna osoba – autor), będzie bał się spojrzeć na monitor, a sama myśl o ingerencji w taki twór będzie powodowała nudności i chęć ucieczki? Tak, to jest myśl skierowana szczególnie do „szefów” projektów – lepiej dać więcej czasu na początku, żeby potem móc go odzyskać podczas dalszego rozwoju, kiedy czysty kod pozwoli wprowadzać zmiany dużo szybciej i prościej.

W dalszej części wpisu zakładam, że masz przynajmniej niewielkie pojęcie o języku C# i (w mniejszym stopniu) ASP.NET MVC. Ale myślę, że każdy pojętny programista zrozumie co się będzie działo mimo kilku skrótów myślowych i szybkich zmian.

Zaczynajmy

No dobra, to jak wygląda ten potwór, którego chcę zmienić? Ano tak:

public ActionResult AddVisit_Step3(NewVisitStep3 data, bool? edit)
        {
            if (data == null)
            {
                return RedirectToAction("Index");
            }

            var err = false;

            List<VisitSuppliers> suppliers = new List<VisitSuppliers>();
            if (data.Suppliers != null)
            {
                foreach (SelectedSuppliers s in data.Suppliers)
                {
                    var sup = new VisitSuppliers();
                    sup.SupplierId = s.Id;
                    sup.Text = s.Text;
                    suppliers.Add(sup);
                }
            }

            int sum = 0;
            List<VisitManufacturers> manufacturers = new List<VisitManufacturers>();
            foreach (SelectedManufacturers m in data.Manufacturers)
            {
                bool group = false;
                foreach (var g in data.ManufacturersGroups)
                {
                    if (g.Id == m.GroupId && g.SelectedManufacturers == true)
                    {
                        group = true;
                        break;
                    }
                }
                if (group == false)
                {
                    if (m.Checked)
                    {
                        sum += m.Percent;
                        var man = new VisitManufacturers();
                        man.ManufacturerId = m.Id;
                        man.GroupId = m.GroupId;
                        man.Percent = m.Percent;
                        manufacturers.Add(man);
                    }
                }
            }

            List<VisitManufacturersGroup> manuGroups = new List<VisitManufacturersGroup>();
            foreach (var g in data.ManufacturersGroups)
            {
                if (g.Checked && g.SelectedManufacturers == false)
                {
                    sum += g.Percent;
                    var group = new VisitManufacturersGroup();
                    group.Percent = g.Percent;
                    group.ManufacturersGroupId = g.Id;
                    group.SelectedManufacturers = g.SelectedManufacturers;
                    manuGroups.Add(group);
                }
            }

            err = sum != 100;

            var distributors = new List<VisitDistributor>();
            var visit = context.Visits.Single(v => v.Id == data.VisitId);
            var businessData = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == visit.BusinessId);

            if (err)
            {
                ModelState.AddModelError("", "Całkowita liczba procent musi wynosić 100%");
                @ViewBag.VisitId = data.VisitId;
                @ViewBag.Suppliers = context.Suppliers.ToList();
                @ViewBag.Countries = context.Countries.ToList();
                @ViewBag.Manufacturers = context.Manufacturers.ToList();
                @ViewBag.ManufacturersGroups = context.ManufacturersGroups.ToList();
                if (data.Suppliers == null)
                {
                    data.Suppliers = new List<SelectedSuppliers>();
                }

                var vis = context.Visits.Single(v => v.Id == data.VisitId);
                var bus = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == vis.BusinessId);

                return View(data);
            }

            ClientStep3 step = null;
            if (!edit.HasValue || edit.Value == false)
            {
                step = new ClientStep3();
            }
            else
            {
                step = context.ClientSteps3.Single(s => s.VisitId == data.VisitId);
                step.Manufacturers.Clear();
                step.ManufacturersGroups.Clear();
                step.Suppliers.Clear();
                context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                context.SaveChanges();
            }
            step.Cooperation = data.Cooperation;
            step.Manufacturers = manufacturers;
            step.ManufacturersGroups = manuGroups;
            step.Suppliers = suppliers;
            step.VisitId = data.VisitId;
            if (!edit.HasValue || edit.Value == false)
            {
                context.ClientSteps3.Add(step);
            }
            else
            {
                context.Entry(step).State = System.Data.Entity.EntityState.Modified;
            }

            context.SaveChanges();
            @ViewBag.VisitId = data.VisitId;
            return RedirectToAction("AddVisit_Step3_5", new { currentVisit = data.VisitId, edit = edit });
        }

I jak? Straszne prawda? Aż się płakać chcę. Jak widać jest to tylko jedna metoda, konkretnie akcja w kontrolerze. A jeszcze gorsze jest to, że nie wybrałem najbardziej rozbudowanej funkcji, chociaż znajduje się ona w czołówce rankingu.

Zadaniem tej funkcji jest przyjęcie danych z widoku, sprawdzenie dla części z nich warunku, że suma procent musi wynosić 100% oraz zapisanie wszystkiego do bazy. Dodatkowo jest tutaj uwzględniony również przypadek, w którym dane nie są tworzone, a edytowane w celu uniknięcia konieczności dublowania większości logiki, które by nastąpiło przy stworzeniu osobnej akcji dla edycji danych. Warto też dodać, że część ze znajdującej się tutaj funkcjonalności jest powielona w innej akcji ponieważ formularz wymagał podawania podobnych danych w kilku kontekstach. Zwieńczeniem całości są średnio dobre nazwy zmiennych, które prawdopodobnie zawierają czasami nawet błędy językowe, a już na pewno nie ułatwiają za bardzo zrozumienia całości. Jednak i tak dużym plusem jest brak zmiennych o nazwach 'aaa’, 'x’, 'xyz’ itp.

Czym jest refaktoryzacja?

Refaktoryzacja zazwyczaj kojarzona jest jedynie z poprawianiem czytelności kodu, upraszczaniem zapisu, wydzielaniem funkcjonalności czy oczyszczaniem plików źródłowych i taka jest w pewnym sensie definicja i rola refaktoryzacji. Jednak są przypadki kiedy może ona również przynieść korzyści jeśli chodzi o wydajność. Właśnie aplikacje webowe są tutaj dobrym przykładem ponieważ intensywnie korzystają z bazy danych, co wiąże się z kolejnymi zapytaniami jak również przeglądaniem sporej porcji danych, nastawione są też z definicji na przesyłanie informacji za pomocą sieci, która jak wiadomo ma ograniczoną prędkość. Dlatego może się zdarzyć, że poprawiając powyższy fragment zaoszczędzimy nieco mocy i/lub łącza np. ograniczając ilość zapisów do bazy i przenosząc cześć odpowiedzialności za filtrowanie bezpośrednio do zapytań, bo zazwyczaj zaufanie silnikowi bazodanowemu jest lepszym pomysłem niż przeglądanie całej kolekcji pobranych elementów w celu znalezienia kilku wybranych. Uwaga, tutaj osoby zaznajomione z tematem powiedzą, że mieszam pojęcie refaktoryzacji i optymalizacji. Prawda, wiem o tym, jednak takie łączenie stosuję z większym lub mniejszym powodzeniem u siebie, a jak zaznaczyłem na początku warto spojrzeć na temat z wielu punków widzenia.

Do dzieła!

Zacznijmy więc refaktoryzować powyższy, brzydki fragment kodu.

Na początek zajmiemy się rzeczami prostymi, łatwymi w modyfikacji. Najpierw coś co poznałem niedawno, czyli możliwość zamienienia pętli szukających czy w kolekcji znajduje się element spełniający podany warunek na wyrażenie Linq, które zdecydowanie skraca zapis, a czasami nawet poprawia jego czytelność. W podanym kodzie jest jedno takie miejsce, które od razu pozwala na taką zmianę:

bool group = false;
foreach (var g in data.ManufacturersGroups)
{
    if (g.Id == m.GroupId && g.SelectedManufacturers == true)
    {
        group = true;
        break;
    }
}

Więc zamieńmy powyższy fragment na jedną linijkę wyrażenia Linq:

bool hasGroup = data.ManufacturersGroups.Any(group => group.SelectedManufacturers && group.Id == m.GroupId);

O co tutaj chodzi? Pętka która wcześniej istniała miała za zadanie sprawdzić czy dany element ma przypisaną jakąkolwiek z grup. Tak więc w tym wypadku już sama nazwa funkcji Any() opisuje działanie utworzonej linijki. Jest ona częścią biblioteki Linq należącej do frameworka .NET, jako parametr przyjmuje wyrażenie lambda sprawdzające wymagany warunek. Jak widać zmieniłem też nazwę zmiennej na bardziej sensowną. Korzystanie z funkcji udostępnianych przez Linq dla wszelkich kolekcji jest często dobrym posunięciem przy upraszczaniu kodu. Ale trzeba uważać, żeby nie przesadzić, ponieważ musisz wiedzieć, że takie wyrażenie jest trudne w debugowaniu. Debugger traktuje je jako jedną instrukcję, przez to śledzenie zmian wartości może być wręcz niemożliwe.

Kolejna rzecz to odwrócenie warunków tak, żeby ograniczać zagnieżdżenia. Przedstawione poniżej dwa fragmenty właśnie pozwalają na taką zmianę:

foreach (SelectedManufacturers m in data.Manufacturers)
 {
     bool hasGroup = data.ManufacturersGroups.Any(group => group.SelectedManufacturers && group.Id == m.GroupId);
     if (hasGroup == false)
     {
         if (m.Checked)
         {
             sum += m.Percent;
             var man = new VisitManufacturers();
             man.ManufacturerId = m.Id;
             man.GroupId = m.GroupId;
             man.Percent = m.Percent;
             manufacturers.Add(man);
         }
     }
 }

 

foreach (var g in data.ManufacturersGroups)
{
    if (g.Checked && g.SelectedManufacturers == false)
    {
        sum += g.Percent;
        var group = new VisitManufacturersGroup();
        group.Percent = g.Percent;
        group.ManufacturersGroupId = g.Id;
        group.SelectedManufacturers = g.SelectedManufacturers;
        manuGroups.Add(group);
    }
}

Jak widać w oby przypadkach jakiekolwiek działanie na elemencie z kolekcji podejmujemy dopiero kiedy spełni określony warunek przez co powstaje nam zagnieżdżony w pętli if. Ale przecież najważniejszą częścią, istotą tych pętli jest właśnie operacja na elementach, dlaczego więc nie odwrócić warunków tak, żeby służyły do pomijania niechcianych iteracji? Spróbujmy:

foreach (SelectedManufacturers m in data.Manufacturers)
{
    bool hasGroup = data.ManufacturersGroups.Any(group => group.SelectedManufacturers && group.Id == m.GroupId);
    if (hasGroup == true || m.Checked == false) continue;

    sum += m.Percent;
    var man = new VisitManufacturers();
    man.ManufacturerId = m.Id;
    man.GroupId = m.GroupId;
    man.Percent = m.Percent;
    manufacturers.Add(man);
}

 

foreach (var g in data.ManufacturersGroups)
{
    if (g.Checked == false || g.SelectedManufacturers == false) continue;

    sum += g.Percent;
    var group = new VisitManufacturersGroup();
    group.Percent = g.Percent;
    group.ManufacturersGroupId = g.Id;
    group.SelectedManufacturers = g.SelectedManufacturers;
    manuGroups.Add(group);
}

Udało się wyeliminować zagnieżdżenia w pętli co sprawiło, że teraz od razu widać co jest istotą ich działania. Przy okazji w pierwszym przypadku połączyłem też dwa osobne ify w jeden. Co więcej w przypadku drugiej pętli można jeszcze bardziej uprościć kod stosując funkcję Where() dostępną dla kolekcji, która działa podobnie jak wyrażenie where znane z SQL. Jako parametr przyjmuje wyrażenie lambda tak jak funkcja Any():

foreach (var g in data.ManufacturersGroups.Where(group => group.Checked && group.SelectedManufacturers))
{
    sum += g.Percent;
    var group = new VisitManufacturersGroup();
    group.Percent = g.Percent;
    group.ManufacturersGroupId = g.Id;
    group.SelectedManufacturers = g.SelectedManufacturers;
    manuGroups.Add(group);
}

Teraz przyszła pora na zajęcie się fragmentami, które wymagają już trochę więcej pracy. Mam na myśli możliwość wyciągnięcia części kodu do osobnych funkcji, tak aby jak najbardziej zbliżyć się do kodu zgodnego z zasadą jednej odpowiedzialności. W tym celu utworzę nową klasę pomocniczą Step3Helper (Step3 ponieważ nawiązuje do nazwy akcji, która zaś oznacza trzeci krok w formularzu, w tym momencie mało istotne, ale wolę wyjaśnić żeby nikt nie zarzucił mi używania bezsensownych nazw). W niej dodam trzy funkcje statyczne, które będą odpowiedzialne za utworzenie obiektów typu VisitSupplier, VisitManufacturer i VisitManufacturersGroup. Wyglądają one tak:

public class Step3Helper
{
   public static VisitSupplier CreateSupplier(SelectedSuppliers data)
   {
        var supplier = new VisitSupplier();
        supplier.SupplierId = data.Id;
        supplier.Text = data.Text;

        return supplier;
    }

    public static VisitManufacturers CreateManufacturer(SelectedManufacturers data)
    {
        var manufacturer = new VisitManufacturers();
        manufacturer.ManufacturerId = data.Id;
        manufacturer.GroupId = data.GroupId;
        manufacturer.Percent = data.Percent;

        return manufacturer;
    }

    public static VisitManufacturersGroup CreateManufacturersGroup(SelectedManufacturersGroups data)
    {
        var group = new VisitManufacturersGroup();
        group.Percent = data.Percent;
        group.ManufacturersGroupId = data.Id;
        group.SelectedManufacturers = data.SelectedManufacturers;

        return group;
    }
}

Ich użycia nie będę pokazywał ponieważ jest to proste przypisanie wartości zwracanej przez funkcję do zmiennej.

Kod zaczyna powoli wyglądać w miarę przyzwoicie:

public ActionResult AddVisit_Step3(NewVisitStep3 data, bool? edit)
        {
            if (data == null) return RedirectToAction("Index");

            var err = false;

            List<VisitSupplier> suppliers = new List<VisitSupplier>();
            if (data.Suppliers != null)
            {
                foreach (SelectedSuppliers s in data.Suppliers)
                {
                    var supplier = Step3Helper.CreateSupplier(s);
                    suppliers.Add(supplier);
                }
            }

            int sum = 0;
            List<VisitManufacturers> manufacturers = new List<VisitManufacturers>();
            foreach (SelectedManufacturers m in data.Manufacturers.Where(manufacturer => manufacturer.Checked))
            {
                bool hasGroup = data.ManufacturersGroups.Any(group => group.SelectedManufacturers && group.Id == m.GroupId);
                if (hasGroup == true) continue;

                sum += m.Percent;
                var manufacturer = Step3Helper.CreateManufacturer(m);
                manufacturers.Add(manufacturer);
            }

            List<VisitManufacturersGroup> manuGroups = new List<VisitManufacturersGroup>();
            foreach (var g in data.ManufacturersGroups.Where(group => group.Checked && group.SelectedManufacturers))
            {
                sum += g.Percent;
                var group = Step3Helper.CreateManufacturersGroup(g);
                manuGroups.Add(group);
            }

            err = sum != 100;

            var distributors = new List<VisitDistributor>();
            var visit = context.Visits.Single(v => v.Id == data.VisitId);
            var businessData = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == visit.BusinessId);

            if (err)
            {
                ModelState.AddModelError("", "Całkowita liczba procent musi wynosić 100%");
                @ViewBag.VisitId = data.VisitId;
                @ViewBag.Suppliers = context.Suppliers.ToList();
                @ViewBag.Countries = context.Countries.ToList();
                @ViewBag.Manufacturers = context.Manufacturers.ToList();
                @ViewBag.ManufacturersGroups = context.ManufacturersGroups.ToList();
                if (data.Suppliers == null)
                {
                    data.Suppliers = new List<SelectedSuppliers>();
                }

                var vis = context.Visits.Single(v => v.Id == data.VisitId);
                var bus = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == vis.BusinessId);

                return View(data);
            }

            ClientStep3 step = null;
            if (!edit.HasValue || edit.Value == false)
            {
                step = new ClientStep3();
            }
            else
            {
                step = context.ClientSteps3.Single(s => s.VisitId == data.VisitId);
                step.Manufacturers.Clear();
                step.ManufacturersGroups.Clear();
                step.Suppliers.Clear();
                context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                context.SaveChanges();
            }
            step.Cooperation = data.Cooperation;
            step.Manufacturers = manufacturers;
            step.ManufacturersGroups = manuGroups;
            step.Suppliers = suppliers;
            step.VisitId = data.VisitId;
            if (!edit.HasValue || edit.Value == false)
            {
                context.ClientSteps3.Add(step);
            }
            else
            {
                context.Entry(step).State = System.Data.Entity.EntityState.Modified;
            }

            context.SaveChanges();
            return RedirectToAction("AddVisit_Step3_5", new { currentVisit = data.VisitId, edit = edit });
        }

Przyjrzyjmy się teraz zmiennej err, która przechowuje informację czy wystąpił błąd. Po pierwsze jej nazwa jest raczej słaba. W końcu pełne słowo error ma tylko 2 znaki więcej. Ale inna sprawa to to, że jedyny moment, w którym może zmienić się wartość tej zmiennej to sprawdzanie czy suma jest równa 100, a potem jest ona używana tylko raz. W takim wypadku nie warto przechowywać wyniku tego porównania w osobnej zmiennej, tym bardziej, że jej nazwa nie mówi o typie błędu za jaki odpowiada. Wniosek jest jeden – pozbądźmy się jej. Dodatkowo przeniosę deklarację wszystkich zmiennych, które nie są tworzone dopiero w jakichś blokach, na samą górę funkcji, zaraz za sprawdzaniem czy dane odebrane przez akcję nie są puste.

Skoro tworzenie obiektów w pętlach przeniosłem do osobnych funkcji to dlaczego nie przenieść całych pętli w inne, wydzielone miejsce? Skróci nam to jeszcze bardziej długość metody, a w dodatku łatwiej będzie szukać ewentualnych błędów w przyszłości.

Wystarczy sprawdzić tylko jakich danych potrzebują pętle i można zabrać się za pisanie kolejnych funkcji, które ponownie znajdą się w klasie Step3Helper. Widać, że potrzebują one do działania danych przesłanych do akcji. Dodatkowo w dwóch z nich następuje sumowanie, dlatego w tym wypadku skorzystamy z modyfikatora out dostępnego w C# i zwrócimy sumę  przez parametr funkcji mimo, że są głosy twierdzące, że zwracanie czegokolwiek przez out jest złą praktyką.

Teraz metoda wygląda już zdecydowanie zwięźlej, z początkowych 119 linii kodu ma ona teraz tylko 64:

public ActionResult AddVisit_Step3(NewVisitStep3 data, bool? edit)
        {
            if (data == null) return RedirectToAction("Index");
            var sum = 0;

            IEnumerable<VisitSupplier> suppliers = Step3Helper.GetSuppliers(data);
            IEnumerable<VisitManufacturers> manufacturers = Step3Helper.GetManufacturers(data, ref sum);
            IEnumerable<VisitManufacturersGroup> manufacturersGroups = Step3Helper.GetManufacturersGroups(data, ref sum);
            var distributors = new List<VisitDistributor>();

            var visit = context.Visits.Single(v => v.Id == data.VisitId);
            var businessData = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == visit.BusinessId);

            ClientStep3 step = null;

            if (sum != 100)
            {
                ModelState.AddModelError("", "Całkowita liczba procent musi wynosić 100%");
                @ViewBag.VisitId = data.VisitId;
                @ViewBag.Suppliers = context.Suppliers.ToList();
                @ViewBag.Countries = context.Countries.ToList();
                @ViewBag.Manufacturers = context.Manufacturers.ToList();
                @ViewBag.ManufacturersGroups = context.ManufacturersGroups.ToList();
                if (data.Suppliers == null)
                {
                    data.Suppliers = new List<SelectedSuppliers>();
                }

                var vis = context.Visits.Single(v => v.Id == data.VisitId);
                var bus = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == vis.BusinessId);

                return View(data);
            }
            
            if (!edit.HasValue || edit.Value == false)
            {
                step = new ClientStep3();
            }
            else
            {
                step = context.ClientSteps3.Single(s => s.VisitId == data.VisitId);
                step.Manufacturers.Clear();
                step.ManufacturersGroups.Clear();
                step.Suppliers.Clear();
                context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                context.SaveChanges();
            }
            step.Cooperation = data.Cooperation;
            step.Manufacturers = (ICollection<VisitManufacturers>)manufacturers;
            step.ManufacturersGroups = (ICollection<VisitManufacturersGroup>)manufacturersGroups;
            step.Suppliers = (ICollection<VisitSupplier>)suppliers;
            step.VisitId = data.VisitId;
            if (!edit.HasValue || edit.Value == false)
            {
                context.ClientSteps3.Add(step);
            }
            else
            {
                context.Entry(step).State = System.Data.Entity.EntityState.Modified;
            }

            context.SaveChanges();
            return RedirectToAction("AddVisit_Step3_5", new { currentVisit = data.VisitId, edit = edit });
        }

Następna zmiana to odwrócenie warunku sumy, co spowoduje przeniesienie kodu odpowiedzialnego za zwrócenie błędu na koniec funkcji. Sprawi to, że kod wykonywany przy prawidłowych danych będzie mógł być czytany jednym ciągiem, bez konieczności przeskakiwania w inne miejsca. Przy okazji pozbyliśmy się bloku else:

public ActionResult AddVisit_Step3(NewVisitStep3 data, bool? edit)
        {
            if (data == null) return RedirectToAction("Index");
            var sum = 0;

            IEnumerable<VisitSupplier> suppliers = Step3Helper.GetSuppliers(data);
            IEnumerable<VisitManufacturers> manufacturers = Step3Helper.GetManufacturers(data, ref sum);
            IEnumerable<VisitManufacturersGroup> manufacturersGroups = Step3Helper.GetManufacturersGroups(data, ref sum);
            var distributors = new List<VisitDistributor>();

            var visit = context.Visits.Single(v => v.Id == data.VisitId);
            var businessData = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == visit.BusinessId);

            ClientStep3 step = null;

            if (sum == 100)
            {
                if (!edit.HasValue || edit.Value == false)
                {
                    step = new ClientStep3();
                }
                else
                {
                    step = context.ClientSteps3.Single(s => s.VisitId == data.VisitId);
                    step.Manufacturers.Clear();
                    step.ManufacturersGroups.Clear();
                    step.Suppliers.Clear();
                    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                    context.SaveChanges();
                }
                step.Cooperation = data.Cooperation;
                step.Manufacturers = (ICollection<VisitManufacturers>)manufacturers;
                step.ManufacturersGroups = (ICollection<VisitManufacturersGroup>)manufacturersGroups;
                step.Suppliers = (ICollection<VisitSupplier>)suppliers;
                step.VisitId = data.VisitId;
                if (!edit.HasValue || edit.Value == false)
                {
                    context.ClientSteps3.Add(step);
                }
                else
                {
                    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                }

                context.SaveChanges();
                return RedirectToAction("AddVisit_Step3_5", new { currentVisit = data.VisitId, edit = edit });
            }

            ModelState.AddModelError("", "Całkowita liczba procent musi wynosić 100%");
            @ViewBag.VisitId = data.VisitId;
            @ViewBag.Suppliers = context.Suppliers.ToList();
            @ViewBag.Countries = context.Countries.ToList();
            @ViewBag.Manufacturers = context.Manufacturers.ToList();
            @ViewBag.ManufacturersGroups = context.ManufacturersGroups.ToList();
            if (data.Suppliers == null)
            {
                data.Suppliers = new List<SelectedSuppliers>();
            }

            var vis = context.Visits.Single(v => v.Id == data.VisitId);
            var bus = context.BusinessDatas.SingleOrDefault(b => b.BusinessId == vis.BusinessId);

            return View(data);
        }

Całość zaczyna już być przyzwoitym fragmentem kodu. Zajmiemy się teraz tym kawałkiem:

if (!edit.HasValue || edit.Value == false)
{
    step = new ClientStep3();
}
else
{
    step = context.ClientSteps3.Single(s => s.VisitId == data.VisitId);
    step.Manufacturers.Clear();
    step.ManufacturersGroups.Clear();
    step.Suppliers.Clear();
    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
    context.SaveChanges();
}
step.Cooperation = data.Cooperation;
step.Manufacturers = (ICollection<VisitManufacturers>)manufacturers;
step.ManufacturersGroups = (ICollection<VisitManufacturersGroup>)manufacturersGroups;
step.Suppliers = (ICollection<VisitSupplier>)suppliers;
step.VisitId = data.VisitId;
if (!edit.HasValue || edit.Value == false)
{
    context.ClientSteps3.Add(step);
}
else
{
    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
}

Mamy tutaj kilka rzeczy do zmiany. Pierwsze co rzuca się w oczy to dwukrotne sprawdzanie czy aktualna akcja wykonywana jest podczas edycji czy tworzenia nowego wpisu. Aby móc się pozbyć tej duplikacji warunku musimy jakoś rozwiązać problem wspólnej części kodu pomiędzy ifami. Moja propozycja jest więc taka aby przenieść tą cześć do osobnej funkcji i wywoływać w każdym przypadku niezależnie:

public static ClientStep3 AddDataToStep(ClientStep3 step, bool cooperation, IEnumerable<VisitManufacturers> manufacturers, IEnumerable<VisitManufacturersGroup> groups, IEnumerable<VisitSupplier> suppliers, int visitId)
        {
            step.Cooperation = cooperation;
            step.Manufacturers = (ICollection<VisitManufacturers>)manufacturers;
            step.ManufacturersGroups = (ICollection<VisitManufacturersGroup>)groups;
            step.Suppliers = (ICollection<VisitSupplier>)suppliers;
            step.VisitId = visitId;

            return step;
        }

 

if (sum == 100)
            {
                if (!edit.HasValue || edit.Value == false)
                {
                    step = new ClientStep3();
                    step = Step3Helper.AddDataToStep(step, data.Cooperation, manufacturers, manufacturersGroups, suppliers, data.VisitId);
                    context.ClientSteps3.Add(step);
                }
                else
                {
                    step = context.ClientSteps3.Single(s => s.VisitId == data.VisitId);
                    step.Manufacturers.Clear();
                    step.ManufacturersGroups.Clear();
                    step.Suppliers.Clear();
                    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                    context.SaveChanges();
                    step = Step3Helper.AddDataToStep(step, data.Cooperation, manufacturers, manufacturersGroups, suppliers, data.VisitId);
                    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                }

                context.SaveChanges();
                return RedirectToAction("AddVisit_Step3_5", new { currentVisit = data.VisitId, edit = edit });
            }

Już prawie koniec

Kod akcji staje się coraz krótszy i zwięźlejszy. Ostatnie co jeszcze planuję zmienić to przenieść część kodu znajdującego się w bloku else do osobnej funkcji oraz dopisać bardzo pomocne funkcje do pobierania zmiennych visit i businessData z bazy ponieważ będą one używane w bardzo wielu miejscach.

Zapewne zastanawiasz się teraz jaki sens ma przenoszenie tylu kawałków kodu do osobnych funkcji, przecież nie zmniejsza to jego ilości, a wręcz ją zwiększa? Tak, to prawda, wydzielanie nowych klas i funkcji powoduje zwiększenie ilości kodu, ale kod który powstaje jest jednostkowo prostszy niż jedna duża funkcja, która robi wszystko. Poza tym często wydzielone fragmenty mogą być wykorzystane w innych miejscach aplikacji. Nawet jeśli tworząc je nie przyszło Ci to do głowy to gwarantuję, że w trakcie dalszej refaktoryzacji możesz trafić na bloki kodu, które mogą być zastąpione przez wywołanie już napisanych metod, bo w większości aplikacji pewne czynności są potrzebne w kilku miejscach.

Ok, ale miałem pisać ostatnie już funkcje. Najpierw blok else. Tak jak było do tej pory tworzę metodę w klasie Step3Helper i przenoszę do niej odpowiedni wycinek logiki modyfikując ją tak, żeby mogła być użyta wewnątrz osobnej funkcji:

public static ClientStep3 GetAndClearStep(int visitId, CerbudContext context)
        {
            var step = context.ClientSteps3.Single(s => s.VisitId == visitId);
            step.Manufacturers.Clear();
            step.ManufacturersGroups.Clear();
            step.Suppliers.Clear();
            context.Entry(step).State = System.Data.Entity.EntityState.Modified;
            context.SaveChanges();
            return step;
        }

W miejscu gdzie te linijki się znajdowały wstawiłem wywołanie funkcji GetAndClearStep().

Pewnie zauważyłeś, że zawsze jak potrzebuję contextu, czyli obiektu odpowiedzialnego za pośrednictwo z bazą danych to przyjmuję go jako parametr funkcji, a nie tworzę na nowo wewnątrz. Robię tak ponieważ wolę przekazywać jeden obiekt dla całej akcji niż mieć ich kilka bo oszczędzam przez to odrobinę zasobów. A każda utworzona instancja contextu tworzy kolejne połączenie z bazą danych dodatkowo zajmując pamięć serwera. Dla kliku osób odwiedzających stronę jednocześnie to nie będzie duża różnica, jednak jeśli można to należy zwracać na takie rzeczy uwagę w odniesieniu do większej skali, przy kilkuset osobach różnica zrobi się już trochę większa. I nie nakłaniam tutaj do przedwczesnych optymalizacji, które oczywiście są złe, ale staram się pokazać, że bez wysiłku można wyeliminować konieczność zmian w przyszłości.

W tym momencie udało się osiągnąć coś czego brakowało na początku – funkcja w całości mieści się na jednym ekranie. Mimo, że mam aż 1080 pikseli w pionie i niejeden mi zarzuci, że przecież ktoś może mieć mniejszy ekran to jednak myślę, że pracując samemu najważniejsze żeby nam to pasowało. Owszem, można dalej zmieniać ten kod i rozbijać go na mniejsze bloki. Zapewne będę to robił z czasem, przy kolejnych iteracjach refaktoryzacji. Ale w tym momencie patrzę na całość aplikacji i wiem, że nawet zmieszczenie funkcji na dużym ekranie jest sporym sukcesem i zdecydowanie ułatwi późniejszą pracę z nią.

Zostało teraz tylko dopisanie ostatnich funkcji, o których wspominałem służących do pobierania zmiennych visit i businessData. Tutaj nic zaskakującego się nie dzieje poza tym, że nie wylądują one w klasie Step3Helper, ale po prostu Helper, bo jak mówiłem, będą mogły być użyte w wielu innych miejscach programu, a nazwa Step3Helper sugerowałaby ich ścisły związek z jedną metodą.

Ostatecznie funkcja, nad którą pracowaliśmy w tym wpisie prezentuje się następująco:

public ActionResult AddVisit_Step3(NewVisitStep3 data, bool? edit)
        {
            if (data == null) return RedirectToAction("Index");
            var sum = 0;

            IEnumerable<VisitSupplier> suppliers = Step3Helper.GetSuppliers(data);
            IEnumerable<VisitManufacturers> manufacturers = Step3Helper.GetManufacturers(data, ref sum);
            IEnumerable<VisitManufacturersGroup> manufacturersGroups = Step3Helper.GetManufacturersGroups(data, ref sum);

            var visit = Helper.GetVisit(data.VisitId, context);
            var businessData = Helper.GetBusinessData(visit.BusinessId, context);

            ClientStep3 step = null;

            if (sum == 100)
            {
                if (!edit.HasValue || edit.Value == false)
                {
                    step = new ClientStep3();
                    step = Step3Helper.AddDataToStep(step, data.Cooperation, manufacturers, manufacturersGroups, suppliers, data.VisitId);
                    context.ClientSteps3.Add(step);
                }
                else
                {
                    step = Step3Helper.GetAndClearStep(data.VisitId, context);
                    step = Step3Helper.AddDataToStep(step, data.Cooperation, manufacturers, manufacturersGroups, suppliers, data.VisitId);
                    context.Entry(step).State = System.Data.Entity.EntityState.Modified;
                }

                context.SaveChanges();
                return RedirectToAction("AddVisit_Step3_5", new { currentVisit = data.VisitId, edit = edit });
            }

            ModelState.AddModelError("", "Całkowita liczba procent musi wynosić 100%");
            @ViewBag.VisitId = data.VisitId;
            @ViewBag.Suppliers = context.Suppliers.ToList();
            @ViewBag.Countries = context.Countries.ToList();
            @ViewBag.Manufacturers = context.Manufacturers.ToList();
            @ViewBag.ManufacturersGroups = context.ManufacturersGroups.ToList();
            if (data.Suppliers == null)
            {
                data.Suppliers = new List<SelectedSuppliers>();
            }

            return View(data);
        }

Prawie 3 razy mniej kodu w jednej funkcji w porównaniu z początkowym stanem. Sam oceń czy to dobry wynik, moim zdaniem tak, chociaż mógłby być zawsze lepszy, ale do perfekcji zawsze się dąży, a rzadko się ją osiąga.

Zakończenie

Gratuluję dotarcia do tego momentu. W tym, jak dotąd, najdłuższym moim wpisie pokazałem w jaki sposób ja podchodzę do tematu refaktoryzacji. Zastosowałem przykład wzięty z życia, bo po co wymyślać na siłę coś co ma się już gotowe. Tworzenie tego posta zmotywowało mnie także do faktycznego poprawienia fragmentu kodu, z czego relację przecież zdawałem. Nie twierdzę, że jest to najlepszy podejście, ba, wręcz uważam, że nie plasuje się ono w czołówce. Jednak mimo to uważam, że jeśli ktoś chce się nauczyć refaktoryzować to wyciągnie pewne wnioski z przeczytanego tekstu i sam wypracuje swoje techniki. Najważniejsze, żeby w rezultacie zostawiać kod, który chce się czytać i który pozwala na bardzo łatwe rozszerzanie i wprowadzanie nowych funkcjonalności i zmian. To co tutaj mogłeś zobaczyć to jedynie mały fragment magii jaką można poczynić w brzydkich plikach źródłowych. Polecam też zapoznać się z narzędziami wbudowanymi w Twoje środowisko programistyczne lub zewnętrznymi dodatkami takimi jak wspomniany na początku Resharper, które zdecydowanie ułatwiają pracę.

Jeśli podobają się wam tego typu wpisy, pokazujące fragmenty mojej pracy i mój „warsztat” to zostawiajcie komentarze lub piszcie maile, w miarę możliwości będę starał się tworzyć kolejne posty na takie tematy i będę miał do tego dużą motywację.