clean code

Okazjonalne (regularne) czyszczenie kodu Resharperem

Posted on Updated on

Powiedzmy że mamy projekt w którym wcześniej nie znano Resharpera. Otwierając coś takiego “wszystko świeci” na różne kolory sugerując że mamy w kodzie wiele problemów. Całe moje doświadczenie programistyczne mówi mi, że warto wysprzątać tą oborę bo na wstępnie z automatu poprawi się wiele bugów i znacznie zwiększy czytelność.

I tutaj staje się przed problemem czy poprawiać wszystko? Czy tylko te ważne? Czy też przy okazji te automatyczne poprzez Alt+Enter?

Dziś przeleciałem solucję za pomocą opcji “Cleanup Code” (PPM na projekcie/solucji/katalogu). cleanup code in resharper

Żeby zacząć utworzyłem nowy profile i nazwałem go “just remove unused references”, i jak widać na obrazku robi on tylko to (też je sortuje). Jest to “bezpieczny” niewymagający naszej ingerencji refactoring. Dzięki niemu gdy będziemy już z głową refaktorować kod (usuwać duplikacje, zmieniać strukture itp,itd, nie będziemy się rozpraszać rzeczami które może poprawić za nas automat.

Warto też od czasu do czasu jeszcze raz przelecieć tym automatem po całym solution, ponieważ takie rzeczy jeśli inne osoby nie zbają są regularnie zaśmiecane.

Muszę kiedyś napisać o podejściu “Resharper green”, może to rzuci trochę więcej światła.

Czytelność kodu/nazw – przykłady

Posted on Updated on

Taki przykład w javascript(typescript):

private deepClone(item) {
    return jQuery.extend(true, {}, item);
}

to że akurat deep copy zrobi “jQuery.extend(true, {}, item)” to implementacja. Myśląc “implementacja” myślę “szczegół implementacyjny”, a dalej myślę i traktuję to jako “szczegół”. Czytając metodę która coś tam robi (akurat tutaj zmienia relację parent-child na indeksy, zeby klonowanie nie miało cykli, klonuje delikwenta i ustatwia z powrotem relacje), wystarczy nam wiedza że robimy “deepClone”. Nie tylko wystarczy, ale większa wiedza byłaby szkodliwa.

private changeParentRelationsToIds(item: ItemViewModel): ItemViewModel {
    // Cloned data must be without parent and children relations but must be added again after copy finishes
    let parent = item.parent;
    let children = item.children;

    item.parentId = this.changeToNullIfParentIsRoot(item.parent.id);
    item.parent = undefined;
    item.children = [];

    let clonedItem = this.deepClone(item);

    item.parent = parent;
    item.children = children;

    return clonedItem;
}

Przytoczona tutaj implementacja deepCopy nie jest jedyną. http://stackoverflow.com/questions/122102/what-is-the-most-efficient-way-to-clone-an-object to pytanie na SO ma 3 tysiące łapek w górę i jest o czym dyskutować w odpowiedziach i komentarzach. W innym kontekście mógłbym użyć innej implementacji bo aktualna nie działa. A ja nie chcę o tym wszystkim wiedzieć, w mojej metodzie robiącej coś konkretnego chcę wywołać:

let clonedItem = this.deepClone(item);

Code Review a spacje i inne pierdoły

Posted on Updated on

Chodzi o taką sprawę:

couple random spaces in Code Review

Te ciemno zielone to kilka dodatkowych spacji, które się pojawiło (a nie było ich wcześniej). (Inne to np, dodatkowo zbędna, podwójna linia, brak pustej linii, brak spacji w parametrach lub zbyt duża ich liczba, itp, itd.)

Przypadkowo podczas pracy nad kodem gdzieś się pojawiło kilka spacji za dużo lub za mało. Czy chcielibyśmy przypieprzać się do takich rzeczy na Code Review? – nie bardzo, są ciekawsze rzeczy do kminienia. Jak unikać takich przeszkadzajek.

  • Formatować kod przed commitem, a najlepiej na bieżąco, skrót Ctrl+k, Ctrl+d.
  • Jeśli powyższego nie możemy zrobić, bo ogólnie cały kod jest syfiasty gdy chodzi o wcięcią/formatowani to możemy formatować tylko kod który dodaliśmy. Zaznaczamy kod i ten sam skrót. To akurat robię rzadko.
  • Jeśli już nie możemy (albo nie chcemy) korzystać z automatycznego formatowania to przed pushowaniem do centralnego repozytorium powinniśmy oczyścić commity z takich przeszkadzajem (git rebase interactive)
  • Już naprawdę w ostateczności gdy nic z powyższych nie podziałało to podczas tworzenia pull requesta (obszerny opis o co w tym biega na przykładzie Git/BitBucketa) zobaczmy wszystkie nasze zmiany. Na tym etapie wyłapiemy te pierdoły, na które będzie tracił czas każdy robiący Code Review (a czasem jest to kilka osób, których czas byśmy zmarnowali).

Innym podejściem jest włączeniem do projektu StyleCopa. Dzięki temu wymusimy pewne standardy formatowania, czyli brudną robotę odwali za nas narzędzie.

Nieużywane argumenty lambd

Posted on

ScrollView.ViewForZoomingInScrollView += x => PageImageView;

Jest tutaj nieużywany argument. Gdyby był potrzebny i wyglądało to np tak:

ScrollView.ViewForZoomingInScrollView += x => PageImageView(x);

to wszystko byłoby w porządku. Jeśli jednak mamy taki nieużywany możemy go sprytnie ominąć korzystając z podkreślenia (podłogi)

ScrollView.ViewForZoomingInScrollView += _ => PageImageView;

Dzięki temu od razu widzimy, że ten argument jest nieistotny i czytając nie musimy się na nim skupiać.

Zbyt długie linie – jak łamać?

Posted on

Tak może wyglądać automatyczne podzielenie linii przez R# gdy nie mieści mu się w zadanym limicie.

var applicationForm =
    _applicationFormService.GetApplicationForm(
        applicationState.GetFieldValueAsString(Furi.ReferenceNumber));

Co można zrobić z takim potworkiem?

Na przykład coś takiego:

var referenceNumber = applicationState.GetFieldValueAsString(Furi.ReferenceNumber);
var applicationForm = _applicationFormService.GetApplicationForm(referenceNumber);

Wiem, że może to wyglądać na małe oszustwo bo wprowadziliśmy dodatkową zmienną. To jednak nic złego. W ten sposób warto poprawiać kod.

Refactoring

Posted on

Ciężko temu refactoringowi nadać nazwę ale bardzo go lubię:

Przed:

if (attributeData != null)
{
    destinationProperty.SetValue(plainDestination, sourceProperty.GetValue(encryptedSource, null), null);
}
else
{
    destinationProperty.SetValue(plainDestination, DecryptValue(encryptedSource, sourceProperty, destinationProperty), null);
}

Po

WIP

Nazywanie zmiennych w lambdach #2 (x versuss t,p,pi…)

Posted on

Podoba mi się konwencja aby nazyw zmiennych w lambdach nazywać ‘x’, jeśli potrzeba kolejnej ‘y’, a jeśli kolejnej (nie zdażyło mi się, pewnie mało widziałem, młody jestem ;)) to ‘z’. Poniżej na przykładach. Gdy jednak uważasz że trzeba je nazwać bardziej opisowo to odsyłam do Nazywanie zmiennych w lambdach.

Zła nazwa

advisers.Single(t => t.AdviserId == adviserId);

Zła nazwa

advisers.Single(a => a.AdviserId == adviserId);

Dobra nazwa

advisers.Single(x => x.AdviserId == adviserId);

Bardzo zła nazwa (i na pewno trzeba z czymś innym kojarzyć)

interns.Single(i => i.Id == choosenInternId);

Gdy trzeba więcej ‘literek’ użyć

products.Where(x => x.Categories.Any(y => y.Id == categoryId));

Kopiowanie kodu i wędrujace złe nazwy

Ludzie kopiuja kod. Nie jest to złe samo w sobie. Np trzeba wykorzystać zgrabne kilkulinijkowe Linq w innym miejscu, a uwspólnianie kodu nie ma sensu. Przy takim kopiowaniu nie sprawdza się najczęściej czy nazwy lambd są sensowne i contractor nie stał się nagle literką h. Gdy trzymamy się konwencji x,y,z to nie zacznie nam się to rozjeżdżać.