Jeśli funkcje muszą wykonać sprawdzanie wartości zerowej przed wykonaniem zamierzonego zachowania, czy to zły projekt?

67

Więc nie wiem, czy to dobry, czy zły projekt kodu, więc pomyślałem, że lepiej zapytam.

Często tworzę metody, które przetwarzają dane z wykorzystaniem klas i często przeprowadzam wiele kontroli metod, aby upewnić się, że nie otrzymam pustych referencji lub innych błędów.

Dla bardzo podstawowego przykładu:

// fields and properties
private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}
public void SetName(string name)
{
    if (_someEntity == null) return; //check to avoid null ref
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

Jak widać, za każdym razem sprawdzam, czy nie ma wartości null. Ale czy metoda nie powinna mieć tej kontroli?

Na przykład kod zewnętrzny powinien wyczyścić dane przed użyciem, aby metody nie musiały sprawdzać poprawności, jak poniżej:

 if(entity != null) // this makes the null checks redundant in the methods
 {
     Manager.AssignEntity(entity);
     Manager.SetName("Test");
 }

Podsumowując, czy metody powinny „sprawdzać poprawność danych”, a następnie przetwarzać je na danych, czy też powinno to być zagwarantowane przed wywołaniem metody, a jeśli nie sprawdzisz poprawności przed wywołaniem metody, powinna zgłosić błąd (lub złapać błąd)?

WDUK
źródło
7
Co się stanie, gdy ktoś nie wykona sprawdzenia zerowego i SetName i tak zgłosi wyjątek?
Robert Harvey
5
Co się stanie, jeśli naprawdę chcę, aby jakiś element był zerowy? Ty decydujesz, co chcesz, albo niektóreEntity może mieć wartość Null, albo nie, a następnie odpowiednio piszesz kod. Jeśli nie chcesz, aby nigdy nie był zerowy, możesz zastąpić czeki twierdzeniami.
gnasher729,
5
Możesz zobaczyć Granice Gary'ego Bernhardta mówiące o wyraźnym podziale między odkażaniem danych (tj. Sprawdzaniem wartości zerowych itp.) W zewnętrznej powłoce a posiadaniem wewnętrznego systemu rdzenia, który nie musi się tym przejmować - i po prostu wykonaj to praca.
mgarciaisaia
1
Z C # 8 potencjalnie nigdy nie będziesz musiał się martwić wyjątkami zerowego odwołania przy włączonych odpowiednich ustawieniach: blogs.msdn.microsoft.com/dotnet/2017/11/15/…
JᴀʏMᴇᴇ
3
Jest to jeden z powodów, dla których zespół językowy C # chce wprowadzić typy referencyjne zerowalne (i typy zerowalne) -> github.com/dotnet/csharplang/issues/36
Mafii

Odpowiedzi:

168

Problemem w twoim podstawowym przykładzie nie jest kontrola zerowa, to cicha porażka .

Błędy wskaźnika zerowego / odniesienia, najczęściej są błędami programisty. Błędy programisty często najlepiej jest rozwiązywać przez natychmiastowe i głośne awarie. Istnieją trzy ogólne sposoby rozwiązania problemu w tej sytuacji:

  1. Nie przejmuj się sprawdzaniem i po prostu pozwól środowisku wykonawczemu zgłosić wyjątek nullpointer.
  2. Sprawdź i wyrzuć wyjątek z komunikatem lepszym niż podstawowy komunikat NPE.

Trzecie rozwiązanie wymaga więcej pracy, ale jest znacznie bardziej niezawodne:

  1. Skonstruuj swoją klasę w taki sposób, aby było praktycznie niemożliwe, _someEntityaby kiedykolwiek była w niewłaściwym stanie. Jednym ze sposobów na to jest pozbycie się go AssignEntityi wymaganie go jako parametru do tworzenia instancji. Przydatne mogą być również inne techniki wstrzykiwania zależności.

W niektórych sytuacjach sensowne jest sprawdzanie poprawności wszystkich argumentów funkcji przed jakąkolwiek pracą, a w innych sensowne jest informowanie osoby dzwoniącej, że są odpowiedzialne za sprawdzenie poprawności wprowadzanych danych, a nie sprawdzanie. Który koniec spektrum zależy od dziedziny problemów. Trzecia opcja ma znaczącą zaletę, ponieważ można do pewnego stopnia zmusić kompilator do wymuszenia, aby program wywołujący zrobił wszystko poprawnie.

Jeśli trzecia opcja nie jest opcją, to moim zdaniem tak naprawdę nie ma znaczenia, dopóki nie po cichu zawiedzie. Jeśli nie sprawdzanie wartości zerowej powoduje, że program natychmiast eksploduje, dobrze, ale jeśli zamiast tego cicho uszkadza dane, powodując problemy na drodze, najlepiej sprawdzić i poradzić sobie z tym natychmiast.

Jaka jest nazwa?
źródło
10
@aroth: Każdy projekt czegokolwiek, łącznie z oprogramowaniem, będzie podlegał ograniczeniom. Sam projekt polega na tym, aby wybrać, dokąd zmierzają te ograniczenia, i nie jest moją odpowiedzialnością, ani też nie powinienem akceptować wkładu i oczekiwać, że zrobię z nim coś pożytecznego. Wiem, czy nulljest źle, czy źle, ponieważ w mojej hipotetycznej bibliotece zdefiniowałem typy danych i zdefiniowałem, co jest ważne, a co nie. Nazywasz to „wymuszaniem założeń”, ale zgadnij, co: tak robi każda istniejąca biblioteka oprogramowania. Są chwile, w których null jest prawidłową wartością, ale to nie zawsze.
whatsisname
4
„że Twój kod jest uszkodzony, ponieważ nie obsługuje się go nullpoprawnie” - To nieporozumienie co oznacza właściwe postępowanie. Dla większości programistów Java oznacza zgłoszenie wyjątku dla każdego niepoprawnego wejścia. Używając @ParametersAreNonnullByDefault, oznacza to również dla każdego null, chyba że argument jest oznaczony jako @Nullable. Robienie czegokolwiek innego oznacza wydłużanie ścieżki, aż w końcu zacznie wiać gdzie indziej, a tym samym wydłuży czas tracony na debugowanie. Cóż, ignorując problemy, możesz osiągnąć, że to nigdy nie wybuchnie, ale wtedy nieprawidłowe zachowanie jest dość gwarantowane.
maaartinus
4
@aroth Dobry Boże, nie chciałbym pracować z każdym kodem, który napiszesz. Wybuchy są nieskończenie lepsze niż robienie czegoś bezsensownego, co jest jedyną inną opcją dla nieprawidłowych danych wejściowych. Wyjątki zmuszają mnie, rozmówcę, do podjęcia decyzji, co należy zrobić w tych przypadkach, gdy metoda nie może odgadnąć, co zamierzałem. Sprawdzone wyjątki i niepotrzebne przedłużanie Exceptionsą całkowicie niepowiązanymi debatami. (Zgadzam się, że oba są kłopotliwe). Dla tego konkretnego przykładu, nulloczywiście nie ma sensu, jeśli celem klasy jest wywoływanie metod na obiekcie.
jpmc26
3
„Twój kod nie powinien forsować założeń w świecie dzwoniącego” - Nie można nie wymuszać na dzwoniącym założeń. Dlatego musimy uczynić te założenia tak wyraźnymi, jak to możliwe.
Diogo Kollross
3
@ zarówno twój kod jest zepsuty, ponieważ przekazuje mój kod zerowy, gdy powinien przekazywać coś, co ma właściwość Name. Każde zachowanie inne niż eksplodowanie jest jakąś dziwaczną wersją dynamicznego pisania IMHO typu backdoor.
mbrig
56

Ponieważ _someEntitymożna go modyfikować na dowolnym etapie, warto go przetestować za każdym razem, gdy SetNamezostanie wywołany. W końcu mogło się to zmienić od ostatniego wywołania tej metody. Ale pamiętaj, że kod w SetNamenie jest bezpieczny dla wątków, więc możesz wykonać to sprawdzenie w jednym wątku, _someEntityustawić na nullinny, a wtedy kod i NullReferenceExceptiontak zostanie zablokowany .

Dlatego jednym z podejść do tego problemu jest uzyskanie jeszcze większej obrony i wykonanie jednej z następujących czynności:

  1. sporządzić lokalną kopię _someEntityi odwołać się do niej za pomocą metody,
  2. użyj blokady, _someEntityaby upewnić się, że nie zmienia się ona podczas wykonywania metody,
  3. użyj a try/catchi wykonaj tę samą akcję na dowolnych NullReferenceExceptionelementach, które występują w metodzie, jak w przypadku początkowej kontroli zerowej (w tym przypadku nopakcji).

Ale tak naprawdę powinieneś przestać i zadać sobie pytanie: czy naprawdę musisz pozwolić _someEntityna nadpisanie? Dlaczego nie ustawić go raz, za pomocą konstruktora. W ten sposób wystarczy tylko raz wykonać zerową kontrolę raz:

private readonly Entity _someEntity;

public Constructor(Entity someEntity)
{
    if (someEntity == null) throw new ArgumentNullException(nameof(someEntity));
    _someEntity = someEntity;
}

public void SetName(string name)
{
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

Jeśli to w ogóle możliwe, takie podejście zalecam w takich sytuacjach. Jeśli nie możesz pamiętać o bezpieczeństwie wątków, wybierając sposób obsługi możliwych zer.

Jako komentarz dodatkowy czuję, że Twój kod jest dziwny i można go poprawić. Wszystkie z:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}

można zastąpić:

public Entity SomeEntity { get; set; }

Który ma tę samą funkcjonalność, z wyjątkiem możliwości ustawienia pola za pomocą narzędzia do ustawiania właściwości, a nie metody o innej nazwie.

David Arno
źródło
5
Dlaczego to odpowiada na pytanie? Pytanie dotyczy sprawdzania wartości zerowej i jest to w zasadzie przegląd kodu.
IEatBagels,
17
@TopinFrassi wyjaśnia, że ​​obecne podejście do sprawdzania wartości zerowej nie jest bezpieczne dla wątków. Następnie dotyka innych technik programowania obronnego, aby obejść ten problem. Następnie kończy się sugestią użycia niezmienności, aby uniknąć konieczności posiadania tego defensywnego programowania. Jako dodatkowy bonus oferuje porady, jak lepiej ustrukturyzować kod.
David Arno
23

Ale czy metoda nie powinna mieć tej kontroli?

To jest twój wybór.

Poprzez stworzenie publicmetody, jesteś oferując społeczeństwu możliwość to nazwać. Zawsze towarzyszy temu dorozumiana umowa dotycząca tego, jak wywołać tę metodę i czego się spodziewać. Umowa ta może (ale nie musinull ) zawierać „ tak, uch, jeśli podasz jako wartość parametru, eksploduje ci prosto w twarz ”. Inne części tego kontraktu mogą brzmieć „ och, BTW: za każdym razem, gdy dwa wątki wykonują tę metodę w tym samym czasie, umiera kotek ”.

Jaki rodzaj umowy chcesz zaprojektować, zależy od Ciebie. Ważne są:

  • stworzyć interfejs API, który jest użyteczny i spójny oraz

  • aby to dobrze udokumentować, szczególnie w przypadku przypadków skrajnych.

Jakie są prawdopodobne przypadki wywoływania metody?

zero
źródło
Cóż, bezpieczeństwo wątków jest wyjątkiem, a nie regułą, gdy istnieje równoczesny dostęp. Dlatego użycie stanu ukrytego / globalnego jest udokumentowane, a każda współbieżność spraw jest bezpieczna.
Deduplicator
14

Inne odpowiedzi są dobre; Chciałbym je rozszerzyć, komentując modyfikatory dostępności. Po pierwsze, co zrobić, jeśli nullnigdy nie jest ważne:

  • metody publiczne i chronione to te, w których nie kontrolujesz dzwoniącego. Powinny rzucać, gdy minę zero. W ten sposób trenujesz swoich rozmówców, aby nigdy nie przekazywali Ci wartości zerowej, ponieważ chcieliby, aby ich program się nie zawieszał.

  • wewnętrzne i prywatne metody są te, w których nie kontrolują rozmówcę. Powinny one twierdzić, gdy otrzymają wartość zero; prawdopodobnie później ulegną awarii, ale przynajmniej najpierw otrzymałeś to stwierdzenie i możliwość włamania się do debuggera. Ponownie, chcesz wyszkolić swoich rozmówców, aby dzwonili do ciebie poprawnie, sprawiając, że będzie bolało, gdy nie będą.

Co teraz robisz, jeśli podanie wartości null może być ważne? Unikałbym tej sytuacji z zerowym wzorcem obiektu . To znaczy: stwórz specjalną instancję tego typu i wymagaj, aby była używana za każdym razem, gdy potrzebny jest niepoprawny obiekt . Teraz wracasz do przyjemnego świata rzucania / zapewniania o każdym użyciu zer.

Na przykład nie chcesz być w takiej sytuacji:

class Person { ... }
...
class ExpenseReport { 
  public void SubmitReport(Person approver) { 
    if (approver == null) ... do something ...

i na stronie połączenia:

// I guess this works but I don't know what it means
expenses.SubmitReport(null); 

Ponieważ (1) trenujesz dzwoniących, że null jest dobrą wartością, a (2) nie jest jasne, co to jest semantyka tej wartości. Rób to raczej:

class ExpenseReport { 
  public static readonly Person ApproverNotRequired = new Person();
  public static readonly Person ApproverUnknown = new Person();
  ...
  public void SubmitReport(Person approver) { 
    if (approver == null) throw ...
    if (approver == ApproverNotRequired) ... do something ...
    if (approver == ApproverUnknown) ... do something ...

A teraz strona połączeń wygląda następująco:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

a teraz czytelnik kodu wie, co to znaczy.

Eric Lippert
źródło
Co więcej, nie ma łańcucha ifs dla obiektów zerowych, ale raczej używaj polimorfizmu, aby działały poprawnie.
Konrad Rudolph
@KonradRudolph: Rzeczywiście, to często dobra technika. Lubię to używać w strukturach danych, w których EmptyQueuetworzę typ, który rzuca się podczas usuwania z kolejki i tak dalej.
Eric Lippert,
@EricLippert - Wybacz mi, bo wciąż się uczę tutaj, ale załóżmy, jeśli Personklasa była niezmienna i nie zawierają konstruktor zerowego argumentu, w jaki sposób konstruować ApproverNotRequiredlub ApproverUnknowninstancje? Innymi słowy, jakie wartości przekażesz konstruktorowi?
2
Uwielbiam wpadać na twoje odpowiedzi / komentarze w całej SE, zawsze są wnikliwe. Gdy tylko zobaczyłem, że popieram metody wewnętrzne / prywatne, przewinąłem w dół, oczekując, że będzie to odpowiedź Erica Lipperta, nie byłem rozczarowany :)
Bob Esponja
4
Nadmiernie zastanawiacie się nad konkretnym przykładem, który podałem. Jego celem było szybkie zrozumienie wzoru zerowego obiektu. Nie miał być przykładem dobrego projektu w systemie automatyzacji kontroli dokumentów i wypłat. W prawdziwym systemie sprawienie, by „osoba zatwierdzająca” miała typ „osoba”, jest prawdopodobnie złym pomysłem, ponieważ nie pasuje do procesu biznesowego; może nie być osoby zatwierdzającej, możesz potrzebować kilku itd. Jak często zauważam, odpowiadając na pytania w tej domenie, naprawdę chcemy obiektu strategii . Nie rozłączaj się na przykładzie.
Eric Lippert,
8

Inne odpowiedzi wskazują, że twój kod może zostać wyczyszczony, aby nie wymagał kontroli zerowej tam, gdzie go masz, jednak w celu uzyskania ogólnej odpowiedzi na temat tego, co może być użytecznym sprawdzeniem zerowym, rozważ następujący przykładowy kod:

public static class Foo {
    public static void Frob(Bar a, Bar b) {
        if (a == null) throw new ArgumentNullException(nameof(a));
        if (b == null) throw new ArgumentNullException(nameof(b));

        a.Something = b.Something;
    }
}

Daje to przewagę w zakresie konserwacji. Jeśli kiedykolwiek wystąpi wada w kodzie, w której coś przekazuje obiekt zerowy do metody, otrzymasz przydatne informacje od metody, który parametr był pusty. Bez kontroli otrzymasz w wierszu wyjątek NullReferenceException:

a.Something = b.Something;

I (biorąc pod uwagę, że właściwość Something jest typem wartości) albo a lub b może mieć wartość NULL i nie będziesz mógł dowiedzieć się, który ze śladu stosu. Dzięki sprawdzeniom dowiesz się dokładnie, który obiekt był pusty, co może być niezwykle przydatne podczas próby usunięcia usterki.

Chciałbym zauważyć, że wzór w twoim oryginalnym kodzie:

if (x == null) return;

Może mieć zastosowanie w pewnych okolicznościach, może również (być może częściej) zapewniać bardzo dobry sposób maskowania podstawowych problemów w bazie kodu.

Paddy
źródło
4

Wcale nie, ale to zależy.

Sprawdzasz referencje zerowe, jeśli chcesz na nie zareagować.

Jeśli sprawdzisz zerową referencję i wyrzucisz sprawdzony wyjątek , zmusisz użytkownika metody do zareagowania na niego i umożliwienia mu powrotu do zdrowia. Program nadal działa zgodnie z oczekiwaniami.

Jeśli nie wykonasz sprawdzenia zerowego odwołania (lub nie wyślesz niesprawdzonego wyjątku), może to spowodować wyrzucenie niezaznaczonego NullReferenceException, co zwykle nie jest obsługiwane przez użytkownika metody, a nawet może spowodować zamknięcie aplikacji. Oznacza to, że funkcja jest oczywiście całkowicie niezrozumiana i dlatego aplikacja jest wadliwa.

Herr Derb
źródło
4

Coś w rodzaju rozszerzenia odpowiedzi @ null, ale z mojego doświadczenia wynika, że ​​sprawdzanie wartości null jest ważne bez względu na wszystko.

Dobre programowanie defensywne polega na tym, że kodujesz bieżącą procedurę osobno, przy założeniu, że cała reszta programu próbuje ją zawiesić. Kiedy piszesz krytyczny kod misji, w którym niepowodzenie nie jest możliwe, jest to właściwie jedyna droga.

To, jak faktycznie radzisz sobie z nullwartością, zależy w dużej mierze od przypadku użycia.

Jeśli nullwartość jest akceptowalna, np. Dowolny z parametrów od drugiego do piątego do procedury MSVC _splitpath (), wówczas zachowanie w ciszy jest poprawnym zachowaniem.

Jeśli nullnigdy nie powinno się to zdarzyć podczas wywoływania omawianej procedury, tj. W przypadku PO, umowa API wymaga, AssignEntity()aby została wywołana z ważnym, Entitya następnie rzucić wyjątek, lub w inny sposób nie zapewnić, że zrobisz dużo hałasu w procesie.

Nie martw się o koszt sprawdzenia zerowego, jeśli są, są tanie, a dzięki nowoczesnym kompilatorom analiza kodu statycznego może i pominie je całkowicie, jeśli kompilator stwierdzi, że tak się nie stanie.

dgnuff
źródło
Lub całkowicie go unikaj (48 minut 29 sekund: „Nigdy nie używaj ani nie zezwalaj w pobliżu programu na zerowy wskaźnik” ).
Peter Mortensen