Najważniejsze aspekty, na które warto zwrócić uwagę podczas refaktoryzacji kodu - projekty komercyjne

Czesc,
mam takie pytanko do doswiadczonych kolegów. Jak robicie jakas implementacje, taska no i macie wypchnac na prke to na co zwracacie uwage jesli chodzi o czysty refactoring? Tak prosto z mostu mocniej technicznie, pytam z ciekawosci na co zwracacie uwage. Magic numbery, stałe, rozdzielenie odpowiedzialnosci, podzial na mniejsze metody, enkapsulacja, pod wzgledem optymalizacji czy da sie to zrobic w jednym zapytaniu do bazy, wiecie o co mi chodzi. Loggery, string formaty etc. Prosze bez jadu i uszczypliwosci, chcialbym poczytac, moze jest cos na co moglbym zwracac wieksza uwage a nigdy nie zwracalem. Z góry bardzo dziekuje:>

Jak masz IDE to zwykle wyświetli Ci hinty na temat kodu - ogarnięcie tego to już 99% sukcesu.
Większość z tego co napisałeś powinno być sprawdzane automatycznie w czasie buildu/CI (jakieś checkstyle, findbugi, sonar itp.).

Co do optymalizacji - czy da sie to zrobic w jednym zapytaniu do bazy to nie polecam, czasem jest tak, że 100 zapytań do bazy i tak działa sprawniej niż jedno grube :-) (znam takie przypadki - choć fakt, że to mocno nietypowe).

Zasadniczo najgorsze jest wrzucanie jakichś niedoróbek, które powstają podczas pracy - warto sobie wpisywać TODO czy FIXME. Ale przy co którymś większym PR zawsze coś głupiego zakomituje - bywa, normalka.

No i ogólnie, nie wiem co robisz, ale testy to standard. Sprawdź pokrycie i ew. dopisz co trzeba.

W sumie najczęściej to chyba wydzielam część logiki do osobnych klas lub metod.

Przeczytaj książkę "Working effectively with legacy code" od Michael Feathers, https://www.amazon.com/Working-Effectively-Legacy-Michael-Feathers/dp/0131177052

Musisz wiedzieć dlaczego dany kawałek kodu ci się nie podoba. Jak już to wiesz to wymyślasz parę rozwiązań na refaktor i wybierasz najlepsze. Niestety programowanie to sztuka pisania kodu dla ludzi i bez pochłonięcia ogromnej ilości kodu i walki z tym złym nie wyrobisz sobie dobrej intuicji

Niestety rady takie jak nie używaj magic numberów albo metody muszą być krótkie to tylko wskazówki, które niekoniecznie ci pomogą. Pij dużo wody może ci pomóc w schudnięciu, ale jak się skupisz tylko na tym to nie zauważysz problemu, że tyjesz przez żarcie słodyczy. Dobry kod z magic numberami będzie się czytało tak samo dobrze jak ten zrefaktorowany, złemu może to pomóc tylko trochę.

Moje doświadczenie mówi, że:

• niczym nieograniczone mutowanie stanu jest najgorsze. Jedna część aplikacji modyfikuje coś należące do innej i nie wiesz absolutnie co się dzieje. Oczywiście rozwiązaniem nie jest przepisanie wszystkich pętli for na map, bo najgorsze są zależności pomiędzy jedną dużą ilością kodu a drugą. Warto więc projektować interfejsy tak, żeby moduły wysokopoziomowe komunikowały się przez niemutowalne wartości
• dobra separacja kodu czystego/nieczystego
• zła abstrakcja przeszkadza w zauważeniu potencjalnej dobrej abstrakcji i zaciemnia kod. Tutaj niestety pomaga tylko intuicja i YAGNI
• warto myśleć o wielu rozwiązaniach danego problemu. Porównanie kilku rozwiązań może cię doprowadzić do sytuacji, że żadne ci się nie podoba przez co wiesz, że musisz myśleć dalej
• wysokopoziomowe testy są najważniejsze. Testowanie na niskim poziomie (klasa, metoda) ma sens, jeśli zakładamy (znowuż, intuicja), że dany kawałek kodu będzie stabilny. Z wysokopoziomowymi testami możemy orać aplikację z dużo większą produktywnością i pewnością
• warto mieć dobre pokrycie testami. Nie ma nic gorszego niż aplikacja, gdzie większość logiki to interakcja z bazą/API i same unit testy bazujące na mockach
• warto oddzielać refaktory od zmian w kodzie na poziomie comitów lub PRek. Nie ma nic gorszego, niż PR, gdzie masz zmian na 1000 linii, gdzie 900 to prosty refaktor a 100 to nowy ficzer. Reviewer przeglądając osobne zmiany będzie skupiony na innych aspektach kodu w zależności od tego czy dana zmiana powinna zmienić zachowanie czy nie
• warto myśleć o enkapsulacji i modularyzacji. Graf zależnosci aplikacji powinien być jak najprostszy, bo to ułatwia zwizualizowanie konkretnego kawałka kodu bez konieczności posiadania całego kodu w głowie

