Bardzo krótkie metody

Ciekawi mnie co doświadczony programista sądzi o takich metodach?
Dobry kod powinien być czytelny i zrozumiały po samych nazwach klas/metod/zmiennych, ale czy powinno się praktykować 1-liniowe metody?

Dla przykładu:

public static boolean fileExist(String fileName) {
        return new File(fileName).isFile();
    }

    public static boolean isEven(int number) {
        return number % 2 == 0;
    }

    public static float averageLineLength(int charsSum, int linesSum) {
        return charsSum / linesSum;
    }

Ja uważam że nie ma co przeginać, bo:

1. Metody powinny być krótkie, ale w tym przypadku możesz zataić co masz na myśli, każdy myśli że isEven zwróci liczbę parzystą ale jak coś skopiesz w tej metodę to wprowadzisz wiele osób w błąd.
2. Nie powiem żeby to było dużo czytelniejsze (w przypadku dwóch ostatnich metod)
3. Metody statyczne to w cale nie jakieś mistrzostwo w programowaniu obiektowym.
4. Wydajność.... niby nic wielkiego - jedna linijka, ale jednak trzeba odłożyć skok do metody i obsługę wyników na stosie/stercie.

Dobre są 4-5 linijkowe metody, te nico dłuższe też, ale jednolinijkowece?

pytanie czy naprawdę potrzeba tutaj osobnej metody, czy nie lepiej po prostu napisać number % 2 == 0 itp. niż komplikować i używać metody.

Myślę, że jak zwykle odpowiedzią jest "to zależy".

w przypadku isEven zapewne zysk będzie zerowy, ale już np. zenkapsulowanie sprawdzania czy plik istnieje w postaci metody fileExist może mieć sens, jeśli będziesz chciał pobawić się potem w polimorfizm i np. obsługę różnych systemów plików. Ale to sam musisz sie za każdym razem zastanowić co ma większy sens w danym przypadku.

W przypadku averageLineLength to szczerze powiedziawszy coś tu jest nie halo. Taka funkcja chyba powinna przyjmować jakiegoś rodzaju kolekcję stringów a nie dwa inty? Mnie się wydaje, że zły poziom abstrakcji tu przyjąłeś.

Zależy od długości i stopnia wykorzystania. Np 1-linijkowe metody używane w jednym miejscu zwykle nie miałyby sensu - takie metody można zastąpić po prostu zmiennymi. Jednak w wielu przypadkach jednolinijkowe metody mają sens, bo opisują co robi dany kod.

Taki kod nie ma sensu, bo nazwa metody nie wnosi nic co jest natychmiastowo widoczne w ciele metody:

void multiplyNumbers(int a, int b) {
  return a * b;
}

Ale taka metoda już coś wnosi:

boolean userCanEditItem(User user, Item item) {
  return user.isAdmin() || user.manageableItemTypes.contains(item.type);
}

bo zdecydowanie szybciej jestem w stanie zrozumieć to:

if (!userCanEditItem(user, item)) {
  throw new Error("illegal operation");
}

niż to:

if (!(user.isAdmin() || user.manageableItemTypes.contains(item.type))) {
  throw new Error("illegal operation");
}

Nie muszę się domyślać, że chodzi o prawa dostępu, bo wynika to wprost z nazwy metody.

Slepiec napisał(a):

Metody powinny być krótkie, ale w tym przypadku możesz zataić co masz na myśli, każdy myśli że isEven zwróci liczbę parzystą ale jak coś skopiesz w tej metodę to wprowadzisz wiele osób w błąd.

Jak skopiesz w innym miejscu, to też wprowadzisz w błąd.

Slepiec napisał(a):

Metody statyczne to w cale nie jakieś mistrzostwo w programowaniu obiektowym.

Do sprawdzenia podzielności liczby nie trzeba kontenera DI i fabryki strategii.

Slepiec napisał(a):

Wydajność.... niby nic wielkiego - jedna linijka, ale jednak trzeba odłożyć skok do metody i obsługę wyników na stosie/stercie.

Tym się zajmuje kompilator, poza tym szanujące się platformy i tak nie zrobią żadnego wywołania metody.

Afish napisał(a):

Slepiec napisał(a):

Metody powinny być krótkie, ale w tym przypadku możesz zataić co masz na myśli, każdy myśli że isEven zwróci liczbę parzystą ale jak coś skopiesz w tej metodę to wprowadzisz wiele osób w błąd.

Jak skopiesz w innym miejscu, to też wprowadzisz w błąd.

Tak ale jak skopiesz w metodzie która powinna być oczywista a nie jest, to chyba gorzej.

Slepiec napisał(a):

Metody statyczne to w cale nie jakieś mistrzostwo w programowaniu obiektowym.

Do sprawdzenia podzielności liczby nie trzeba kontenera DI i fabryki strategii.

Do oczywistych rzeczy nie trzeba takich metod wcale.

Slepiec napisał(a):

Wydajność.... niby nic wielkiego - jedna linijka, ale jednak trzeba odłożyć skok do metody i obsługę wyników na stosie/stercie.

Tym się zajmuje kompilator, poza tym szanujące się platformy i tak nie zrobią żadnego wywołania metody.

Tak, owszem, obyśmy mieli do czynienia tylko z szanującymi się platformami :D

Slepiec napisał(a):

Tak ale jak skopiesz w metodzie która powinna być oczywista a nie jest, to chyba gorzej.

