Czy duże wyrażenie logiczne jest bardziej czytelne niż to samo wyrażenie podzielone na metody predykatów? [Zamknięte]

63

Co jest łatwiejsze do zrozumienia, duża instrukcja boolowska (dość złożona) lub ta sama instrukcja w podziale na metody predykatów (dużo dodatkowego kodu do odczytania)?

Opcja 1, duże wyrażenie logiczne:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {

        return propVal.PropertyId == context.Definition.Id
            && !repo.ParentId.HasValue || repo.ParentId == propVal.ParentId
            && ((propVal.SecondaryFilter.HasValue && context.SecondaryFilter.HasValue && propVal.SecondaryFilter.Value == context.SecondaryFilter) || (!context.SecondaryFilter.HasValue && !propVal.SecondaryFilter.HasValue));
    }

Opcja 2, warunki w podziale na metody predykatów:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {
        return MatchesDefinitionId(context, propVal)
            && MatchesParentId(propVal)
            && (MatchedSecondaryFilter(context, propVal) || HasNoSecondaryFilter(context, propVal));
    }

    private static bool HasNoSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);
    }

    private static bool MatchedSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    }

    private bool MatchesParentId(TValToMatch propVal)
    {
        return (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    }

    private static bool MatchesDefinitionId(CurrentSearchContext context, TValToMatch propVal)
    {
        return propVal.PropertyId == context.Definition.Id;
    }

Wolę drugie podejście, ponieważ widzę nazwy metod jako komentarze, ale rozumiem, że jest to problematyczne, ponieważ musisz przeczytać wszystkie metody, aby zrozumieć, co robi kod, więc abstrahuje on od intencji kodu.

willem
źródło
13
Opcja 2 jest podobna do tej, którą zaleca Martin Fowler w swojej książce o refaktoryzacji. Ponadto nazwy metod służą jako cel wszystkich wyrażeń losowych, zawartość metod to tylko szczegóły implementacji, które mogą się zmieniać z czasem.
programista
2
Czy to naprawdę to samo wyrażenie? „Lub” ma mniejszy priorytet niż „I”. W każdym razie drugi mówi o twoich zamiarach, drugi (pierwszy) jest techniczny.
pakujący
3
Co mówi @ pakujący. Fakt, że zrobienie tego w pierwszy sposób spowodował błąd, jest całkiem dobrą wskazówką, że pierwszy sposób nie jest łatwo zrozumiały dla bardzo ważnego sektora docelowej grupy odbiorców. Siebie!
Steve Jessop
3
Opcja 3: Nie lubię żadnej. Drugi jest absurdalnie gadatliwy, pierwszy nie jest równoważny drugiemu. Pomoc w nawiasach.
David Hammen,
3
Może to być pedantyczne, ale nie masz żadnych if instrukcji w żadnym bloku kodu. Twoje pytanie dotyczy wyrażeń logicznych .
Kyle Strand,

Odpowiedzi:

88

Co jest łatwiejsze do zrozumienia

To drugie podejście. Jest nie tylko łatwiejszy do zrozumienia, ale także łatwiej pisać, testować, refaktoryzować i rozszerzać. Każdy wymagany warunek można bezpiecznie oddzielić i obsługiwać na swój własny sposób.

jest to problematyczne, ponieważ musisz przeczytać wszystkie metody, aby zrozumieć kod

Nie ma problemu, jeśli metody są poprawnie nazwane. W rzeczywistości łatwiej byłoby to zrozumieć, ponieważ nazwa metody opisuje zamiar warunku.
Widz if MatchesDefinitionId()jest bardziej objaśniający niżif (propVal.PropertyId == context.Definition.Id)

[Osobiście pierwsze podejście odświeża moje oczy.]

cudowny dzwonek
źródło
12
Jeśli nazwy metod są dobre, łatwiej jest je zrozumieć.
BЈовић
I proszę, uczyń je (nazwy metod) znaczącymi i krótkimi. Ponad 20 nazw znaków znosi moje oczy. MatchesDefinitionId()jest granicą.
Mindwin
2
@Mindwin Jeśli sprowadza się do wyboru między utrzymywaniem krótkich nazw metod a utrzymywaniem ich w znaczeniu, mówię, że za każdym razem używaj tej drugiej. Krótki jest dobry, ale nie kosztem czytelności.
Ajedi32,
@ Ajedi32 nie trzeba pisać eseju o tym, co robi metoda na nazwie metody, ani mieć gramatycznie poprawnych nazw metod. Jeśli utrzyma się jasne standardy skrótów (w grupie roboczej lub organizacji), nie będzie problemu z krótkimi nazwami i czytelnością.
Mindwin
Korzystaj z prawa Zipf: spraw, aby rzeczy były bardziej szczegółowe, aby zniechęcić do ich używania.
hoosierEE 11.03.16
44

Jeśli jest to jedyne miejsce, w którym będą używane te funkcje predykatów, możesz boolzamiast tego użyć również zmiennych lokalnych :

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    bool matchesDefinitionId = (propVal.PropertyId == context.Definition.Id);
    bool matchesParentId = (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    bool matchesSecondaryFilter = (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    bool hasNoSecondaryFilter = (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);

    return matchesDefinitionId
        && matchesParentId
        && matchesSecondaryFilter || hasNoSecondaryFilter;
}

Można je również rozbić i uporządkować, aby były bardziej czytelne, np. Za pomocą

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

a następnie zastępując wszystkie wystąpienia propVal.SecondaryFilter.HasValue. Jedną z rzeczy, która od razu się wyróżnia, jest hasNoSecondaryFilterużycie logicznego AND na negowanych HasValuewłaściwościach, podczas gdy matchesSecondaryFilterlogiczne AND na niez negowanych HasValue- więc nie jest dokładnie odwrotnie.

Simon Richter
źródło
3
To rozwiązanie jest całkiem dobre i na pewno napisałem dużo podobnego kodu. Jest bardzo czytelny. Minusem, w porównaniu do rozwiązania, które opublikowałem, jest szybkość. Dzięki tej metodzie przeprowadzasz stos testów warunkowych bez względu na wszystko. W moim rozwiązaniu operacje można znacznie zmniejszyć na podstawie przetworzonych wartości.
BuvinJ
5
@BuvinJ Testy takie jak te pokazane tutaj powinny być dość tanie, więc jeśli nie wiem, że niektóre warunki są drogie lub jeśli nie jest to kod wrażliwy na wydajność, wybrałbym bardziej czytelną wersję.
svick
1
@svick Bez wątpienia jest to mało prawdopodobne, aby powodowało to problemy z wydajnością przez większość czasu. Jeśli jednak możesz zmniejszyć liczbę operacji bez utraty czytelności, dlaczego tego nie zrobić? Nie jestem przekonany, że jest to o wiele bardziej czytelne niż moje rozwiązanie. Daje samo-dokumentujące „nazwy” testom - co jest miłe ... Myślę, że sprowadza się to do konkretnego przypadku użycia i tego, jak zrozumiałe są same testy.
BuvinJ
Dodanie komentarzy może również pomóc w
zwiększeniu
@BuvinJ Naprawdę podoba mi się to rozwiązanie, ponieważ ignorując wszystko oprócz ostatniej linii, mogę szybko zrozumieć, co robi. Myślę, że to jest bardziej czytelne.
svick,
42

Ogólnie ten drugi jest preferowany.

To sprawia, że ​​strona połączeń jest bardziej wielokrotnego użytku. Obsługuje DRY (co oznacza, że ​​masz mniej miejsc do zmiany, gdy zmieniają się kryteria, i możesz to zrobić bardziej niezawodnie). I bardzo często te podkryteria to rzeczy, które zostaną ponownie wykorzystane niezależnie gdzie indziej, umożliwiając ci to.

Aha, a to znacznie ułatwia testowanie jednostkowe, dając pewność, że zrobiłeś to poprawnie.

Telastyn
źródło
1
Tak, chociaż twoja odpowiedź powinna również dotyczyć poprawiania użycia repo, które wydaje się statycznym polem / właściwością, tj. Zmienną globalną. Metody statyczne powinny być deterministyczne i nie powinny wykorzystywać zmiennych globalnych.
David Arno,
3
@DavidArno - choć nie jest to świetne, wydaje się styczne do podjętego pytania. I bez dodatkowego kodu jest prawdopodobne, że istnieje półprawidłowy powód, dla którego projekt działa w ten sposób.
Telastyn
1
Tak, nie ważne repo. Musiałem trochę zaciemnić kod, nie chcę udostępniać kodu klienta w stanie, w jakim się znajduje w interwebach :)
willem
23

Jeśli jest pomiędzy tymi dwiema opcjami, ta druga jest lepsza. Nie są to jednak jedyne opcje! Co powiesz na rozbicie pojedynczej funkcji na wiele ifs? Sprawdź sposoby wyjścia z funkcji, aby uniknąć dodatkowych testów, z grubsza emulując „zwarcie” w teście z pojedynczą linią.

Jest to łatwiejsze do odczytania (może być konieczne dwukrotne sprawdzenie logiki dla twojego przykładu, ale koncepcja jest prawdziwa):

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    if( propVal.PropertyId != context.Definition.Id ) return false;

    if( repo.ParentId.HasValue || repo.ParentId != propVal.ParentId ) return false;

    if( propVal.SecondaryFilter.HasValue && 
        context.SecondaryFilter.HasValue && 
        propVal.SecondaryFilter.Value == context.SecondaryFilter ) return true;

    if( !context.SecondaryFilter.HasValue && 
        !propVal.SecondaryFilter.HasValue) return true;

    return false;   
}
BuvinJ
źródło
3
Dlaczego dostałem opinię o tym w ciągu kilku sekund od opublikowania? Dodaj komentarz, gdy zagłosujesz! Ta odpowiedź działa równie szybko i jest łatwiejsza do odczytania. Więc w czym problem?
BuvinJ
2
@BuvinJ: Absolutnie nie ma w tym nic złego. Taki sam jak w oryginalnym kodzie, tyle że nie musisz walczyć z tuzinem nawiasów i pojedynczą linią, która rozciąga się poza koniec ekranu. Mogę odczytać ten kod od góry do dołu i natychmiast go zrozumieć. Liczba WTF = 0.
gnasher729,
1
Zwracanie innych niż na końcu funkcji sprawia, że ​​kod jest mniej czytelny, a nie bardziej czytelny, IMO. Wolę pojedynczy punkt wyjścia. Kilka dobrych argumentów w obie strony pod tym linkiem. stackoverflow.com/questions/36707/…
Brad Thomas
5
@Brad Thomas Nie mogę się zgodzić z jednym punktem wyjścia. Zwykle prowadzi do głęboko zagnieżdżonego nawiasu. Powrót kończy ścieżkę, więc dla mnie jest o wiele łatwiejszy do odczytania.
Borjab,
1
@BradThomas W pełni zgadzam się z Borjabem. Unikanie głębokich zagnieżdżeń jest powodem, dla którego używam tego stylu częściej niż do łamania długich instrukcji warunkowych. Zwykle piszę kod z mnóstwem zagnieżdżeń. Potem zacząłem szukać sposobów, aby nigdy nie zagłębiać więcej niż jednego lub dwóch zagnieżdżeń, a mój kod stał się DUŻO łatwiejszy do odczytania i utrzymania. Jeśli możesz znaleźć sposób na wyjście z tej funkcji, zrób to jak najszybciej! Jeśli możesz znaleźć sposób na uniknięcie głębokich zagnieżdżeń i długich warunków warunkowych, zrób to!
BuvinJ
10