pod wzgledem optymalizacji czy da sie to zrobic w jednym zapytaniu do bazy, wiecie o co mi chodzi

Po prostu dobry monitoring. Sam kod nigdy ci nie powie, czy jest wolny czy nie, bo to od danych wejściowych a nie algorytmu bierze się wolny czas wykoniania. Znowuż podstawowe testy wysokopoziomowe pozwalają na łatwe uruchomienie profilera. Dzięki takim testom też możesz robić bardzo proste benchmarki, co pozwala na eksperymentowanie na zasadzie zobaczmy jak się zmieni czas wykonania jak zrobię to w jednym zapytaniu. Polecam https://users.ece.utexas.edu/~adnan/pike.html

Loggery, string formaty etc

Polecam dobre testy wysokopoziomowe. Odpalasz test z logami, który nie przechodzi i zadajesz sobie pytanie czy jak to wrzucę w takim stanie na produkcję to będę w stanie szybko ogarnąć co i dlaczego nie działa z logów?.

Zazwyczaj ludzie nie czytają logów, które sami piszą co powoduje, że czegoś brakuje/czegoś jest za dużo albo skorelowanie powiązanych ze sobą lini jest niemożliwe. Od siebie polecam structured logging z dwóch powodów:

• pisze się je szybciej i nie trzeba konstruować zdania w języku naturalnym cacheName=foo refreshType=forced "cache refresh started" jest prostsze to napisania niż cache "foo" forced refresh started
• łatwiej dodać kontekst do loggera co pozwala na lepszą korelację kolejnych wiadomości.
• dużo prostsza filtrowanie logów, bo nie trzeba pisać regexów

Gdzieś kiedyś przeczytałem, żenie powinno się refaktorować kodu (przy okazji robienia innego zadania) który potem nie zostanie przetestowany w ramach zadania który robiłeś (bo finalnie okazuje się, że zmieniłeś kod który prawdopodobnie działa na taki który potencjalnie nie działa). Istotne imho też jest czy dany kod jest pokryty testami (jeśli nie jest to go nie refaktoruje na rympał).

Co do optymalizacji zapytań do db to tak jak @jarekr000000 i @slsy napisali skutki mog.ą być różne i warto byłoby się tu posiłkować twardymi danymi w postaci np. jakichś metryk czy warto coś optymalizować i jaki potem jest efekt tej optymalizacji.

RequiredNickname napisał(a):

Gdzieś kiedyś przeczytałem, żenie powinno się refaktorować kodu (przy okazji robienia innego zadania) który potem nie zostanie przetestowany w ramach zadania który robiłeś (bo finalnie okazuje się, że zmieniłeś kod który prawdopodobnie działa na taki który potencjalnie nie działa). Istotne imho też jest czy dany kod jest pokryty testami (jeśli nie jest to go nie refaktoruje na rympał).

Tak i nie.

Ogólnie jak sobie przeczytasz książki "Clean Code" (Robert Martin), albo "Tidy First" (Kent Beck), jak i wiele innych, to refaktorowanie kodu (nawet takiego którego akurat nie edytujesz) jest zalecane. Oczywiście trzeba dbać o to żeby refaktor nic nie zespół, i to można robić na trzy sposoby:

• Albo miej dobre testy automatyczne
• Albo testuj refaktor manualnie (słabe)
• Albo wykonuj tylko "IDE-refactoring", czyli jedynie refaktoring za pomocą akcji IDE (move, rename, extract, inline); bo one zazwyczaj nie wprowadzają błędów, zwłaszcza w statycznie typowanych językach.

Ale jeśli znajdujemy się w sytuacji w której nie ma testów niestety, to oczywiście odpowiedzią nie jest "nie refaktoruj" tylko odpowiedzią jest "napisz testy".

Co do optymalizacji zapytań do db to tak jak @jarekr000000 i @slsy napisali skutki mog.ą być różne i warto byłoby się tu posiłkować twardymi danymi w postaci np. jakichś metryk czy warto coś optymalizować i jaki potem jest efekt tej optymalizacji.

To na 100% najlepiej profiler.

