Clean code - tylko "!string.IsNullOrWhiteSpace()" w metodzie?

Hej,

mam przykladowy kod (w C#):
if (IsCommentColumn() && !string.IsNullOrWhiteSpace(_abc.Comment)) { tooltip.Show(mousePosition); }

Czy umiescilibyscie (dla przejrzystosci) rowniez prosty kod !string.IsNullOrWhiteSpace(_abc.Comment) w osobnej metodzie?

Za mało kontekstu, pokaż więcej kodu.

Nie i w sumie nie wiem co to ma to clean code za bardzo. Ludzie rozstrząsają takie pierdoły jak im się za bardzo nudzi. Dla mnie nieistotny szczegół, a przeniesienie do metody ma ten minus, że trzeba jej wymyślić sensowną nazwę, inną niż IsCommentNullOrWhitespace(Abc abc).

Ja chyba bym zrobił

if (IsColumnNotEmpty()) {
  tooltip.Show(mousePosition);
}

boolean IsColumnNotEmpty() {
  return IsCommentColumn() && !string.IsNullOrWhiteSpace(_abc.Comment);
}

Ale tak na prawdę żeby wymyślić dobrą nazwę musisz wiedzieć:

a. jaki jest problem który próbujesz rozwiązać?
b. po co?
c. jak?

Bierzesz te trzy rzeczy pod uwagę, i wychodzi nazwa która to opisuje. @still.still. Czytam ten kod i domyślam się, że user najedzie myszką nad komentarz, to ma się pokazać tooltip, chyba że komentarz jest pusty, jak rozumiem?

Saalin napisał(a):

Nie i w sumie nie wiem co to ma to clean code za bardzo. Ludzie rozstrząsają takie pierdoły jak im się za bardzo nudzi. Dla mnie nieistotny szczegół, a przeniesienie do metody ma ten minus, że trzeba jej wymyślić sensowną nazwę, inną niż IsCommentNullOrWhitespace(Abc abc).

Minus?

Moim zdaniem właśnie to jest plus, bo jeśli nie wiesz jak nazwać pod-metodę, to to jest silny sygnał że nie do końca rozumiesz jak kod ma działać i jakie zadanie spełnia. Więc to jest bardzo dobra okazja, żeby samemu się nauczyć więcej, wymyślając nazwę. Jeśli nazwę jest trudno wymyślić to może to być sygnał że design kodu jest zły, i należy go poprawić.

@Riddle: Pokazanie tooltip dokladnie w komorce, ktora posiada komentarz. Cos na zasadzie:

if (IsCommentExists()) {
  tooltip.Show(mousePosition);
}

boolean IsCommentExists() {
  return IsCommentColumn() && IsCommentInCellExists();
}

boolean IsCommentInCellExists() {
  return !string.IsNullOrWhiteSpace(_abc.Comment); 
}

still.still napisał(a):

@Riddle: Pokazanie tooltip dokladnie w komorce, ktora posiada komentarz. Cos na zasadzie:

Sam sobie odpowiedziałeś "w komorce, ktora posiada komentarz" -> CellHasComment().

if (CellHasComment()) {
  tooltip.Show(mousePosition);
}

boolean CellHasComment() {
  return IsCommentColumn() && !string.IsNullOrWhiteSpace(_abc.Comment);
}

Riddle napisał(a):

still.still napisał(a):

@Riddle: Pokazanie tooltip dokladnie w komorce, ktora posiada komentarz. Cos na zasadzie:

Sam sobie odpowiedziałeś "w komorce, ktora posiada komentarz" -> CellHasComment().

if (CellHasComment()) {
  tooltip.Show(mousePosition);
}

boolean CellHasComment() {
  return IsCommentColumn() && !string.IsNullOrWhiteSpace(_abc.Comment);
}

@Riddle: Nie lepiej pisac IsCommentInCellExists() , bo wtedy widzac Is na poczatku nazwy metody wiem, ze to jest metoda, ktora zwraca boolean.

still.still napisał(a):

@Riddle: Nie lepiej pisac IsCommentInCellExists() , bo wtedy widzac Is na poczatku nazwy metody wiem, ze to jest metoda, ktora zwraca boolean.

Ja bym tak nie napisał, bo jest niegramatycznie. Jak widzę HasComment, to przez słowo "has" też wiem że zwraca booean, tak samo jak can*(), has(), exists(), etc. Więc ja sugerowałbym zostawienie CellHasComment. Myślę że byłby to dla Ciebie dobry krok w stronę lepszych nazw.

Ale jak bardzo chcesz zostać w swojej strefie komfortu, żeby zaczynać wszystkie metody boolean od Is to możesz nazwać IsCellWithComment() albo IsCellNotEmpty(), ale moim zdaniem to jest gorsze.

still.still napisał(a):

@Riddle: Nie lepiej pisac IsCommentInCellExists() , bo wtedy widzac Is na poczatku nazwy metody wiem, ze to jest metoda, ktora zwraca boolean.

To w ogóle jest poprawnie po angielsku? już n-ty raz widzę ten pattern "IsSomethingExist" i dla mnie to brzmi dziwnie

Czy umiescilibyscie (dla przejrzystosci) rowniez prosty kod !string.IsNullOrWhiteSpace(_abc.Comment) w osobnej metodzie?

Jeżeli taki check występuje więcej niż jeden raz, to na pewno.

Jeżeli nie to można przymknąć oko, ale akurat to wydaje się być czymś, co pojawi się więcej niż jeden raz

jeśli taka prosta i krótka ifologia się pojawia tylko raz w kodzie to bym zrobił zmienne lokalne (typu boolean), a nie serię metod. nie wiem jak bardzo (nie)czyste jest to moje podejście.

minusem serii zwykłych zmiennych lokalnych jest brak zachowania typu short-circuit, ale tu nie powinno mieć to znaczenia (chociaż w przypadku ogólnym trzeba uważać).

Czy umiescilibyscie (dla przejrzystosci) rowniez prosty kod !string.IsNullOrWhiteSpace(_abc.Comment) w osobnej metodzie?

Nie wiem jaki jest szerszy kontekst, więc może nie pasować do Twojego przypadku. Dlaczego tego całego if'a nie wrzucić do osobnej metody? showToolTipForCommentColumn.
Ten kto czyta kod w którym jest to użyte, powinien zrozumieć intencję, a jak będzie ciekawy to podejrzy implementację.

yarel napisał(a):

Czy umiescilibyscie (dla przejrzystosci) rowniez prosty kod !string.IsNullOrWhiteSpace(_abc.Comment) w osobnej metodzie?

Nie wiem jaki jest szerszy kontekst, więc może nie pasować do Twojego przypadku. Dlaczego tego całego if'a nie wrzucić do osobnej metody? showToolTipForCommentColumn.
Ten kto czyta kod w którym jest to użyte, powinien zrozumieć intencję, a jak będzie ciekawy to podejrzy implementację.

Mam dwa wywolania w innych eventach. Obydwa maja inny warunek w if i jedno z wywolan ma (po wywolaniu tooltipa) jeszcze czyszczenie zmiennej (ustawianie na null), wiec troche to dziwne bylo by, ze przenosze tylko jedno wywolanie do metody. Musialbym napisac kolejne dwie methody. Jedna showToolTipForCommentColumn i druga jeszcze dla innego warunku oraz czyszenia zmiennej (ustawianie na null) po wyswietleniu tooltipa.

WeiXiao napisał(a):

To w ogóle jest poprawnie po angielsku? już n-ty raz widzę ten pattern "IsSomethingExist" i dla mnie to brzmi dziwnie

I can able to agree.

To co najmniej DoesCommentExist powinno być, ewentualnie ShouldShowComment - i zawierać wszystkie warunki. Metoda będąca jedynie wrapperem na IsNullOrEmpty nie ma dużego sensu, ale sensownie nazwana metoda pozwalająca stwierdzić czemu w ogóle potrzebujemy tego warunku, już tak.

still.still napisał(a):

@Riddle: Nie lepiej pisac IsCommentInCellExists() , bo wtedy widzac Is na poczatku nazwy metody wiem, ze to jest metoda, ktora zwraca boolean.

O tym, że zwraca bool wiadomo stąd, że np. jest używana w if.

W innych miejscach też stosujesz notację węgierską?

Wszyscy powyżej prócz somekind wymyślają jakieś gramatyczne potwory w nazwach tak prostych. Powinno być zawsze hasXXXSomething, doesXXXExists, canXXXDoSomething, isXXXEmpty.

Odpowiedź na pytanie zawarte w funkcji ma zwracać boolean - tak, nie i tyle.

A czy rozbić tak prosty warunek na osobną funkcję? Nie, ja bym zrobił jakiś StringUtils który ma metodę isNotEmpty (zamiast !string.IsNullOrWhiteSpace()) i będzie znacznie przejrzyściej. Gdy warunki będą skomplikowane i będzie wynikać to z decyzji biznesowych to wtedy można się pokusić o rozdzielenie. A tak lepiej skupić się na czytelności.

Płacą u was od linii kodu, że logikę prostego ifa na dwie linie opakowujecie w kolejne 5?
Jeszcze zrozumiałbym jakby tu była jakaś zawiła logika biznesowa, ale nie string.IsNullOrWhiteSpace