Częste stosowanie final

0

Spotkałem się z tym że wiele osób dość często stosuje modyfikator final. Chodzi mi o to, że ja pewnie bym napisał poniższy kod bez final w przekazywanych parametrach- co dało autorowi użycie final w tych wszystkich miejscach ?

public class ReverseWord implements ReverseLetter {

    public static ReverseWord with(final ReverseLetter  reversePhrase) {
        return new ReverseWord(reversePhrase);
    }

    @Override
    public String apply(final String sentence) {
        final Iterable<String> words = Splitter.on(' ').split(sentence);
        return Joiner.on(' ').join();
    }
1

Tyle że przypadkiem nie nadpisze sobie argumentu funkcji gdzieś w kodzie.

0

@Shalom: i to jest dobra praktyka ? Czyli tak naprawdę warto podawać wszystkie argumenty w final bo jeżeli gdzieś w metodzie bedzie potrzeba zmiany to IDE wykryje ze na ta chwilę nie można bo jest final i wtedy się zastanowisz czy usunąć final czy coś źle piszesz.

1

To co piszesz brzmi mniej więcej tak: "Warto chodzić w ochraniaczach na spacer? Bo jeżeli wywalisz się na prostej drodze to przynajmniej Cię nie zaboli".

Mam na myśli to że jeszcze nigdy nie zdążyło mi się spędzić więcej niż pół sekundy zastanawiając się czy zmienić zmienną, i ten final nie jest wart zachodu. Optymalizator i tak go doda.

1

Stosowanie final dla pól klas ma duży sens i stosuje.

Robienie final class ma sens, ale o tym zapominam zwykle:/ ( i nikt na review nie przypomina).
Robienie metod nieprywatnych final ma troche sensu (jak klasa nie jest final). Ale zwykle zapominam:/. J.w.

Robienie zmiennych lokalnych i atrybutow final jest zwykle bez sensu. Metody powinny być na tyle krótkie, żeby effectively final było widać. Poza tym rzadko tu ktoś się myli.
Tylko, że akurat w praktyce, sadzę tych zbędnych finali mnóstwo :/ (witaj spójności...). Ręce same wpisują final (chciały val) Nie kasuję. Traktuje to trochę jak obsikanie kodu, są finale - czyli tu już byłem.

1
jarekr000000 napisał(a):

Robienie final class ma sens, ale o tym zapominam zwykle:/ ( i nikt na review nie przypomina).

Wtedy nie można zrobić:

return new HashMap<>() {{
    put("One", "Two");
    put("Three", "Four");
}};

Robienie metod nieprywatnych final ma troche sensu (jak klasa nie jest final). Ale zwykle zapominam:/. J.w.

Wtedy nie zrobisz decoratora - boże jak mnie to czasem wnerwia jak korzystam z gotowego rozwiązania, w którym ktoś nie przewidział case'a, chce sobie zmienić jedną metodę ale nie mogę bo ktoś zrobił final.

2
TomRiddle napisał(a):

Wtedy nie można zrobić:

return new HashMap<>() {{
    put("One", "Two");
    put("Three", "Four");
}};

Jak się przyjrzsz klasie HashMap to zobaczysz, że kluczowe metody tej klasy sa oznaczone final (np. putVal). Dzieki czemu jak dziedziczysz z tego HashMap,
to nie strzelisz sobie łatwo w stopę np. wkładając elementy, ale nie updatując size. Klasa HashMap jest przygotowana pod dziedziczenie. W związku z tym jej twórcy wykonali kawał dodatkowej pracy tak ją projektując, żeby się to dało robić bezpiecznie. (nie naruszając niezmiennika obiektów).

Pisząc kod przeważnie, nie wyobrażam sobie nawet, żeby ktoś po moich klasach dziedziczył i nie poświęcam dodatkowego czasu na takie zabezpieczenia i dla pewności jak nie zapomnę daje ten final. Lenistwo.

Robienie metod nieprywatnych final ma troche sensu (jak klasa nie jest final). Ale zwykle zapominam:/. J.w.

Wtedy nie zrobisz decoratora - boże jak mnie to czasem wnerwia jak korzystam z gotowego rozwiązania, w którym ktoś nie przewidział case'a, chce sobie zmienić jedną metodę ale nie mogę bo ktoś zrobił final

Akurat Dekoratora to możesz zrobić, bo dekorator właśnie pozwala Ci na obdekorowanie obiektu z klasy całkiem niedziedziczalnej. Po to jest. Zajrzyj do definijcji. https://en.wikipedia.org/wiki/Decorator_pattern

Generalnie twoje narzekanie na final można rozszerzyć na narzekanie na **private **. Możesz mieć pretensje, ze ktoś zrobił pole private i nie możesz łatwo (albo czasem wcale) pola zmienić. Ale to nie złosliwość, tylko autor klasy nie chciał rozwiązywać wszystkich możliwych problemow jakie wyjdą jeśli ktoś, z boku, będzie manipulował danym polem.
Im więcej ruchomych częsci tym więcej potencjalnych problemów do rozważenia. Tak samo, robiąc klasę final, ogłaszam: pewnie dziedzicząc z tego strzeliłbyś sobie w stopę, bo nie rozważałem co sie stanie jak podziedziczy i nie zabezpieczałem się, nie rób mi tego.
Jeśli jednak rozważałem, klasa nie jest final, to dla zabezpieczenia spójności obiektów (jak w HashMap) **musze **kilka kluczowych metod zrobić final. Reszta metod jest przewidziana do nadpisywania.

Tak dodatkowo uważam, że podejście kotlina jest o wiele fajniejsze. W javie brak final przy klasie oznacza: nie wiadomo co, może autor chciał żebyśmy po tym dziedziczyli, ale może po prostu zapomniał. W kotlinie domyślnym modyfikatorem jest final, jak stwierdzam, że przygotowałem się na dziedziczenie to daję modyfikator open. Wtedy wiadomo, że taka była moja intencja i poświęciłem chwilę czasu żeby rozważyć konsekwencje.

0

Stosowanie final w metodach może denerwować, ale chyba nie aż tak jak coś takiego:


void mojaSporaMetoda() {
// 200 linii
   wykonajPrivate();
// 300 linii
}

private void wykonajPrivate() {
   MojGenialnyUtil.obliczSumeWgWzoruX(); // static ofc
}

Mam podejrzenie że to taki zastępnik final.

0
jarekr000000 napisał(a):
TomRiddle napisał(a):

Wtedy nie można zrobić:

return new HashMap<>() {{
    put("One", "Two");
    put("Three", "Four");
}};

Jak się przyjrzsz klasie HashMap to zobaczysz, że kluczowe metody tej klasy sa oznaczone final (np. putVal). Dzieki czemu jak dziedziczysz z tego HashMap,
to nie strzelisz sobie łatwo w stopę np. wkładając elementy, ale nie updatując size. Klasa HashMap jest przygotowana pod dziedziczenie. W związku z tym jej twórcy wykonali kawał dodatkowej pracy tak ją projektując, żeby się to dało robić bezpiecznie. (nie naruszając niezmiennika obiektów).

Pisząc kod przeważnie, nie wyobrażam sobie nawet, żeby ktoś po moich klasach dziedziczył i nie poświęcam dodatkowego czasu na takie zabezpieczenia i dla pewności jak nie zapomnę daje ten final. Lenistwo.

Robienie metod nieprywatnych final ma troche sensu (jak klasa nie jest final). Ale zwykle zapominam:/. J.w.

Wtedy nie zrobisz decoratora - boże jak mnie to czasem wnerwia jak korzystam z gotowego rozwiązania, w którym ktoś nie przewidział case'a, chce sobie zmienić jedną metodę ale nie mogę bo ktoś zrobił final

Akurat Dekoratora to możesz zrobić, bo dekorator właśnie pozwala Ci na obdekorowanie obiektu z klasy całkiem niedziedziczalnej. Po to jest. Zajrzyj do definijcji. https://en.wikipedia.org/wiki/Decorator_pattern

Generalnie twoje narzekanie na final można rozszerzyć na narzekanie na **private **. Możesz mieć pretensje, ze ktoś zrobił pole private i nie możesz łatwo (albo czasem wcale) pola zmienić. Ale to nie złosliwość, tylko autor klasy nie chciał rozwiązywać wszystkich możliwych problemow jakie wyjdą jeśli ktoś, z boku, będzie manipulował danym polem.
Im więcej ruchomych częsci tym więcej potencjalnych problemów do rozważenia. Tak samo, robiąc klasę final, ogłaszam: pewnie dziedzicząc z tego strzeliłbyś sobie w stopę, bo nie rozważałem co sie stanie jak podziedziczy i nie zabezpieczałem się, nie rób mi tego.
Jeśli jednak rozważałem, klasa nie jest final, to dla zabezpieczenia spójności obiektów (jak w HashMap) **musze **kilka kluczowych metod zrobić final. Reszta metod jest przewidziana do nadpisywania.

Tak dodatkowo uważam, że podejście kotlina jest o wiele fajniejsze. W javie brak final przy klasie oznacza: nie wiadomo co, może autor chciał żebyśmy po tym dziedziczyli, ale może po prostu zapomniał. W kotlinie domyślnym modyfikatorem jest final, jak stwierdzam, że przygotowałem się na dziedziczenie to daję modyfikator open. Wtedy wiadomo, że taka była moja intencja i poświęciłem chwilę czasu żeby rozważyć konsekwencje.

Z trwogą przeczytałem tego posta. A już zwłaszcza końcówkę.

@jarekr000000: czy Ty sugerujesz, że przy projektowaniu klas trzeba myśleć? I, że trzeba świadomie decydować o tym, którym klasom pozwolić na bycie dziedziczonymi, a którym metodom na bycie nadpisywanymi?

Bo jak ja kiedyś napisałem, że autor klasy powinien przemyśleć, co pozwolić dziedziczyć, a czego nie, i że pozwolanie na nadpisywanie wszystkich metod w klasach potomnych jest złym pomysłem, to zostałem zakrzyczany przez tutejszego eksperta, że mam alergię na metody wirtualne, i że JVM ma tak wspaniale rozwiązaną dewirtualizację, że nie trzeba w ogóle się tym przejmować. A poza tym pozostawienie metod otwartymi na nadpisywanie pozwala na łatwe mockowanie, a nawet pisanie testowych implementacji, w których zmieniona jest tylko jedna metoda biznesowej klasy.

To jak to w końcu w Javie jest?

0

final w javie czy read only w c# służy mi głównie za dokumentację - dzięki temu patrząc na legacy code nie muszę się zastanawiać czy mogę ze spokojem użyć zmiennej czy muszę analizować cały kod w poszukiwaniu czy zmienna nie jest gdzieś modyfikowana lub czy jest bezpieczna do użycia w innym wątku.
podobnie za dokumentację mi służą final czy sealed na klasie

0
jarekr000000 napisał(a):

Tak dodatkowo uważam, że podejście kotlina jest o wiele fajniejsze. W javie brak final przy klasie oznacza: nie wiadomo co, może autor chciał żebyśmy po tym dziedziczyli, ale może po prostu zapomniał. W kotlinie domyślnym modyfikatorem jest final, jak stwierdzam, że przygotowałem się na dziedziczenie to daję modyfikator open. Wtedy wiadomo, że taka była moja intencja i poświęciłem chwilę czasu żeby rozważyć konsekwencje.

To podejście (znane z C++) wg mnie jest słabe. Ale to subiektywna opinia.
Ludzie nie odróżniają pisania kodu API (bibliotecznego) od pisania kodu systemu (zamkniętego na zewnątrz, ale rozszerzalnego).
Może w dobie RESTful API to ma mniejsze znaczenie, ale ogólnie dyskusje o private bez uwzględnienia tego są toczeniem piany po kilku głębszych.

Na rozluźnienie temat do przemyślenia: czy istnieje korelacja bycia fanem private-first z nie-posiadaniem konta na githubie?
Nie odnoszę się do nikogo tutaj personalnie (nikogo nie sprawdzałem).

2

Ta kwestia piszemy biblioteke czy kawałek webówki dla januszexu / banku czy innego korpo to jest ciekawa.
Ale w kwestii final widzę to prosto.
Jak jest to biblioteka to warto stosować, choćby aby przesztrzec użytkowników przed złym użyciem, które w przysżłości by nam przeszkadzalo robić zmiany.
A jak jesteśmy w naszym korpoprojekcie, to dla odmiany, też nic nie szkodzi stosować, bo kolega Zenobiusz z biurka obok zawsze tego finala sobie zdejmie jak będzie potrzeba, tylko przejrzy kod, sprawdzi czy to ma sens i ewentualnie spyta (*).

somekind napisał(a):

To jak to w końcu w Javie jest?

W javie stosowanie metod wirtualnych (domyślnie ) lub nie (final lub prvate) jest pdoyktowane tylko i wyłącznie względami designowymi, bo na szybkość nie ma wpływu w typowych warunkach (bym powiedział, że w typowym programie w C++ na nowszym hardware też już nie ma wpływu).
Natomiast co do stosowania final lub to jak widać są różne szkoly.
Lubie taki dopieszczony design, ale zwykle mam o wiele większe problemy niż tylko brak final.

1

Kiedyś oddawałem jakiś kod, i tak sobie policzyłem ile razy robię final. Wyszło że co 4 linijka ma final i to było rzeczywiście tzw. overkillem. Ale pola klas muszą byc final, i klasy tez powinny być final. Klasy z powodu dokumentowania tego czy są do dziedziczenia czy nie, a pola żeby zmiejszyc (wyeliminować?) mutowalność i możliwośc wywalenia czegoś przypadkiem.

0

Bo jak ja kiedyś napisałem, że autor klasy powinien przemyśleć, co pozwolić dziedziczyć, a czego nie, i że pozwolanie na nadpisywanie wszystkich metod w klasach potomnych jest złym pomysłem, to zostałem zakrzyczany przez tutejszego eksperta, że mam alergię na metody wirtualne, i że JVM ma tak wspaniale rozwiązaną dewirtualizację, że nie trzeba w ogóle się tym przejmować. A poza tym pozostawienie metod otwartymi na nadpisywanie pozwala na łatwe mockowanie, a nawet pisanie testowych implementacji, w których zmieniona jest tylko jedna metoda biznesowej klasy.

Podaj jakiś konkretny argument na to, że nadpisywanie metod w testach jest złe.

1
somekind napisał(a):

Bo jak ja kiedyś napisałem, że autor klasy powinien przemyśleć, co pozwolić dziedziczyć, a czego nie, i że pozwolanie na nadpisywanie wszystkich metod w klasach potomnych jest złym pomysłem, to zostałem zakrzyczany przez tutejszego eksperta, że mam alergię na metody wirtualne, i że JVM ma tak wspaniale rozwiązaną dewirtualizację, że nie trzeba w ogóle się tym przejmować. A poza tym pozostawienie metod otwartymi na nadpisywanie pozwala na łatwe mockowanie, a nawet pisanie testowych implementacji, w których zmieniona jest tylko jedna metoda biznesowej klasy.

Zawsze kiedy czytam taki post, wyobrażam sobie że jego autor wygląda jak golum, broniący za wszelką cenę swojego pierścienia, bo właśnie tak obrazuję sobie programistów którzy utrzymują że "ich" klasy i kod trzeba jakoś bronić przed innymi - że przecież trzeba nakładać finale i levele dostępu właśnie po to żeby inni programiści w zespole nie zepsuli przecież Twojego kodu.

Ja się z tym zupełnie nie zgadzam, i utrzymuję że jak dopisujesz coś do projektu to to jest tylko Twoja kontrybucja, i ten kod należy do wszystkich. Więc różnego rodzaju "zabezpieczenia" przed innymi programistami są niepotrzebne.

1
scibi92 napisał(a):

Ale pola klas muszą byc final, i klasy tez powinny być final. Klasy z powodu dokumentowania tego czy są do dziedziczenia czy nie, a pola żeby zmiejszyc (wyeliminować?) mutowalność i możliwośc wywalenia czegoś przypadkiem.

Ale przecież final przy polu albo zmiennej lokalnej ma zupłnie inne znaczenie od tego przy klasie i metodzie.

Moim zdaniem nakładanie final na klasę/metodę jest nawet nie tyle że niepotrzebne, ale wręcz szkodliwe. Jak to jest Twój kawałek kodu, to jeszcze pół biedy bo sobie zabierzesz to final, zmienisz dziedziczenie na kompozycję albo na polimorfizm. Ale jak to jest w jakiejś bibliotece w której kodu nie możesz ruszyć, a potrzebujesz zmienić implementację too jesteś w czarnej d***, bo już nie da się nic wtedy zrobić.

A żeby wyeliminiować mutowalność, wcale nie potrzebujesz final. Po pierwsze dlatego że nawet jak zrobisz final Super super to i tak możesz zrobić super.setValue(2); (czyli final to "zapewnia" tylko na jeden poziom w dół), a po drugie, nawet jak nie masz final w klasie to i tak możesz tak napisać tą klasę żeby jej interfejs w żaden sposób nie pozwalał na zmianę tej wartości i ta dam - wyeliminowano mutowalność.

1
TomRiddle napisał(a):
somekind napisał(a):

Bo jak ja kiedyś napisałem, że autor klasy powinien przemyśleć, co pozwolić dziedziczyć, a czego nie, i że pozwolanie na nadpisywanie wszystkich metod w klasach potomnych jest złym pomysłem, to zostałem zakrzyczany przez tutejszego eksperta, że mam alergię na metody wirtualne, i że JVM ma tak wspaniale rozwiązaną dewirtualizację, że nie trzeba w ogóle się tym przejmować. A poza tym pozostawienie metod otwartymi na nadpisywanie pozwala na łatwe mockowanie, a nawet pisanie testowych implementacji, w których zmieniona jest tylko jedna metoda biznesowej klasy.

Zawsze kiedy czytam taki post, wyobrażam sobie że jego autor wygląda jak golum, broniący za wszelką cenę swojego pierścienia, bo właśnie tak obrazuję sobie programistów którzy utrzymują że "ich" klasy i kod trzeba jakoś bronić przed innymi - że przecież trzeba nakładać finale i levele dostępu właśnie po to żeby inni programiści w zespole nie zepsuli przecież Twojego kodu.

Ja się z tym zupełnie nie zgadzam, i utrzymuję że jak dopisujesz coś do projektu to to jest tylko Twoja kontrybucja, i ten kod należy do wszystkich. Więc różnego rodzaju "zabezpieczenia" przed innymi programistami są niepotrzebne.

Nie, celem tego jest unikanie robienia syfu. Niestety często spotykam się z nadmiarem stosowania dziedziczenia, po zatym to słowo final oznacza że ktoś nie ręczy za to że klasa nadaje się do dziedziczenia. Równie dobrze można byłoby sie spytać bo co pola prywatne...

0

Mam wrażenie że Twoja wypowiedź jest pusta.

scibi92 napisał(a):

Niestety często spotykam się z nadmiarem stosowania dziedziczenia,

Więc stwierdziłeś że najlepszą metodą przeciwko temu jest dopisywnie final do klas? Razem ze wszystkimi efektami ubocznymi które to za sobą niesie?

scibi92 napisał(a):

po zatym to słowo final oznacza że ktoś nie ręczy za to że klasa nadaje się do dziedziczenia.

Co to znaczy że nie nadaje się do dziedziczenia?

Przecież ten problem w ogóle nie istnieje. Ktoś jej użyje w złym miejscu - albo jego testy nie przejdą albo jego feature nie będzie działał. Super rozwiązeniem na to miałoby być dopisywanie ślepo wszędzie słówka final? Dziwne że programiści z całego świata nie uważają tego za obowiązkowe, skoro takim prostym sposobem można by się uchronić przed bugami?

scibi92 napisał(a):

Równie dobrze można byłoby sie spytać bo co pola prywatne...

To coś innego. Modyfikatory dostępu są po to żeby klienci klasy korzystali z niej zgodnie z interfejsem i wiedzieli jak najmniej o implementacji. Poza tym jak sobie zdziedziczysz z klasy to bazowa ma swoje prywatne, a dziecko ma swoje prywatne i żadna nie wie nic o prywatnych drugiej.

1
TomRiddle napisał(a):

Zawsze kiedy czytam taki post, wyobrażam sobie że jego autor wygląda jak golum, broniący za wszelką cenę swojego pierścienia, bo właśnie tak obrazuję sobie programistów którzy utrzymują że "ich" klasy i kod trzeba jakoś bronić przed innymi - że przecież trzeba nakładać finale i levele dostępu właśnie po to żeby inni programiści w zespole nie zepsuli przecież Twojego kodu.

Napisz chociaż, że nie jesteś programistą... Bo to, że w pieczętowaniu klas/metod i używaniu modyfikatorów dostępu chodzi o jakąś obronę autorstwa kodu to absurd, na który mógł wpaść chyba tylko jakiś nawiedzony PM, który nawciągał się spaghetti.

Ja się z tym zupełnie nie zgadzam, i utrzymuję że jak dopisujesz coś do projektu to to jest tylko Twoja kontrybucja, i ten kod należy do wszystkich. Więc różnego rodzaju "zabezpieczenia" przed innymi programistami są niepotrzebne.

No właśnie - to jest moja kontrybucja, a nie moje odchody. Klasy trzeba pisać tak, żeby ktoś mógł sobie ich w razie potrzeby użyć w innym miejscu. Wygodnie, bezpiecznie i intuicyjnie, bez zastanawiania się, co jest w środku i nawet bez zaglądania do ich kodu.
A jeśli ktoś potrzebuje tylko pewną cześć klasy, a część chce napisać po swojemu, to niech zamiast nadpisywać część metod zrobi to od razu porządnie - wydzieli wspólny kod do klasy bazowej, starą klasę zrobi jej potomkiem, a swoją nową uczyni kolejnym. Wtedy struktura jest jasna i nie ma żadnych niespodzianek.

Pozwalanie na nadpisywanie dowolnych metod to prosta droga do chaosu. Zwłaszcza, że dzięki temu mogą powstawać partial mocki, które prowadzą do tego, że testy teoretycznie mające sprawdzać kod produkcyjny de facto nie są uruchamiane na nim tylko na jakichś nadpisanych metodach.

1

Pozwalanie na nadpisywanie dowolnych metod to prosta droga do chaosu. Zwłaszcza, że dzięki temu mogą powstawać partial mocki, które prowadzą do tego, że testy teoretycznie mające sprawdzać kod produkcyjny de facto nie są uruchamiane na nim tylko na jakichś nadpisanych metodach.

Jeśli partial mocki to zło to pełne mocki to zło do kwadratu. W sumie bym się z tym zgodził, ale jest jeden szczegół - jeśli w kodzie produkcyjnym jedna metoda w klasie np wysyła mejla to nie chcę w testach faktycznie wysyłać mejla. W takim przypadku nadpiszę sobie tę metodę do wysyłania mejla metodą, która np zapisuje mejla w zmiennej i dalej w teście sprawdzę jej zawartość. Alternatywą może być opakowanie problematycznej metody w klasę, wydzielenie interfejsu, zrobienie implementacji testowej, wstrzykiwanie tej nowej klasy do klasy wykorzystującej itd czyli zrobienie z tego niepotrzebnej ceremonii. Być może jest jakiś elegancki, zwięzły i niebudzący kontrowersji (u niektórych) sposób bez niepotrzebnej ceremonii, ale na razie na taki nie wpadłem. Widziałem przerzucanie lambd - są zwięzłe, ale dalej mniej zwięzłe niż nadpisywanie metod a także gorzej się nawiguje po wstrzykiwanych lambdach niż po zwykłych metodach.

5
Wibowit napisał(a):

Jeśli partial mocki to zło to pełne mocki to zło do kwadratu.

Jak dla mnie, to jest zupełnie odwrotnie. Pełny mock z daleka mówi - jestem mockiem, tylko udaję, popraw kod i skasuj mnie. Partial mock jest ukryty, i wykrywa się go często dopiero, gdy unit testy przechodzą, a produkcja się sypie. Wtedy nagle okazuje się, że testy zamiast faktycznie coś przetestować zwracają zahardcodowane wyniki z czapy.

W sumie bym się z tym zgodził, ale jest jeden szczegół - jeśli w kodzie produkcyjnym jedna metoda w klasie np wysyła mejla to nie chcę w testach faktycznie wysyłać mejla. W takim przypadku nadpiszę sobie tę metodę do wysyłania mejla metodą, która np zapisuje mejla w zmiennej i dalej w teście sprawdzę jej zawartość. Alternatywą może być opakowanie problematycznej metody w klasę, wydzielenie interfejsu, zrobienie implementacji testowej, wstrzykiwanie tej nowej klasy do klasy wykorzystującej itd czyli zrobienie z tego niepotrzebnej ceremonii.

Jeśli w klasie biznesowej masz metodę wysyłającą maile, to łamiesz SRP. Wysyłanie maili to standardowy przykład czegoś, co warto wydzielić do osobnego utila, a w wystarczająco dużym systemie nawet do odrębnego mikroserwisu.

1
somekind napisał(a):

Jak dla mnie, to jest zupełnie odwrotnie. Pełny mock z daleka mówi - jestem mockiem, tylko udaję, popraw kod i skasuj mnie. Partial mock jest ukryty, i wykrywa się go często dopiero, gdy unit testy przechodzą, a produkcja się sypie. Wtedy nagle okazuje się, że testy zamiast faktycznie coś przetestować zwracają zahardcodowane wyniki z czapy.

Zgadzam się.

Zwłaszcza że jeśli chcesz zrobić partial mocka (część kodu jest testowalna, część do mockowania) to czemu nie miałbyś zaprojektować lepszej struktury - wydzielić z niej to co jest do mockowania, a testowalną część po prostu unit przetestować?

1

To jest właśnie to o czym pisałem - zamiast nadpisywać metodę mam ceremonię z tworzeniem hierarchii klas. @somekind idzie nawet jeszcze dalej i sugeruje zrobienie mikroserwisu. Strzelanie z armaty do muchy.

Partial mock jest ukryty, i wykrywa się go często dopiero, gdy unit testy przechodzą, a produkcja się sypie. Wtedy nagle okazuje się, że testy zamiast faktycznie coś przetestować zwracają zahardcodowane wyniki z czapy.

Mejla i tak nie wyślę, więc muszę mieć jakąś testową implementację, która zamiast wysyłania mejli np wrzuca je do kolekcji. Potem w teście sprawdzam zawartość tejże kolekcji i gitara. Zastąpienie tego hierarchią/ kompozycja/ etc klas nic mi nie da oprócz zwiększenia objętości kodu.

Nie nadziałem się jeszcze na żadne problemy wynikające z mojego podejścia, natomiast zahardkodowane wartości z czapy to w zasadzie esencja mockowania (którego w dużej mierze unikam).

0
Wibowit napisał(a):

To jest właśnie to o czym pisałem - zamiast nadpisywać metodę mam ceremonię z tworzeniem hierarchii klas.

W sensie nadpisujesz metodę nie tworząc klasy? Ciekawe

Nawet jeśli nie ma żadnych testów, to i tak należy stosować SRP. Czego najwyraźniej nie robisz, bo wolisz tworzyć fejkowe hierarchie. Ty - nie ja, bo ja nic o żadnych hierarchiach nie pisałem.

@somekind idzie nawet jeszcze dalej i sugeruje zrobienie mikroserwisu. Strzelanie z armaty do muchy.

Tak, w wystarczająco dużym systemie. Np. przy 30 modułach, z których każdy wysyła 50 rodzajów maili i wszystkiego razem wychodzi ćwierć miliona dziennie. Można wtedy elastyczniej skalować niż mając submoduł wysyłania maili w każdym module.

0

Nawet jeśli nie ma żadnych testów, to i tak należy stosować SRP.

SRP jest OK, ale często ciężko sensownie określić co jest jedną odpowiedzialnością. SRP traktowałbym raczej jako pomocną zasadę przy refaktoryzacji, podziale rozrastającej się klasy na mniejsze części, niż jako żelazną regułę stosowaną nawet jak klasy są mikroskopijne i dzielenie ich dalej jest nieopłacalne. Nadgorliwość gorsza od faszyzmu.

Podam przykład by lepiej zobrazować co miałem na myśli pisząc o nadpisywaniu metod i unikaniu dodatkowych hierarchii klas (obojętnie czy hierarchia kompozycji czy dziedziczenia).

Klasa produkcyjna (są 3 typy mejli, podobnie jak np w pewnym "dużym mikroserwisie" u nas w pracy i tego się trzymajmy):

import org.apache.commons.mail.SimpleEmail

object SecretIds {
  def hash(data: String, secret: String): Int =
    (secret + data).hashCode // TODO replace with sth stable
}

class AppMailer(smtpHostName: String,
                smtpPort: Int,
                smtpUsername: String,
                smtpPassword: String,
                appBaseUrl: String,
                secret: String,
                feedbackEmail: String) {
  def sendPasswordValidationEmail(userId: Long, userEmail: String): Unit = {
    val checksum = SecretIds.hash(userEmail, secret)
    sendEmail("my-app@no-reply",
              userEmail,
              "Password validation",
              s"To validate password go to: " +
                s"$appBaseUrl/password/validate/$userId/$checksum")
  }

  def sendPasswordRecoveryEmail(userId: Long, userEmail: String): Unit = {
    val checksum = SecretIds.hash(userEmail, secret)
    sendEmail("my-app@no-reply",
              userEmail,
              "Password recovery",
              s"To recover password go to: " +
                s"$appBaseUrl/password/validate/$userId/$checksum")
  }

  def sendFeedbackEmail(userEmail: String, feedback: String): Unit = {
    sendEmail("my-app@no-reply",
              feedbackEmail,
              s"Feedback from user $userEmail",
              s"Feedback: \n$feedback")
  }

  protected def sendEmail(fromAddress: String,
                          toAddress: String,
                          subject: String,
                          content: String): Unit = {
    val email = new SimpleEmail()
    email.setHostName(smtpHostName)
    email.setSmtpPort(smtpPort)
    email.setAuthentication(smtpUsername, smtpPassword)
    email.setFrom(fromAddress)
    email.addTo(toAddress)
    email.setSubject(subject)
    email.setMsg(content)
    email.send()
  }
}

Klasa testowa:

import org.scalatest.{FlatSpec, MustMatchers}

class AppMailerSpec extends FlatSpec with MustMatchers {
  behavior of "AppMailer"

