Metoda przydatna dla kilku klas - gdzie ją umieścić?

Ostatnio po raz kolejny spotkałem w swoim kodzie taką sytuacje więc postanowiłem o to zapytać.

Juz któryś raz mam potrzebę stworzenia metody**¹**, która docelowo została stworzona na potrzebe jakiejś klasy, jako metoda prywatna, taki "helper". Później okazało się, że w innej klasie też by się przydała. Jednak klasy te są mało albo w ogóle ze sobą nie powiązane.

Duplikować te metode i mieć w 2 klasach osobno - lipa raczej.
I dalej - zmienić te metode na public (static?) i używać z tej drugiej klasy to troche słabe rozwiązanie (tak mi sie wydaje). Możnaby to wyjąc i spakować w jakąś klase np SomethingUtils i albo SomethingService (w wypadku springa) - tylko znowu czy jest sens wrzucać jedną metode na klase?
Jest szansa, że później znowu będzie potrzeba stworzenia podobnej metody (która chce użyć w paru, niezależnych klasach). Ale znowu może przeznaczeniem wcale nie pasować do tej klasy Utilsowej już stworzonej i trzeba będzie robić następną.

Jakbyście do tego podeszli?

**¹**metoda to jakiś mały helper który np ze stringa cos usuwa; coś, co brzydko pakować w kod który piszemy i stwierdzamy ze lepiej jest zrobić krótką metode, dla czytelności choćby

Tak, jest sens robić serwis z jedną metodą i wstrzykiwać tam gdzie jest potrzebny.

@Shalom nawet jeśli to jest metoda w stylu ze stringa usuń jakieś znaki itd. - czyli mówiąc krócej krótka, nieskomplikowana?

Tym lepiej, łatwiej się to będzie testować ;)

@azalut no tak takie utilsy. Nie ma w tym nic błędnego może nawet z czasem rozrosną Ci się te klasy.

zresztą looknij sobie np. na apache stringutils może tam już są te metody, które niepotrzebnie sam piszesz ;]

Nawet w takim wypadku?: :)

 
public String deleteQuotes(String str) {
        return str.replaceAll("\"", "");
}

(może się wydawać zbędna, ale hmm.. załóżmy, że ma być użyta pare razy podczas parsowania czegoś itd. - upakowanie w metode znacznie zwiększa czytelność)

A co sądzicie o takim zabiegu, że mamy np jakąś klase User i ona ma jakąś metode która coś robi :) i potrzebna jest nam metoda taka supportowa, ale tylko i wyłącznie dla tego Usera. Też lepiej jest to delegować do serwisu czy po prostu zrobić małą prywatną metode i z głowy?

azalut napisał(a):

(może się wydawać zbędna, ale hmm.. załóżmy, że ma być użyta pare razy podczas parsowania czegoś itd. - upakowanie w metode znacznie zwiększa czytelność)

nie. str = str.replaceAll("\",""); jest wystarczająco czytelne

odnośnie drugiego fajnie byłoby jakbyś podał jakiś przykład (np. kod).

azalut napisał(a):

Nawet w takim wypadku?: :)

 
public String deleteQuotes(String str) {
        return str.replaceAll("\"", "");
}

tak, gdy kod jest uzywany wielokrotnie, powinienes go wydzielic zamiast robic copy-paste. w wypadku np potrzeby dodania glupiego ifa, obslugi single quote lub gdybys wynalazl lepszy sposob na wywalanie znakow, zmiana bedzie dotyczyla jednego miejsca.

karolinaa napisał(a):

