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

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.

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.]

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.

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.

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;   
}

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.

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.

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.

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.

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);
}

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;
}

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
}