  it must "send password validation email" in {
    val mailer = new TestMailer
    val eAddress = "[email protected]"
    mailer.sendPasswordValidationEmail(34, eAddress)
    val secretId = SecretIds.hash(eAddress, "secret")
    mailer.lastEmails mustBe Seq(
      Email("my-app@no-reply",
            eAddress,
            "Password validation",
            "To validate password go to: " +
              s"base URL/password/validate/34/$secretId"))
  }

  it must "send password recovery email" in {
    val mailer = new TestMailer
    val eAddress = "[email protected]"
    mailer.sendPasswordRecoveryEmail(78, eAddress)
    val secretId = SecretIds.hash(eAddress, "secret")
    mailer.lastEmails mustBe Seq(
      Email("my-app@no-reply",
            eAddress,
            "Password recovery",
            "To recover password go to: " +
              s"base URL/password/validate/78/$secretId"))
  }

  it must "send feedback" in {
    val mailer = new TestMailer
    val eAddress = "[email protected]"
    mailer.sendFeedbackEmail(eAddress, "I am happy")
    mailer.lastEmails mustBe Seq(
      Email("my-app@no-reply",
            "[email protected]",
            s"Feedback from user $eAddress",
            "Feedback: \nI am happy"))
  }

  case class Email(fromAddress: String,
                   toAddress: String,
                   subject: String,
                   content: String)