nie. str = str.replaceAll("\",""); jest wystarczająco czytelne

Czytelne to to moze jest, ale jesli jest powtarzane to bym i tak wyciagnal gdzies na zewnatrz. Zawsze moze takie 'oczyszczanie' moze sie zmienic i wtedy tylko jedno miejsce nalezy poprawic.

@azalut ja myśle że w ogóle trochę źle do tego podchodzisz. Bo dla ciebie powodem do rozmyślania nad wydzielaniem do osobnej klasy było używanie tego samego kodu w innym miejscu, a w takiej sytuacji to w ogóle nie powinno być nawet rozmyśleń, to powinno być oczywiste. O tym czy wydzielać powinieneś pomyśleć znacznie wcześniej -> kiedy masz w danej klasie metodę która robi "coś innego niż ta klasa". Naewt jeśli to "coś innego" jest używane tylko w tej jednej klasie!
Na przykład piszesz klasę do nawiązywania połączeń HTTP i masz tam metodę walidującą URLa. Ta metoda jest używana tylko w tej twojej jednej klasie, niemniej należałoby ją wydzielić do osobnej klasy.

no ja też jakieś prepareToParse, SEPARATOR lub coś w tym stylu bym wydzieliła. samo deleteQuotes, bo się powtarza - nie.

@Shalom: z tym bym az tak nie przesadzal, bo skonczy sie z z dziesiecioma klasami po jednej metodzie kazda, i 10 zaleznosciami wstrzykiwanymi. Robilbys to setterami (nie da sie miec pol final), konstruktor-injection (10 parametrow?) czy field-injection (ciezko testowac, nie mozna zrobic final), czy nie uzywal wstrzykiwania?
Tak, wiem ze w ogolnym przypadku to jest 'dobre' i poprawne, ale jak mowie, nie przesadzalbym. Jesli dana metoda jest uzywana tylko w jednej klasie, taki helper, to pierwsze co bym robil to wlasnie zrobil private w tej samej klasie. Mozem glupi, mozem niedoswiadczony, mozem slaby programista.

To zależy co to za metoda ;) Jeśli to jest faktycznie taki jednolinijkowiec to nie ma co przesadzać. Ale jeśli to jest kilkadziesiąt linijek, jeszcze do tego podzielone na kilka mniejszych metod to zaczyna mieć sens wydzielenie tego. Dla mnie zwykle sygnałem jest problem z testowaniem. Jeśli chce napisać unit testy i nagle się okazuje że testowana metoda korzysta z dość skomplikowanej metody prywatnej i nie dość że muszę napisać kilka testów ze względu na moją metodę publiczną to jeszcze każdy z nich zależy od działania tej metody prywatnej. Co więcej często widać że warto byłoby przetestować tą prywatną metodę samodzielnie, a to bez magii Powermocka czy innych cudów może być trudne. Wtedy widać że warto zrobić ekstrakcje :)
Jeśli chodzi o wstrzykiwanie to preferuje field injection. Z testowaniem nie ma aż takich problemów. Easymock pozwala na przykład na zrobienie

@RunWith(EasyMockRunner.class)
class MojaKlasaTest{
    @TestSubject
    private final MojaKlasa objectUnderTest = new MojaKlasa();
    
    @Mock
    private Zaleznosc1 zaleznosc1;
    @Mock
    private Zaleznosc2 zaleznosc2;
}

I voila, mockowane zależności wstrzyknięte.

@katelx i @the real mucka - między innymi o to mi chodzi, że gdyby coś się zmieniło - zmiana będzie w jednym miejscu, a nie w bóg wie ilu

@karolinaa myślę jednak, że to coś w sobie ma tworzenie takiej metody. Może w tym wypadku to nie jest aż tak widoczne - ale jak pisałem pojawia się takich sytuacji pare i czasem to takie trywialne usuwanie cudzysłowiów (tak to sie odmienia?), jednak często to troszke większe operacja, ale nie urastająca do rozmiarów wartych klepania klas i pakietów

wiesz co.. przykład ciężko mi podać ale chodzi mi o sytuacje, w której np.. mam klase User i w nim mam jakąś metode - powiedzmy statyczne User.of(Data data) - czyli na podstawie jakiegos obiektu Data tworze usera. Może przydać się jakaś krótka metoda która coś sprawdza/wygładza/dostosowuje.

I tu wiem, że są dwa podejścia, widziałem kiedys prezentacje na ten temat na youtube chyba. Lepiej robić metody w modelu tzn np w klasie User zrobic metode eat(Meal meal) czy zrobic osobny UserService i w nim eat(User user, Meal meal)

Stąd pytam - co sądzicie o wrzucaniu metod które "coś robią" bezpośrednio w klasy tj User, Item i tym podobne

@Shalom wiem, że powinienem zaplanować wcześniej i to zrobiłem. Wiem, że klasa powinna mieć odpowiedzialność za jedną rzecz. Ale gdzieś wyczytałem, że jak mam zrobić taki "helper-method" na 2-3 linijki to lepiej zrobic prywatną metode gdzies u dołu klasy niż cudować z budowaniem nazw dla nowych klas, pisania wyjaśnień, tworzenia pakietu itd..

Że powinienem wyekstrahować to gdzieś to wiem - stąd moje pytanie. Bo troche głupio robić klase:

public class SomethingParsingUtils { // i static lub @Service
   public String deleteQuotes(String str) {
        return str.replaceAll("\"", "");
   }
}

Jest szansa, że się ona rozrośnie - ale jak duża, kto to wie? Może lepiej zostawić jedną private metodę i refactorować dopiero, gdy zajdzie potrzeba użycia tej metody w kilku innych miejscach. (tzn wydzielić do osobnej klasy)

