Programowanie

Linq zamiast foreach -> czyli jak poprawić stary kod.

Nowy artykuł o sposobie działania Linq w stosunku do tematu poniżej znajdziesz tu.

 

Ponieważ mam tą przyjemność refactorować czasem naprawdę stary kod (z czasów .NET 1) to trafiam na takie fajne fragmenty “jak to się robiło kiedyś”. Kod który jest pod moją opieką staram się poprawiać, testować i upraszczać używając najnowszych sztuczek. Do kodu!

W powyższym kodzie znajduje się kilka “złych rzeczy”. Jedną z nich, której od razu się pozbędziemy jest dziwny warunek w pętli. Ponieważ Bodies to lista elementów BodySectionBase to !(sec is BodySectionBase) = !(true), więc można ten element spokojnie wywalić.

Kolejna rzecz to zmienna która jest przecząca z “nazwy”. Zmienne tak zadeklarowane to czyste kłopoty. IsNonPaged (dokument nie jest podzielony na strony) po winien być zapisany za pomocą !isPaged. Tak zróbmy!

IsNonPaged jest zmienną globalną dla dość dużej klasy więc na razie nie będziemy z tym walczyć. Wymagałoby to wiele zmian w klasie, która nie ma testów. Ale zajmiemy się teraz pętlą. Co ona robi? Sprawdza, jeśli w Bodies są elementy typu GroupSection ustawia nam isPaged na true. Ale przecież Linq ma coś takiego…

Druga część pętli powiększa nam licznik nBodies, a potem sprawdza i jeśli jest on wyższy niż 1 ustawia isPaged = true. Czy możemy zapisać coś takiego prościej? Możemy!

Z góry przestrzegam by nie używać w takim wypadku w Linq Cast<>(), bo on rzutuje wszystkie elementy na wskazany typ. OfTypes<>() natomiast wyciąga elementy danego typu.

Zsumujmy to co otrzymaliśmy.

No i na koniec to co pozostało to pozbyć się zbędnej zmiennej isPaged oraz zbędnego ifa.

Moim zdaniem dzięki temu kod stał się bardziej czytelny, łatwiejszy w obsłudze i jest go o wiele mniej. A Wy co myślicie?

===== Edit =====

Jak ja lubię konstruktywne komentarze. Po podpowiedzi Revisa zmieniłem kod w ten sposób:

Dzięki tym założeniom, na razie nie sprawdzonym -> Any() powinno się zatrzymać przy pierwszym elemencie spełniającym warunek (pesymistycznie nie spełni żaden, więc iteruje po całości), a przy OfType<>() i Take(2) iterujemy tylko tyle elementów by zwrócić 2 czyli w najgorszym wypadku tylko raz… To powinno przyspieszyć. Rozwiązaniem najszybszym jest jak wskazał Revis zrobienie for i w tej pętli sprawdzenie warunków. Wtedy przebieg będzie tylko jeden, aczkolwiek kod nie będzie już taki fajny. Kwestia czytelności, bądź fajnego kodu względem wydajności…

 

A sposób działania Linq i jak można lepiej znajdziesz tu.