Najbardziej zwracam uwagę żeby nie rozwalić tego co działa.

Kilka rad praktycznych:

• Refactor obowiązkowo w osobnym komicie, tak żeby dało się go łatwo wycofać.
• Lepiej nawet pójść o krok dalej i zrobić osobny PR na refactor, inni mogą nie docenić twoich "ulepszeń".
• Poza prostymi przypadkami np. użycie nieefektywnej struktury danych typu LinkedList zamiast ArrayList prawie nigdy nie próbuj dokonywać optymalizacji kodu. Optymalizacja to zawsze cykl: zmierz, zmień, zmierz, porównaj.
• Małe refaktory w obrębie klasy to bułka z masłem, IDE pomaga - zmiana nazwy metody, extract method, zmiana położenia parametrów, extract interface. Te są bezpieczne i można ich dowolnie używać podobnie jak rename na zmiennych czy metodach.

Reszta przychodzi z doświadczeniem. Generalnie nie zmieniaj kodu gdy nie rozumiesz jak działa. Dobiero gdy znasz dany fragment kodu, możesz stosować poważniejsze refactory np. przeoranie 5 klas na 3.

PS. Uwaga na zmiany logów. Na logach mogą być poustawiane alerty, lub jakieś zapytania - nie zmieniaj jeśli nie wiesz do czego dana loglinia jest używana. Na małych projektach ta uwaga jest nieistotna, ale widziałem duże gdzie np. pewne loglinie wyzwalały alerty czy inne alarmy. Inne z kolei były wciągane przez system do monitorowania bezpieczeństwa...

RequiredNickname napisał(a):

Gdzieś kiedyś przeczytałem, żenie powinno się refaktorować kodu (przy okazji robienia innego zadania) który potem nie zostanie przetestowany w ramach zadania który robiłeś (bo finalnie okazuje się, że zmieniłeś kod który prawdopodobnie działa na taki który potencjalnie nie działa). Istotne imho też jest czy dany kod jest pokryty testami (jeśli nie jest to go nie refaktoruje na rympał).

To mogłem pisać ja. Przeważnie walczę o to w projektach, żeby refactoringi robić na bieżąco w ramach zadania, a nie jako "tech debt" osobne zadania na później.
Zasadniczo z powodów "ekonomicznych".
Jak robisz zadanie i coś zmieniasz, to znaczy, że wgryzłeś się w kod (:-) ) i masz szansę zrobić refaktoring, który dodatkowo sprawdzisz od razu w praniu.

Co więcej - większość kodu w aplikacjach jest "stabilna", 90% leży sobie spokojnie, rzadko się tam coś zmienia, rzadko się przegląda - można ten kod zostawić w spokoju - niech sobie leży, brzydki, przestarzały itd.

Lepiej zainwestować w "gorącą" część kodu - to tu spędza się większość dnia, zmiany w tej części najlepiej wpływają na "morale".

jarekr000000 napisał(a):

RequiredNickname napisał(a):

Gdzieś kiedyś przeczytałem, żenie powinno się refaktorować kodu (przy okazji robienia innego zadania) który potem nie zostanie przetestowany w ramach zadania który robiłeś (bo finalnie okazuje się, że zmieniłeś kod który prawdopodobnie działa na taki który potencjalnie nie działa). Istotne imho też jest czy dany kod jest pokryty testami (jeśli nie jest to go nie refaktoruje na rympał).

To mogłem pisać ja. Przeważnie walczę o to w projektach, żeby refactoringi robić na bieżąco w ramach zadania, a nie jako "tech debt" osobne zadania na później.
Zasadniczo z powodów "ekonomicznych".
Jak robisz zadanie i coś zmieniasz, to znaczy, że wgryzłeś się w kod (:-) ) i masz szansę zrobić refaktoring, który dodatkowo sprawdzisz od razu w praniu.

Co więcej - większość kodu w aplikacjach jest "stabilna", 90% leży sobie spokojnie, rzadko się tam coś zmienia, rzadko się przegląda - można ten kod zostawić w spokoju - niech sobie leży, brzydki, przestarzały itd.

Lepiej zainwestować w "gorącą" część kodu - to tu spędza się większość dnia, zmiany w tej części najlepiej wpływają na "morale".

Niby to jest technicznie prawda; ale coś mi tutaj nie pasuje.