Co do przykładu z HTTP - owszem, zrobiłbym jak piszesz. Ale chodzi mi tu o trywialne sprawy tj poprawa stringa, przekonwertowanie czegoś w jakiś specyficzny sposób z dziwnego stringa na inta/double i takie tam

Jeśli chodzi o testowanie - zgadzam się, że pewnie łatwiej byłoby przetestować metodę w jakiejś osobnej klasie niż w samym User. Tylko że tu znowu wracamy do pytania co według nas jest lepsze: nadawanie User'owi metod wykonujących na nim operacje(np eat Meal), czy robienie @Service któremu bedziemy podawali jak napisałem - Usera i Meal jaki ma zjeść.

ps
wybaczcie ze odpisuje tak pozno, ale byłem cały dzień na wyjezdzie

Jeżeli odpowiedzialnością klasy User jest jedzenie meala to niech go sobie ma i je. Ale wtedy niekoniecznie jego odpowiedzialnością będzie walidowanie danych osobowych na przykład ;)
Problem z takimi metodami jest taki że potem się nagle okazuje, szczególnie jak pracujesz w zespole, że masz 5 różnych wersji tego samego kodu, każda troszeczkę inna i marnujesz pół dnia na konsolidacje ;) Na to zrobiłbym jakies StringUtils jeśli faktycznie korzystasz w większej liczbie miejsc.

Ciekszy mnie że nie jesteś jednym z tych geniuszy którzy napotykając ten problem próbują wydzielić klasę bazową (!) z tą biedną metodą a potem wpychają zupełnie niezwiązane ze sobą klasy w hierarchię dziedziczenia żeby mieć do niej dostęp :D

Czyli finalnie: dobrze jest to przerzucić do klasy jakiejś Utilsowej jeśli używa tego więcej niż jedna klasa, a gdyby to było coś na prawde prostego, na użytek jednej klasy, można zrobić private.

Ciekszy mnie że nie jesteś jednym z tych geniuszy którzy(..)
Jak komuś będę chciał kiedyś zrobić na złość, to tak zrobie! a co tam!

azalut napisał(a):

Że powinienem wyekstrahować to gdzieś to wiem - stąd moje pytanie. Bo troche głupio robić klase:

public class SomethingParsingUtils { // i static lub @Service
   public String deleteQuotes(String str) {
        return str.replaceAll("\"", "");
   }
}

czemu nie wystarczy wydzielić public static final String CHAR_TO_REPLACE = "\"; czy czegoś w ten deseń?

to jest realny przykład - serio coś takiego Ci się powtarza? jak wygląda to parsowanie, że w kilku miejscach używasz tego replaceAll("",""); ? obstawiam, że masz coś dziwnie napisane.

azalut napisał(a):

I tu wiem, że są dwa podejścia, widziałem kiedys prezentacje na ten temat na youtube chyba. Lepiej robić metody w modelu tzn np w klasie User zrobic metode eat(Meal meal) czy zrobic osobny UserService i w nim eat(User user, Meal meal)

ja jestem za User::eat(Meal meal) , ale w praktyce zwykle jak mam jakiś model/dto i większe rzeczy to nie pozostaje mi nic innego
jak UserService::eat(User user, Meal meal) ;]

W jednym z projektów, nad którym pracowałem bywało, że tworzyły się takie Utils (POJO i @Inject) najpierw z jedną metodą. Wraz z rozwojem projektu było ich coraz więcej (np. Utilsy powiązania daty z domeną biznesową: taki projekt). Dość łatwo było to testować.

Ja jestem jednak zwolennikiem wstrzykiwania (@Inject), ponieważ dzieki temu błędy w metodzie utilsowej nie propagują się tak łatwo do obiektu, który tę metodę wykorzystuje (co mogłoby nastąpić w przypadku statica: utrata izolacji).

Static jest lepszy niż @Inject w przypadku dobrze wytestowanych bibliotek ogólnego przeznaczenia np. Apache Commons (jak StringUtils, CollectionUtils). Po prostu podczas testów zakładamy, że klasa z zewnętrznej biblioteki jest na pewno ok i jej nie mockuje (np. z użyciem PowerMocka).

Czasem w trywialnych, bardzo prostych przypadkach taka metoda pomocnicza to był zwykły private. Jeśli okazywało się, że będzie potrzebna we wielu miejscach to robiłem refactoring i tak powstawała klasa utilsowa (@Inject).

