#2 Recepta na – pretty if
Druga „recepta na” będzie związana z warunkami if i skracaniem ich zapisu a zarazem poprawy ich czytelności.
Na początek przykład nad którym popracujemy:
public class MyBusinessClass { AuthorizationService authorizationService = new AuthorizationService(); public void AddOrderToInvoice(Order order, Invoice invoice, UserInfo user) { if (authorizationService.HasAuthorization(order, user, AuthorizationLevel.Read) && authorizationService.HasAuthorization(invoice, user, AuthorizationLevel.Write)) { //do something } } }
Klasa biznesowa, której metoda AddOrderToInvoice musi wykonać sprawdzenie czy użytkownik ma odpowiednie uprawnienia do wykonania tej operacji. Weryfikujemy uprawnienia do odczytu na zamówieniu i uprawnienia do zapisu na fakturze. Od razu zaznaczam, że jest to pseudo kod – tak aby pokazać istotę długich, skomplikowanych warunków if. (W sumie nie taki on pseudo bo spotykam go dość często …)
Pierwsze co możemy zrobić bez większego wysiłku to oczywiście przerzucenie drugiego warunku do osobnej linii:
public void AddOrderToInvoice(Order order, Invoice invoice, UserInfo user) { if (authorizationService.HasAuthorization(order, user, AuthorizationLevel.Read) && authorizationService.HasAuthorization(invoice, user, AuthorizationLevel.Write)) { //do something } }
Jest o tyle lepiej, że przynajmniej mieści się już na ekranie 😉 Ale to jeszcze średnio mnie zadowala. W takich sytuacjach często brakuje biznesowego opisu co dany warunek weryfikuje. A jakby tak wyciągnąć tą weryfikację przed klauzulę if przypisując wyniki do zmiennych, których nazwy dobrze je opisują?
public void AddOrderToInvoice(Order order, Invoice invoice, UserInfo user) { bool hasReadPermissionOnOrder = authorizationService.HasAuthorization(order, user, AuthorizationLevel.Read); bool hasWritePermissionOnInvoice = authorizationService.HasAuthorization(invoice, user, AuthorizationLevel.Write); if (hasReadPermissionOnOrder && hasWritePermissionOnInvoice) { //do something } }
Teraz śledząc taki warunek nie musimy czytać całego wywołania metod tylko wprost widzimy co jest sprawdzane. Czy możemy to tak zostawić? … No pewnie, że nie! Przerzucając od tak wywołania metod do zmiennych pozbyliśmy się jednej bardzo istotnej właściwości operatora &&. Sprawdzenie drugiego warunku (uprawnień do zapisu na fakturze) odbywało się dopiero kiedy pierwszy warunek (uprawnienia do odczytu na zamówieniu) zwracał wartość true. Aktualnie wykonujemy dwa sprawdzenia niezależnie. W przypadku prostych metod, nie sięgających do bazy, zużywających zasobów itd. może nie byłoby to problemem. Ja jednak optuje za jeszcze innym rozwiązaniem. Nazywam to „lazy check” 😉 W tym celu wykorzystuje delegat Func<T, TResult>
public void AddOrderToInvoice(Order order, Invoice invoice, UserInfo user) { Func<bool> hasReadPermissionOnOrder = () => authorizationService.HasAuthorization(order, user, AuthorizationLevel.Read); Func<bool> hasWritePermissionOnInvoice = () => authorizationService.HasAuthorization(invoice, user, AuthorizationLevel.Write); if (hasReadPermissionOnOrder() && hasWritePermissionOnInvoice()) { //do something } }
Wprowadziłem delegat Func<bool> z deklaracją typu „inline”. Jego wykonanie nastąpi dopiero w klauzuli if przy czym jeśli hasReadPermissionOnOrder() zwróci wartość false nie nastąpi wykonanie hasWritePermissionOnInvoice() – a o to ostatecznie nam chodziło.
Jeśli macie jakieś swoje „recepty na” klauzule if to podzielcie się nimi w komentarzach. Chętnie dowiem się co sądzicie o tym konkretnym rozwiązaniu.
Hej,
Ja w takiej sytuacji odwracam warunki, rozbijam je na dwa oddzielne ify i po prostu wychodzę z metody. Czyli coś w stylu:
public void AddOrderToInvoice(Order order, Invoice invoice, UserInfo user)
{
if (authorizationService.HasAuthorization(order, user, AuthorizationLevel.Read) == false) {
return;
}
if (authorizationService.HasAuthorization(invoice, user, AuthorizationLevel.Write) == false)
{
return;
}
//do something
}
Myślę, że na dłuższą metę jest to czytelniejsze. Nie wprowadzamy dodatkowych elementów w kodzie, jak przy rozwiązaniu z delegatem oraz podobnie jak w przypadku && druga metoda nie wykona się, gdy pierwsza zwróci false.
Cześć Daniel,
Cenna uwaga! 'Fail fast’ staram się stosować jak najczęściej.
W tym przykładzie nie do końca chciałem to pokazać a bardziej skupić się na zaprezentowanym właśnie podejściu – bez odwracania logiki przepływu. Z 'fail fast’ mam trochę problem. Gryzie mi się z zasadą, że metoda powinna mieć jeden punkt wyjścia. A w tym przykładzie nie da się tego uzyskać – będą co najmniej 3. Więc tutaj chyba nie będzie sytuacji win-win.
Pozdrawiam Grzesiek
Tworzenie delegatów (bo tym w praktyce są Func i Action w C#) specjalnie po to, żeby je nazwać zupełnie mija się z tym po co one są i z samym wzorcem delegata. Każdy taki kolejny funktor zaciemniałby jedynie kod, a przy nieumiejętnym użyciu Func utworzył domknięcia. Dla mnie kod z drugiego wycinka jest najlepszy i „szyty na miarę”. Jeśli ktoś chciałby dodawać kolejne warunki zawsze można skorzystać z Extract Method i nazwać ją w stylu CanExecute. Lub tak jak kolega wyżej jeśli mamy dużą liczbę warunków walidujących, to zgodnie ze sztuką ostatni „return” powinien być naszym oczekiwanym punktem wyjścia, bądź „krótką” funkcją, która ma się wykonać. Bardzo przekombinowane to opakowywanie w funktory.
Ja osobiście warunki, zmienne lokalne wyrzucam do nowej metody z dwóch powodów. Pierwsze, to pomoc JIT’owi w inlining’owaniu metod. Drugie, to usunięcia lokalnych gc root’ow, od których GC musiałby zacząć oznaczanie(faza mark). Czyli oba podejścia stricte optymalizacyjne a poza tym respektuję Fowlera i Beck’a to trzymam się ich podejścia do refactoringu i nazewnictwa 🙂
W tym kontekście bym aż tak mocno na performance nie zwracał uwagi. Ale! Czy mieliście już okazję stosować funkcje lokalne dostępne w C# 7.0? https://docs.microsoft.com/pl-pl/dotnet/csharp/programming-guide/classes-and-structs/local-functions#local-function-syntax
Ja osobiście jeszcze nie ale mogłyby z powodzeniem zastąpić użyte tutaj delegaty. Dlaczego tak? Jeśli każdą taką małą funkcjonalność wyrzucał bym do odrębnej metody w klasie to mogłaby wystąpić duża granulacja takich malutkich metod, które używane są tylko w jednym kontekście. A tak były by zamknięte wewnątrz tej metody biznesowej. Kontrowersyjne 🙂 Ale co o tym sądzicie?