Bardziej podoba mi się wariant 2, ale sugerowałbym jedną zmianę strukturalną. Połącz dwa kontrole w ostatnim wierszu warunku w jedno połączenie.

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    return MatchesDefinitionId(context, propVal)
        && MatchesParentId(propVal)
        && MatchesSecondaryFilterIfPresent(context, propVal);
}

private static bool MatchesSecondaryFilterIfPresent(CurrentSearchContext context, 
                                                    TValToMatch propVal)
{
    return MatchedSecondaryFilter(context, propVal) 
               || HasNoSecondaryFilter(context, propVal);
}

Sugeruję, że to dlatego, że dwie kontrole są pojedynczą jednostką funkcjonalną, a zagnieżdżanie nawiasów warunkowych jest podatne na błędy: zarówno z punktu widzenia początkowego pisania kodu, jak iz punktu widzenia osoby, która go czyta. Jest tak szczególnie w przypadku, gdy podelementy wyrażenia nie mają tego samego wzorca.

Nie jestem pewien, czy MatchesSecondaryFilterIfPresent()to najlepsza nazwa dla tej kombinacji; ale nic lepszego nie przychodzi mi na myśl.

Dan Neely
źródło
Bardzo miło, próba wyjaśnienia, co się dzieje w ramach metod, jest w rzeczywistości lepsza niż zwykłe wezwania do restrukturyzacji.
klaar
2

