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

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)?

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:

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

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.

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?

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.

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.

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.

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.