Czemu "sprawdzanie czy nie zepsułeś po refaktorze" miałoby być konieczne albo nawet spodziewane przy pracy z kodem (a nie samo w sobie)? Przecież jeśli jesteśmy poważnymi pracownikami i robimy refaktor, to powinniśmy się upewnić że nic nie zepsuliśmy podczas refaktorowania , np. uruchamiając testy automatyczne. To powinno dać nam wysoką, prawie 100% pewność że nic nie zepsuliśmy, na tyle dużą że możemy taką zmianę wdrożyć. Jeśli nie mamy pewności; to odpowiednim wyjściem nie jest "robienie refaktora podczas pracy nad featurem", tylko np. poprawienie zestawu testów albo znalezienie innego sposobu żeby wprowadzać bezpieczny refaktor.

Jakby, jasne - tworząc feature możemy sobie dodatkowo załatwić sprawdzenie tego refaktora, ale to nie powinno być konieczne. Sprawdzenie refaktora powinno się odbyć w momencie jego robienia, nie później.

Riddle napisał(a):

Czemu "sprawdzanie czy nie zepsułeś po refaktorze" miałoby być konieczne albo nawet spodziewane przy pracy z kodem (a nie samo w sobie)? Przecież jeśli jesteśmy poważnymi pracownikami i robimy refaktor, to powinniśmy się upewnić że nic nie zepsuliśmy podczas refaktorowania , np. uruchamiając testy automatyczne. To powinno dać nam wysoką, prawie 100% pewność że nic nie zepsuliśmy, na tyle dużą że możemy taką zmianę wdrożyć. Jeśli nie mamy pewności; to odpowiednim wyjściem nie jest "robienie refaktora podczas pracy nad featurem", tylko np. poprawienie zestawu testów albo znalezienie innego sposobu żeby wprowadzać bezpieczny refaktor.

Ekonomia 1.

Możemy mieć taką sytuacje jak moim projekcie: każdy mały błąd to ryzyko wielomilionowych strat. Mamy dobre pokrycie testatmi. Tony testów różnego typu, w tym "property based". Piszemy w Scali(+ Lintery) i Haskell co od razu eliminuje wiele klas błędów znanaych z Javy czy PHP (po prostu nie da się ich popełnić)... ale co z tego. Testy dobre, szansa na problem nikła, ale jak nastąpi to straty duże.

Z drugiej strony mamy jakieś proste stronki z prostymi serwisami.. i znowu - jak się wywali to straty będą, ale bez dramatu -> wtedy nie robi się tyle testów, bo nie jest to ekonomicznie uzasadnione.

To skrajne przypadki, ale wniosek jeden refaktor to zawsze jakieś ryzyko. I nieważne ile testów będzie -> Turing to świnia -> zmiany zawsze mogą skutkować nieoczekiwanymi błędami.

Tym niemniej, szansa dostrzeżenia potencjalnej problemu podczas zmiany ograniczonej do 200 linii kodu, który dopiero co przeglądaliśmy (bo robiliśmy jakiś ficzur) jest dużo większa niż podczas refaktoru czysto technicznego, gdzie kontekst dawno nam wyleciał z pamięci, a zmiany często wprowadza się przekrojowo w całym kodzie/sporej jego części.

Ekonomia 2.
Poprawianie jakiegoś zakurzonego modułu, gdzie dawno nikt nic nie ruszał, też kosztuje, a szanse, że komuś to się naprawdę przyda są nikłe.

Za to jeśli ruszasz jakiś fragment kodu (ficzer/bugfix) to jest duża szansa, że ktoś niedługo w tym miejscu coś znowu ruszy. Ulepszenia w takim "żyjącym" kodzie faktycznie wpływają na komfort pracy.

jarekr000000 napisał(a):

Możemy mieć taką sytuacje jak moim projekcie: każdy mały błąd to ryzyko wielomilionowych strat.

Dobry argument.

Jeśli w istotcie tak jest, że każdy mały błąd to wielomilionowe straty, to nie powinieneś mieć "dobrego pokrycia" i "dobrych testów". Powinieneś mieć 99.9999999% pokrycie oraz "bardzo, bardzo, bardzo dobre testy". I jeśli takie masz, to nadal może robić refaktor nie przy okazji feature'a, tylko tak po prostu.

Chodzi mi o to, że "robienie refaktoru tylko tam gdzie robisz feautre", ma być zabezpieczeniem przed błędami. Spoko. Ale ma to taką wadę, że nie robisz refaktorów które mógłbyś zrobić. Dlatego lepszym sposobem na zabezpieczenie się przed błędami jest pisanie dobrych testów - wtedy możesz robić refaktor wszędzie.

jarekr000000 napisał(a):

Ekonomia 2.
Poprawianie jakiegoś zakurzonego modułu, gdzie dawno nikt nic nie ruszał, też kosztuje, a szanse, że komuś to się naprawdę przyda są nikłe.