Chociaż w języku C #, kod nie jest bardzo zorientowany obiektowo. Wykorzystuje metody statyczne i coś, co wygląda na pola statyczne (np repo.). Ogólnie uważa się, że statyka sprawia, że ​​kod jest trudny do refaktoryzacji i trudny do przetestowania, a jednocześnie utrudnia ponowne użycie, a także, jeśli chodzi o twoje pytanie: użycie statyczne jest mniej czytelne i łatwe do utrzymania niż konstrukcja obiektowa.

Powinieneś przekonwertować ten kod do postaci bardziej obiektowej. Gdy to zrobisz, zauważysz, że istnieją rozsądne miejsca do umieszczenia kodu, który porównuje obiekty, pola itp. Prawdopodobnie możesz poprosić obiekty o porównanie się, co zredukowałoby Twoją dużą instrukcję if do proste żądanie porównania (np. if ( a.compareTo (b) ) { }które może obejmować wszystkie porównania w terenie).

C # ma bogaty zestaw interfejsów i narzędzi systemowych do wykonywania porównań na obiektach i ich polach. Poza oczywistym .Equalssposobem, na początek, patrzeć IEqualityComparer, IEquatablei jak media System.Collections.Generic.EqualityComparer.Default.

Erik Eidt
źródło
0