Jak dla mnie w obu miejscach jest to samo, a jak nie wyjdzie na testach i przeglądzie kodu, to nieużycie małej metody i tak nie pomoże.

Slepiec napisał(a):

Do oczywistych rzeczy nie trzeba takich metod wcale.

Nie chodzi o oczywistość, tylko o czytelność. Dla jednego użycie operatora modulo będzie czytelne, inny użyje koniunkcji bitowej, a jeszcze inny metody z biblioteki standardowej. Zależy od zespołu.

Slepiec napisał(a):

Tak, owszem, obyśmy mieli do czynienia tylko z szanującymi się platformami :D

To wtedy masz o wiele większe problemy, niż jakieś tam wywołanie metody, do tego platforma może wreszcie zacząć się szanować i wprowadzać optymalizacje, a ręczne inlineowanie metod wcale nie jest optymalizacją.

Metody powinny być krótkie, ale w tym przypadku możesz zataić co masz na myśli, każdy myśli że isEven zwróci liczbę parzystą ale jak coś skopiesz w tej metodę to wprowadzisz wiele osób w błąd.

Nazwa isEven jest zła, bo jest zbyt oczywista? To może nazwa hasMagicalProperty byłaby lepsza? Wtedy nikt nie byłby przesadnie pewny siebie, ba każdy byłby podejrzliwy i sprawdzałby ciało metody.

Jeśli metoda jest skopana to znaczy, że:

• była słabo wytestowana (prawdopodobnie wcale, bo łatwe metody łatwo się testuje)
• wystarczy poprawić kod w jednym miejscu, a nie w wielu

Aczkolwiek robienie metody isEven to już nieco przegięcie. Myślę, że przez takie kiepskie przykłady dyskusja nad krótkimi metodami idzie w złym kierunku.

Po prostu zmieść całą metodę w jednym ekranie lub mniej. Tworzenie miliona jednolinijkowych metod to przesada w drugą stronę, bo nie bardzo wyobrażam sobie analizę takiego kodu potem.

Ja tylko spytałem o opinię, nie chciałem niczego złego :D W każdym razie dziękuję za każdą odpowiedź! Myślę, że dyskusję można zakończyć.

Ja mam taką metauwagę:

Dobry kod powinien być czytelny i zrozumiały po samych nazwach klas/metod/zmiennych,

dobry kod nie musi być czytelny po samych nazwach klas, bo może być czytelny na poziomie samego języka. I teraz:
number % 2 == 0;

Myślę, że dla każdego programisty, który jest w stanie napisać fizz-buzz (klasyczne zadanie sprawdzające, czy znasz podstawy programowania), zapis number % 2 będzie zrozumiały. Tak samo jeśli masz kod a + b to nie musisz wydzielać go do funkcji addTwoNumbers(a, b) bo to by tylko zaciemniło kod.

Nie ma nic złego w używaniu operatorów matematycznych (czy innych) istniejących w języku. Po to one są w końcu. To operatory są już same w sobie cukrem składniowym i świadczą o tym, że programujemy w języku wysokiego poziomu a nie np. w assemblerze*

Chyba, że faktycznie masz jakiś skomplikowany wzór matematyczny

x * x * 20 + x * 10 + 498 * sqrt(cos(angle) * r + sin(angle) * r2) 

(ten wzór nie ma sensu oczywiście, tak wymyśliłem na poczekaniu)

tutaj będzie miało więcej sensu wydzielenie tego do osobnej funkcji i nazwanie funkcji nazwą wzoru.

*swoją drogą ciekawy trend. W C++ pamiętam ludzie uciekali od funkcji i tworzyli własne operatory (żeby mogli pisząc transformacje geometryczne napisać np. vec1 + vec2 zamiast addVectors(vec1, vec2). A w językach, w których nie da się przeciążać operatorów jak widać ludzie biorą operatory i zamieniają w funkcje. Np. dzisiaj się dowiedziałem, że istnieje biblioteka JS, która wykonuje operację a > 0: npmjs.com/package/is-positive . Widać ludzie zawsze będą niezadowoleni z tego co mają w języku...

Wibowit napisał(a):

Zależy od długości i stopnia wykorzystania. Np 1-linijkowe metody używane w jednym miejscu zwykle nie miałyby sensu - takie metody można zastąpić po prostu zmiennymi. Jednak w wielu przypadkach jednolinijkowe metody mają sens, bo opisują co robi dany kod.
Ale taka metoda już coś wnosi:

Kopiuj

boolean userCanEditItem(User user, Item item) {
  return user.isAdmin() || user.manageableItemTypes.contains(item.type);
}

bo zdecydowanie szybciej jestem w stanie zrozumieć to:

Kopiuj

if (!userCanEditItem(user, item)) {
  throw new Error("illegal operation");
}

Nie chodzi tylko, żeby zrozumieć na pierwszy rzut oka, co to robi. Metoda sprawdzenia, czy użytkownik może edytować element, może się zmienić. I jeśli masz metodę, to zmieniasz tylko w jednym miejscu w aplikacji.

Jeśli fragmentu kodu używasz w wielu miejscach i ten kod ma szansę, żeby kiedyś się zmienić (np. fileExists, userCanEditItem, avgLineWidth), to zdecydowanie metoda ma tu sens.

W innych sytuacjach to wszystko zależy :)