Ale to już jest nieadekwatne. Oczywistym jest, że bez sensu jest refaktorować moduł w którym nie ma zmian.

Najlepiej w ogóle nie robić refaktoru, to nie ma ryzyka wprowadzenia tym błędów - prodit!

jak nie wiesz co robisz to tego nie rób
jak zobaczysz w błędnym działaniu efekty np. istnienia magicznych numerków to wtedy to zgrokujesz i będziesz wiedział jak poprawić iwkiedy
a na razie pytaj kogoś doświadczonego w tym projekcie ( co ja gadam jak u ostatniego janusza za TL był zasiedziały junior który nie widział sensu w tym żeby tą samą funkcjonalność w kilku plikach do funkcji przenieść)

Riddle napisał(a):

Jeśli w istotcie tak jest, że każdy mały błąd to wielomilionowe straty, to nie powinieneś mieć "dobrego pokrycia" i "dobrych testów". Powinieneś mieć 99.9999999% pokrycie oraz "bardzo, bardzo, bardzo dobre testy". I jeśli takie masz, to nadal może robić refaktor nie przy okazji feature'a, tylko tak po prostu.

1. Pokrycie 100% kodu, branchy itp. nadal nie gwarantuje sukcesu
2. Koszt testów przy zwiększaniu ich skuteczności rośnie nieliniowo - gdzieś trzeba ustawić akceptowalny poziom ryzyka
3. My wyszliśmy nawet poza testy miejscami używając np. języków które nie są turing complete...
4. Gdzieniegdzie nawet mamy jakies formalne dowody poprawności (a to jest jeszcze bardziej kosztowne niż testy)
5. A i tak kuj - czasem coś ćwierknie :-)

Btw. stale podczas pracy robimy refaktoringi, dla mnie normalna część zadania.

Jeśli testy automatyczne nie zawsze złapią regresję i robisz testy manualane; a jednocześnie pomyłki są kosztowne; to faktycznie może wydawać się że nie warto robić refaktora poza feature'ami. 100% Zgoda.

Ale moim zdaniem "nie rób refaktora poza feautrem" to jest tylko pierwszy krok w stronę bezpieczeństwa. Kolejnym krokiem byłaby postawa "od teraz piszmy lepsze testy automatyczne, które wyłapią te regresje". Warto zrobić ten drugi krok.

jarekr000000 napisał(a):

Btw. stale podczas pracy robimy refaktoringi, dla mnie normalna część zadania.

To bardzo dobra postawa Ale refactoring powinno się dać móc robić również poza zadaniem, nawet w miejscu w którym nie masz feature'a do zrobienia. Da się to zrobić tylko jesli mamy testy automatyczne potrafiące wyłapać te same regresje (lub więcej) co manualne.

• trzeba się upewnić, czy nic nie zepsujemy
• im większa skala refaktoru, tym bardziej trzeba się zastanowić oraz konsultować z innymi.
  co innego zmienić coś w jednym module (np. zminimalizować duplikację), a co innego zaorać połowę aplikacji przepisując ją na nowo.

Magic numbery, stałe, rozdzielenie odpowiedzialnosci, podzial na mniejsze metody, enkapsulacja, pod wzgledem optymalizacji czy da sie to zrobic w jednym zapytaniu do bazy, wiecie o co mi chodzi.

Najlepszy refaktor to taki, który pozwala na rzeczy niemożliwe wcześniej np.

• kod, w którym jest takie spaghetti, że prosta zmiana wymaga tygodni ==> kod po refaktorze staje się utrzymywalny i łatwy do zmiany
• klasa, która robi 10 różnych rzeczy i zakłada że będą używane one razem. Ciężko używać takiej klasy do różnych celów ==> 10 różnych klas, gdzie sam decydujesz, co ci potrzebne
• dane/stan czegoś wala się po całej aplikacji i ciężko tym zarządzać => stan/dane są trzymane w jednym miejscu i można nim łatwo zarządzać, przenosić z jednej funkcji do drugiej, (de)serializować itp.

Czyli ogólnie mamy jakieś ograniczenia spowodowane kodem ==> po przerobieniu ograniczenia znikają.

Oczywiście nie każde założenia "co nam da refactor" muszą się spełnić. Można zrefaktorować z jednego słabego kodu na drugi słaby (bo myśleliśmy, że będzie lepiej). Więc to też trzeba wziąć pod uwagę. Ale nieudany refaktor można cofnąć. Ew. też można robić większy refaktor w iluś fazach, a nie że wszystko naraz zmieniasz.