  class TestMailer
      extends AppMailer(null,
                        0,
                        null,
                        null,
                        "base URL",
                        "secret",
                        "[email protected]") {
    private[AppMailerSpec] var lastEmails = Vector.empty[Email]

    override protected def sendEmail(fromAddress: String,
                                     toAddress: String,
                                     subject: String,
                                     content: String): Unit = {
      lastEmails :+= Email(fromAddress, toAddress, subject, content)
    }
  }
}

Jak widać z powyższego kodu:

  • w kodzie produkcyjnym mamy jedną klasę - AppMailer, więc nie ma mowy o żadnej hierarchii
  • podklasa jest w testach, ale jest to podklasa lokalna dla speca
  • w tej testowej podklasie nadpisuję nieabstrakcyjną metodę z efektami ubocznymi - podmieniam jedne efekty uboczne (ciężko testowalne) na inne (łatwo testowalne)
  • logika produkcyjna jest trywialna, a logika testowa nie jest wcale kiepska

Można cudować i np dzielić klasę produkcyjną na mniejsze klasy, ale nie widzę w tym przypadku żadnego zysku z tego.

0
Wibowit napisał(a):

Nawet jeśli nie ma żadnych testów, to i tak należy stosować SRP.

SRP jest OK, ale często ciężko sensownie określić co jest jedną odpowiedzialnością. SRP traktowałbym raczej jako pomocną zasadę przy refaktoryzacji, podziale rozrastającej się klasy na mniejsze części, niż jako żelazną regułę stosowaną nawet jak klasy są mikroskopijne i dzielenie ich dalej jest nieopłacalne. Nadgorliwość gorsza od faszyzmu.

Oczywistym jest że lepsze 20 klas na 200 linijek, niż 200 klas na 20 linijek. Zgadzam się również że czasem ciężko rozpoznać odpowiedzialność klasy, i że wydzielanie kodu na siłę tylko pro forma, dla zasady często może okazać się nie korzystne.

Niemniej nie zgadzam się, żeby w Twoim przypadku wydzielenie ciężko testowalnej logiki do innej klasy celem jej zamockowania miałoby spełniać to kryterium. Moim zdaniem takie wydzielenie to jak najbardziej sensowny zabieg.

1

Moim zdaniem natomiast zysk z podziału na klasy jest czysto teoretyczny, a strata z powodu zwiększonej ilości kodu produkcyjnego jest odczuwalna. W ogólności bardzo nie lubię mnożenia klas produkcyjnych ponad potrzebę, bo im więcej kodu tym dłużej trzeba się wdrażać w kod. Sama nawigacja po kodzie też staje się dłuższa (a co za tym idzie łatwiej zgubić kontekst podczas nawigacji po kodzie).

Takich klas jak ta przykładowa co podałem zdarza się dość sporo. Takich czyli generalnie łatwo testowalnych oprócz jednej metody mającej efekty uboczne, których chcemy uniknąć w testach i zastąpić je takimi łatwo testowalnymi.

Alternatywą do tego co podałem powyżej (przy założeniu że bardzo nie chcemy nadpisywać nieabstrakcyjnych metod) może być seria następujących kroków:

  • wydzielenie metody sendEmail do osobnej klasy, np EmailSenderImpl (co za brzydka nazwa)
  • wydzielenie interfejsu z tej klasy - EmailSender
  • w testach zaimplementowanie StubEmailSender implementujacego EmailSender
  • dodatkowo trzeba wstrzyknąć obiekt typu EmailSender do klasy AppMailer co samo w sobie jest dodatkową ceremonią i komplikuje nieco kod produkcyjny (a moim priorytetem jest prostota i czytelność kodu produkcyjnego)

Jak widać, w tym alternatywnym sposobie mamy w kodzie produkcyjnym dodatkowy interfejs, dodatkową klasę i dodatkowe wstrzykiwanie. Tylko po to, by ktoś poczuł się lepiej.

2
TomRiddle napisał(a):

Oczywistym jest że lepsze 20 klas na 200 linijek, niż 200 klas na 20 linijek.

Na mojej planecie nie jest to w żaden sposób oczywiste.
A gdzie jest granica? Czy 2 klasy na 2000 linijek są jeszcze lepsze?

0
Wibowit napisał(a):

SRP jest OK, ale często ciężko sensownie określić co jest jedną odpowiedzialnością. SRP traktowałbym raczej jako pomocną zasadę przy refaktoryzacji, podziale rozrastającej się klasy na mniejsze części, niż jako żelazną regułę stosowaną nawet jak klasy są mikroskopijne i dzielenie ich dalej jest nieopłacalne. Nadgorliwość gorsza od faszyzmu.

SRP to świnia. Bo nie jest precyzyjna, albo inaczej - tam gdzie jedni już nie widzą sensu podziału inni zobaczą 5 powodów. I nie wiem gdzie jest granica.
Hipoteza: jak w klasie są 3 komponenty (pola, metody) to pewnie znajdzie się odopowiednie uzasadznienie na rozdzielenie względem SRP.

Tym niemniej, jeśli chodzi o kod, który podałeś to jest to wręcz książkowy przykład na złamanie RSP (wysyłanie i składanie treści maila - normalnie jak z któregoś przykładu Uncle Boba).

Żeby zmienić sposób testowania wcale nie trzeba tony bytów wprowadzać.

Prawie ten sam kod, a łatwiej testowalny, wydzielona funkcja.

case class Email ( fromAddress: String,
                   toAddress: String,
                   subject: String,
                   content: String)


class AppMailer(
                 appBaseUrl: String,
                 secret: String,
                 feedbackEmail: String,
                 mailSender : Email => Unit) {
  def this ( appBaseUrl: String,
             secret: String,
             feedbackEmail: String,
             smtpHostName: String,
             smtpPort: Int,
             smtpUsername: String,
             smtpPassword: String) = this (appBaseUrl, secret, feedbackEmail,
    (e:Email) => {
      val email = new SimpleEmail()
      email.setHostName(smtpHostName)
      email.setSmtpPort(smtpPort)
      email.setAuthentication(smtpUsername, smtpPassword)
      email.setFrom(e.fromAddress)
      email.addTo(e.toAddress)
      email.setSubject(e.subject)
      email.setMsg(e.content)
      email.send()
    })

  def sendPasswordValidationEmail(userId: Long, userEmail: String): Unit = {
    val checksum = SecretIds.hash(userEmail, secret)
    mailSender (Email("my-app@no-reply",
      userEmail,
      "Password validation",
      s"To validate password go to: " +
        s"$appBaseUrl/password/validate/$userId/$checksum"))
  }

  def sendPasswordRecoveryEmail(userId: Long, userEmail: String): Unit = {
    val checksum = SecretIds.hash(userEmail, secret)
    mailSender (Email("my-app@no-reply",
      userEmail,
      "Password recovery",
      s"To recover password go to: " +
        s"$appBaseUrl/password/validate/$userId/$checksum"))
  }

  def sendFeedbackEmail(userEmail: String, feedback: String): Unit = {
    mailSender (Email("my-app@no-reply",
      feedbackEmail,
      s"Feedback from user $userEmail",
      s"Feedback: \n$feedback"))
  }

}

Wprowadzona klasa **Email **nie ma nic wspólnego z problemem, bolały mnie 4 argumenty typu String do metody,
mogła też być krotka, lub funkcja od 4 argumentów i byłoby bez dodatkowej klasy zupełnie.

2

Jak widzę główna zmiana to przeniesienie logiki z metody do funkcji i zrobienie dodatkowego konstruktora (dodatkowe case classy można pominąć, wiem że w moim kodzie kilka powinno dojść, ale to nie jest meritum dyskusji tutaj). Niewiele to się różni od zmiany defa na vala, więc sądzę iż docelowym przekształceniem byłoby wyniesienie sendMail poza klasę AppMailer. Dodatkowy konstruktor jest po to, by ułatwić testowanie (a w zasadzie ułatwić unikanie nadpisywania metod, bo w moim przykładzie testowanie wcale nie jest trudniejsze), a więc widać, że priorytetem nie jest zwięzłość kodu produkcyjnego.

Abstrahując od szczegółów, jak widzę takie coś jak zaproponowałeś powyżej to od razu zamieniam lambdę na metodę i nadpisywanie metody w testach. Powód jest bardzo praktyczny - IDE świetnie sobie radzą z nawigacją po metodach, bezproblemowo przenoszą mnie do implementacji metody produkcyjnej jak i testowej. W przypadku lambdy stworzonej gdzieś daleko nawigacja jest naprawdę kiepska. Jeśli jestem w miejscu użycia mailSender to by sprawdzić co się dzieje w środku najpierw ląduję w głównym konstruktorze, a potem muszę szukać gdzie został on wywołany i z jaką lambdą w środku. Takie coś mnie rozprasza i denerwuje, a co za tym idzie utrudnia zrozumienie kodu. Ponadto użyłeś standardowego typu funkcji czyli FunctionN, a te nie mają treściwych nazw parametrów (tylko jakieś _1, _2, itd). Zastąpiłeś je więc dodatkową klasą Email, by odzyskać podpowiedzi od IDE. To też element ceremonii unikania metod do nadpisywania.

Dla mnie lambda nadaje się do wywalenia (tzn zamianę na zwykłe metody) jeśli utrudnia eksplorację kodu. Zastanawiam się jak sformalizować kategorie lambd, ale na razie mam tylko częściowe pomysły. Dobra lambda to zwykle taka, która ma maksymalnie ogólny typ, np A => B, A => Boolean czy A => C[B] jak w kolekcjach. Kiepska lambda ma często szczegółowe typy, czyli np Email => Unit, Report => Report czy nawet String => Int. Wynika to z tego, że szczegółowy typ wskazuje na silne powiązanie ciała lambdy i logiki wywołującej lambdę. Skoro kawałki logiki są silnie powiązane to powinny dać się szybko naraz przejrzeć, a więc leżeć blisko siebie bądź IDE powinno dać możliwość szybkiego przeskoczenia z jednego kawałka do drugiego (czyli jeśli leżą blisko siebie to taka lambda może być OK). Bardzo ogólne typy rodzaju A => B wskazują natomiast na używanie narzędzia ogólnego przeznaczenia typu kolekcja, strumień, parser czy innego typu monada. Tego typu narzędzi jest mało i można się ich uczyć niezależnie od eksploracji projektu nad którym pracujemy.

Awersję do nadmiarowego lambdowania mam po eksperymentach kolegi z zespołu (już nie pracuje u nas), który pewnego czasu trenował sobie lambdy, currying itd na żywym organizmie (kodzie produkcyjnym). Efektem był trudny do prześledzenia kod. W jednym przypadku zamieniłem co się dało na zwykłe metody, zmniejszyłem ilość lambd, trochę metod przeniosłem gdzie indziej do hierarchii klas i kod stał się czytelniejszy, nie tylko dla mnie, ale także dla innych kolegów z zespołu. Nadal jednak zostało sporo miejsc, gdzie lambdy straszą (w innych serwisach).

1 użytkowników online, w tym zalogowanych: 0, gości: 1