14 thoughts on “Linq zamiast foreach -> czyli jak poprawić stary kod.

  1. Pingback: dotnetomaniak.pl
  2. Śliczny rezultat 🙂 Takie poprawianie kodu jest jak gruntowny porządek w mieszkaniu – takiego cotygodniowego sprzątania nie chce się robić, bo w sumie niezbyt widać rezultat, ale jak tak sie porządnie odgraci to jest wielka radość 😀
    Z jednej strony Ci współczuje, że musisz babrać się w takim legacy kodzie, ale z drugiej strony fajnie tak czasem odgracić coś. 🙂

  3. Z fragmentu kodu nie wynika czy to co zmieniasz to pojedyncza metoda czy jakaś część jakiejś dużo większej metody, ale czy nie lepiej byłoby wydzielić taki kod jako małą metodę, napisać do niej testy (choćby zobaczyć co wygeneruje IntelliTest), a dopiero później go zrefaktoryzować ? Ponadto jak domniemam BodySectionBase to jakaś klasa bazowa, a GroupSection już niekoniecznie. W związku z tym wydaje mi się, że te dwa warunki w ifie mogą oznaczać, że kiedyś ktoś skopiował ten kod z innego miejsca i zmienił tylko typ. Co “śmierdzi” złamaniem zasady DRY i wartoby przeszukać kod jakimś narzędziem czy się nie powtarza 😉 Mimo wszystko efekt fajny 🙂

    1. Jest fragmentem dużo większej metody.Teoretycznie się zgadzam co do wyciągnięcia jako osobna metoda. Ale żeby nie łamać wielu zasad musiałaby to być metoda prywatna. A to oznacza, że nie ma jej co testować. Należy testować metodę co najmniej internal (a najchętniej public tylko). Przynajmniej wg. mojego uznania.

      1. Pewnie dlatego że dodając ! pojedyncza negacja jest naturalna, a negacja negacji zmusza do dodatkowego myślenia. Szczególnie jeżeli nie jest się pierwszym autorem kodu.

  4. Szczerze mówiąc to oryginalny kod jest dziwny. Jaki jest sens sprawdzac wszystkie elementy skoro już 1xGroup lub 2xBody są wystarczające. W każdym razie refaktoryzacja powiela ten problem. Pesymistycznie trzeba 2x przelecieć cała kolekcje. I na koniec skrót z &= czyli wywołujemy kod który niekoniecznie jest potrzebny.

    1. Zaktualizowałem wpis, ale pesymistycznie zawsze będzie iterował 2x po całej kolekcji. Masz jakiś pomysł jak to poprawić?

  5. Wychodzę tutaj z założenia, że LINQ jest fajne (nawet bardzo fajne), ale nie trzeba pchać go wszędzie. Prędzej wydzieliłbym kod do metody IsRootGroupPaged i tam zrobił jednak pętlę foreach. Za każdym obrotem analizując: if dla Group + break i if dla Body z licznikiem + break gdy > 1. Tak pesymistycznie obrócimy się maks 1x po wszystkich elementach sprawdzając każdorazowo 2/3 warunki.
    Dodatkowo, zapis |= podobnie jak &= jest operacją bitową i kod po prawej stronie zawsze się wykona. Zakładam, że chodziło Ci o uzyskanie efektu jak tutaj: isPaged = isPaged || IsRootGroupPaged(…). Czyli liczymy tylko kiedy to faktycznie jest potrzebne.

    1. Dziękuję – miałeś rację co do |= i &=. Poprawiłem ten kawałek. Co do powyżeszej propozycji. Zgadzam się z nią, ale jest to nic innego jak cofnięcie się do kodu który był poprzednio i poprawienie go. Na szczęście super wydajność nie jest kwestią w tym przypadku, wolę czytelność (bo IMHO kod linq jest czytelniejszy).
      Dla ciekawości: https://ayende.com/blog/173121/good-fast-pretty-code-how-to-choose

      1. Czytelność to już kwestia gustu. Znam osoby dla których LINQ to jakieś potworki
        A co do edycji po Twojej stronie uściślijmy jeszcze że połączenie OfType i Take nie zatrzyma się po 2 elementach. Przeleci cały zbiór wybierając obiekty z typem, a dopiero później weźmie 2. To są jednak operacje sekwencje w takim zapisie. Przydałby się Take z ilością i predykatem, a takiego o ile pamiętam nie ma w standardzie. Zawsze jednak można dopisać

        1. Tu znów muszę się z Tobą zgodzić 🙂 Nie będę jednak zmieniał już kodu. Otrzymałem wymaganą przeze mnie czytelność kosztem wydajności (nie względem poprzedniego rozwiązania, ale w ogóle), która jest do zaakceptowania.

Leave a Reply

Your email address will not be published. Required fields are marked *