Jeżeli metoda pomocnicza była używana tylko w jednym miejscu zdarzało mi się i tak zakładać jedną klase dla niej, jeśli robiła coś skomplikowanego (np. związanego z domeną biznesową). Miało to na celu tylko i wyłącznie ułatwienie pisania testów. Abym mógł oddzielić działanie Utils, który robił coś skomplikowanego od klienta wykorzystującego ten Utils (@Inject).

trochę mnie to ciekawi.
taki przykład - aby przetestować musiałabym mockować plik. Powinnam zmienić te metody na publiczne, wydzielić do utilsów czy olać testowanie?

@Named
public class AccountTxtDaoImpl implements AccountDao {

    public static final String SEPARATOR = "\t";

    private File file;
    private Charset charset;

    public AccountTxtDaoImpl() {
        charset = Charset.defaultCharset();
    }

    @Override
    public List<Account> findAll() {
        List<Account> result = Lists.newArrayList();
        try (Stream<String> lines = Files.lines(file.toPath(), charset)) {
            result = lines.map(this::accountFromLine).collect(Collectors.toList());
        } catch (IOException e) {
            e.printStackTrace();
        }
        return result;
    }

    private Account accountFromLine(String line) {
        String[] fields = line.split(SEPARATOR);
        Account account = new Account();
        account.setName(fields[0]);
        account.setPayments(paymentsFromFields(fields));
        return account;
    }

    private Set<Payment> paymentsFromFields(String[] fields) {
        Set<Payment> payments = Sets.newHashSet();
        for (int i = 1; i < fields.length; ++i) {
            payments.add(new Payment(fields[i]));
        }
        return payments;
    }

    public void setFile(File file) {
        this.file = file;
    }
}

co robić z metodami private gdy zmiana na public narusza hermetyzacje i nie jest to w interesie api a żeby przetestować jadąc tylko na publicznych trzeba jakiś cudów na kiju?
testowanie ponad wszystko ;?

Ja testuje tylko publiczne metody. Bo prywatne moga sie zmienic w ramach refactoringu i to powinno byc calkowicie przezryczyste dla testow. Dzieki temu, ze masz pubiczny setter dla pliku mozesz wstrzyknac za pomoca settera mock do interfejsu File. Przypomina to mockowanie EntityManagera i jest to kontrowersyjne. Takie cos lepiej integracyjnie testowac jesli juz. To nie jest logika.

Jest jeszcze problem z tym kodem w obecnej postaci nie nadaje sie on do testowania:

Files.lines

Taki static wydaje sie byc nietestowalny i powoduje, ze unit test nie zadziala. Tu warto ewentualnie wydzielic go w service i mockowac (@Inject) albo uzyc PowerMocka. Ja wole wrapper (mnniej zaleznosci niz PowerMock).

Jakbym musial to robilbym to mniej wiecej tak. Co robie? Mowie jakie wejscie ma zwrocic mock file. Nastepnie mowie co ma zwrocic wrapper nietestowalnego kodu. I potem sprawdzam czy zwrocona kolekcja odpowiada wymaganiom. To taki chaotyczny pseudocode. Nietestowalne fragmenty zostaly wrzucone do service / wrapper.

public class AccountDaoTest {

   @Before
   public void init() throws Exception {
          MockitoAnnotations.initMocks(this);
   }

   // system under test
  @InjectMocks // field injection @Inject/@Autowired obiektow oznaczonych jako @Mock
  private AccountDao sut = new AccountDaoImpl();
  // wrappuje nietestowalny static
  @Mock
  private FileInputSrc inputSrv;

  @Test
  public void findAllShouldReturnValidResults() throws Exception {
    File filemock = mock(File.class);
    // lekkie naruszenie hermetyzacji, ale umozliwia wstrzykniecie mocka pliku w przypadku testu
    sut.setFile(filemock);
    // mock na pliku: trzeba zasymulowac dane wejsciowe
    // z pliku: tu moze byc kilka metod
    when(filemock.mojaMetoda()).thenReturn(expectedInput);
    (...)
    // test
    wyjscie = sut.findAll();
    // sprawdzenie czy zwrocone dane sa spoko
    // czy kolekcja List<Account> zostala dobrze wygnerowana
   (...)
   }
}

Tylko findAll wyglada na metoda, ktora potencjalnie warto wytestowac. Ale badzmy szczerzy: plik to tylko taka baza danych. W tym przypadku testowanie tego ma podobny sens co testowanie DAO (czyli niewielki). Lepiej testowac logike, ktora przetwarza dane pobrane z bazy.

Łatwo popłnić mockery anti-pattern:
http://stackoverflow.com/questions/333682/unit-testing-anti-patterns-catalogue/333686#333686