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.
c#
readability
willem
źródło
źródło
if
instrukcji w żadnym bloku kodu. Twoje pytanie dotyczy wyrażeń logicznych .Odpowiedzi:
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.
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.]
źródło
MatchesDefinitionId()
jest granicą.Jeśli jest to jedyne miejsce, w którym będą używane te funkcje predykatów, możesz
bool
zamiast tego użyć również zmiennych lokalnych :Można je również rozbić i uporządkować, aby były bardziej czytelne, np. Za pomocą
a następnie zastępując wszystkie wystąpienia
propVal.SecondaryFilter.HasValue
. Jedną z rzeczy, która od razu się wyróżnia, jesthasNoSecondaryFilter
użycie logicznego AND na negowanychHasValue
właściwościach, podczas gdymatchesSecondaryFilter
logiczne AND na niez negowanychHasValue
- więc nie jest dokładnie odwrotnie.źródło
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.
źródło
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.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):
źródło
Bardziej podoba mi się wariant 2, ale sugerowałbym jedną zmianę strukturalną. Połącz dwa kontrole w ostatnim wierszu warunku w jedno połączenie.
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.źródło
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
.Equals
sposobem, na początek, patrzećIEqualityComparer
,IEquatable
i jak mediaSystem.Collections.Generic.EqualityComparer.Default
.źródło
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.
źródło
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.
źródło
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:
źródło
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.
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
co zmniejszy bałagan. Pisałbym
źródło
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
źródło
compareContextProp(propVal, "PropertyId", context.Definition.Id)
byłaby łatwiejsza do odczytania niż logiczna kombinacja OP ~ 5 porównań formypropVal.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)