Ten ostatni jest zdecydowanie preferowany, widziałem przypadki z pierwszym sposobem i prawie zawsze jest to niemożliwe do odczytania. Popełniłem błąd, robiąc to w pierwszy sposób i poproszono mnie o zmianę na metody predykatów.

Podejrzeć
źródło
0

Powiedziałbym, że oba są mniej więcej takie same, JEŻELI dodacie trochę białych znaków dla czytelności i kilka komentarzy, które pomogą czytelnikowi w bardziej niejasnych częściach.

Pamiętaj: dobry komentarz mówi czytelnikowi, o czym myślałeś , pisząc kod.

Przy takich zmianach, jakie zasugerowałem, prawdopodobnie wybrałbym poprzednie podejście, ponieważ jest mniej zagracone i rozproszone. Wywołania podprogramów są jak przypisy: dostarczają użytecznych informacji, ale zakłócają przepływ czytania. Gdyby predykaty były bardziej złożone, podzieliłbym je na osobne metody, aby koncepcje, które zawierają, można było zbudować w zrozumiałych częściach.

Mark Wood
źródło
Zasługuje na +1. Dobre jedzenie do przemyślenia, choć nie jest to popularna opinia oparta na innych odpowiedziach. Dzięki :)
willem
1
@willem Nie, nie zasługuje na +1. Dwa podejścia nie są takie same. Dodatkowe komentarze są głupie i niepotrzebne.
BЈовић
2
Dobry kod NIGDY nie zależy od komentarzy, aby były zrozumiałe. W rzeczywistości komentarze są najgorszym bałaganem, jaki może mieć kod. Kod powinien mówić sam za siebie. Ponadto dwa podejścia, które OP chce ocenić, nigdy nie mogą być „mniej więcej takie same”, bez względu na to, ile dodanych białych spacji.
cudowny dzwonek
Lepiej mieć sensowną nazwę funkcji niż czytać komentarz. Jak stwierdzono w książce „Czysty kod”, komentarz oznacza brak wyrażenia kodu rzutu. Po co tłumaczyć, co robisz, gdy funkcja mogła to wyrazić o wiele wyraźniej.
Borjab,
0

