Oczy me zostały ukłute ostatnie takim oto pytaniem:
“Where are the null checks?”
Nie pamiętam czy to było na poziomie kontrolerów, czy może gdzieś w serwisie, albo gdzieś dalej. Pamiętam tylko, że jeden z przeglądających kod odczuł potrzebę posiadania takiego sprawdzenia, a osoba poddająca się kontroli spełniła tą prośbę, bez najmniejszego zająknięcia się. W taki oto sposób mamy teraz kod:
Dlaczego mnie to boli? Bo są to ludzie, którzy tworzą soft od bardzo długa, ale nadal nie wypracowali sobie dobrej zasady pracy z nulami (czy w ogóle z ifami). Skoro null jest złą wartością, to skąd mogę wiedzieć że int jest poprawną wartością w całym swoim zakresie, czy long, czy wreszcie jakiś decimal.
Skoro już sprawdzamy wszystko ifami, to może warto także sprawdzić czy wszystkie pola w param2 typu Model też są poprawne, co może doprowadzić do takiego potworka:
Skoro sprawdzamy wszystko, to może warto także sprawdzić czy lista stringów też nie jest pusta? Nie będę tego pisać, mam nadzieję że potraficie sami to sobie wyobrazić. Takiego sprawdzania parametrów wejściowych może się narobić cała masa. I co, czy tak ma wyglądać kod z którym chcecie pracować?
Uprzedzając różne odpowiedzi: NIE. Całe to sprawdzenie powinno odbyć się gdzieś indziej i metoda foo nie powinna być w ogóle wywołana. Lub w warunkach niesprzyjających frameworków ogarnąć się w 3 linijkach kodu.
Opcja pierwsza została świetnie opisana przez Łukasza Kurzyńca i jest dostępna pod tym adresem: http://kurzyniec.pl/artykuly/automatyczna-walidacja-parametrow-akcji/
W dużym skrócie nadajemy atrybuty walidacyjne, które sprawdzają model (liczba pojedyncza) który zostanie przekazany do metody foo. Jeśli model nie spełnia wymagań, metoda nie zostaje w ogóle wywołana a do klienta zwracany jest odpowiedni kod błędu.
Opcja druga: samodzielne sprawdzenie czy model jest poprawny, ALE, zaraz. Żeby było jasne to nie wy sprawdzacie że model jest sprawny macie od tego ludzi:
- DataAnnotation
- FluentValidation
- IValidator
- IMójMechanizmSprawdzający,
- IValidationServiceChecker
- tutaj wpis inny rodzaj walidacji
Dzięki temu ograniczy zasięg smrodku do przykładowych trzech linijek:
Nie mnóżcie proszę linijek z ifami na początku każdej metody, bo przyjdzie czas na zmiany w kodzie i trzeba będzie wam poprawić te ify zmienić. Okaże że może być nullem, albo doda się nowy parametr i trzeba będzie dodać nowego ifa – będziecie pamiętać? (trochę jak z komentarzami – też się starzeją i nie mówią prawdy)
Ja osobiście lubię używać kontraktów. Taka naleciałość z programowania w Adzie, ale jakże przydatna 🙂
Podasz przykład jak to wygląda w kodzie?
Jasne http://pastebin.com/G7W3nBiK Prosta klasa z przykładem kontraktów pre, post oraz niezmiennikami pętli. Wystarczy ściągnąć paczkę https://marketplace.visualstudio.com/items?itemName=RiSEResearchinSoftwareEngineering.CodeContractsforNET i poustawiać w właściwościach projektu w zakładce Code Contracts co jak sygnalizować.
Dzięki! 🙂
Się totalnie zgadzam choć w przykładzie z null-checkiem chyba powinieneś rzucać ArgumentNullException?
Chyba masz zupełną rację. W kodzie który mnie inspirował jest poprawnie ArgumentNullException.
Sprawdzanie powinno być. Kropka. Jak już się przyzwyczaisz, że w kodzie nie ma prawie nulli Twoje życie będzie przyjemniejsze.
A żeby nie pisać ich dla każdego parametru można użyć automatu w postaci NullGuard.PostSharp albo NullGuard.Fody. Dzięki nim dostajesz “za darmo” taki sprawdzenie w każdym miejscu kodu (parametry, zwracane wartości, wartości właściwości). A tylko tam gdzie jesteśmy pewni, że null jest na miejscu możemy sprawdzanie wyłączyć.
I nie chodzi o to, że sprawdzenie powinno być przed wywołaniem metody – z tym się zgadzam. Chodzi o to, że jak już ktoś zapomni o sprawdzeniu to będzie dokładnie wiedział który parametr zawiódł. A jak nie masz rzucania wyjątkiem dla każdego parametru to NullReferenceException może polecieć w niejednoznacznym wyrażeniu (i nie wiadomo co było null) albo gdzie kilka wywołań wgłąb i ciężko znaleźć powód błędu.