Cóż, jeśli są części, których możesz chcieć użyć ponownie, rozdzielenie ich na osobne odpowiednio nazwane funkcje jest oczywiście najlepszym pomysłem.
Nawet jeśli nigdy ich nie użyjesz, może to pozwolić na lepszą strukturę warunków i nadanie im etykiety opisującej ich znaczenie .

Spójrzmy teraz na twoją pierwszą opcję i przyznaj, że ani wcięcie, ani łamanie linii nie były tak przydatne, ani warunek nie był tak dobrze skonstruowany:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal) {
    return propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue
        || repo.ParentId == propVal.ParentId
        && propVal.SecondaryFilter.HasValue == context.SecondaryFilter.HasValue
        && (!propVal.SecondaryFilter.HasValue || propVal.SecondaryFilter.Value == context.SecondaryFilter.Value);
}
Deduplikator
źródło
0

Pierwszy jest absolutnie okropny. Używałeś || dla dwóch rzeczy w tej samej linii; jest to albo błąd w kodzie, albo zamiar zaciemnienia kodu.

    return (   (   propVal.PropertyId == context.Definition.Id
                && !repo.ParentId.HasValue)
            || (   repo.ParentId == propVal.ParentId
                && (   (   propVal.SecondaryFilter.HasValue
                        && context.SecondaryFilter.HasValue 
                        && propVal.SecondaryFilter.Value == context.SecondaryFilter)
                    || (   !context.SecondaryFilter.HasValue
                        && !propVal.SecondaryFilter.HasValue))));

Jest to przynajmniej w połowie przyzwoicie sformatowane (jeśli formatowanie jest skomplikowane, to dlatego, że warunek if jest skomplikowany), a ty masz przynajmniej szansę dowiedzieć się, czy coś w tym jest nonsensowne. W porównaniu do śmieci sformatowanych, jeśli coś jeszcze jest lepsze. Ale wydaje się, że potrafisz robić tylko skrajności: albo kompletny bałagan instrukcji if, albo cztery całkowicie bezcelowe metody.

Należy zauważyć, że (cond1 i& cond2) || (! cond1 && cond3) można zapisać jako

cond1 ? cond2 : cond3

co zmniejszy bałagan. Pisałbym

if (propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue) {
    return true;
} else if (repo.ParentId != propVal.ParentId) {
    return false;
} else if (propVal.SecondaryFilter.HasValue) {
    return (   context.SecondaryFilter.HasValue
            && propVal.SecondaryFilter.Value == context.SecondaryFilter); 
} else {
    return !context.SecondaryFilter.HasValue;
}
gnasher729
źródło
-4

Nie podoba mi się żadne z tych rozwiązań, oba są trudne do uzasadnienia i trudne do odczytania. Abstrakcja do mniejszych metod tylko dla mniejszych metod nie zawsze rozwiązuje problem.

Idealnie myślę, że metaprogrmatycznie porównałbyś właściwości, więc nie masz zdefiniowanej nowej metody lub gałęzi za każdym razem, gdy chcesz porównać nowy zestaw właściwości.

Nie jestem pewien co do c #, ale w javascript coś takiego byłoby DUŻO lepsze i mogłoby przynajmniej zastąpić MatchesDefinitionId i MatchesParentId

function compareContextProp(obj, property, value){
  if(obj[property])
    return obj[property] == value
  return false
}
użytkownik1152226
źródło
1
Nie powinno być problemu z implementacją czegoś takiego w C #.
Snoop,
Nie rozumiem, w jaki sposób logiczna kombinacja ~ 5 wywołań compareContextProp(propVal, "PropertyId", context.Definition.Id)byłaby łatwiejsza do odczytania niż logiczna kombinacja OP ~ 5 porównań formy propVal.PropertyId == context.Definition.Id. Jest znacznie dłuższy i dodaje dodatkową warstwę bez ukrywania złożoności witryny wywoływania. (jeśli to ma znaczenie, nie głosowałem)
Ixrec