Jak zaimplementować właściwość w klasie A, która odnosi się do właściwości obiektu potomnego klasy A.

9

Mamy ten kod, który po uproszczeniu wygląda następująco:

public class Room
{
    public Client Client { get; set; }

    public long ClientId
    {
        get
        {
            return Client == null ? 0 : Client.Id;
        }
    }
}

public class Client 
{
    public long Id { get; set; }
}

Teraz mamy trzy punkty widzenia.

1) To jest dobry kod, ponieważ Clientwłaściwość powinna być zawsze ustawiona (tj. Nie pusta), więc Client == nullnigdy się nie pojawi, a wartość 0Id i tak oznacza fałszywy identyfikator (taka jest opinia autora kodu ;-))

2) Nie można powoływać się na rozmówcy, aby wiedzieć, że 0jest fałszywa wartość Id, a gdy Clientnieruchomość powinna być zawsze należy rzucać exceptionw getgdy Clientnieruchomość zdarza się być null

3) Gdy Clientwłaściwość powinna być zawsze ustawiona, po prostu powracasz Client.Idi pozwalasz kodowi zgłosić NullRefwyjątek, gdy Clientwłaściwość jest zerowa.

Który z nich jest najbardziej poprawny? Czy jest czwarta możliwość?

Michel
źródło
5
Nie odpowiedź, ale tylko uwaga: nigdy nie rzucaj wyjątków od podmiotów zajmujących się nieruchomościami.
Brandon
3
Aby rozwinąć kwestię Brandona: stackoverflow.com/a/1488488/569777
MetaFight
1
Pewnie: Ogólnym pomysłem ( między innymi msdn.microsoft.com/en-us/library/... , stackoverflow.com/questions/1488472 /...) jest to, że właściwości powinny robić tak mało, jak to możliwe, być lekkie i reprezentować czyste , publiczny stan obiektu, z indeksatorami stanowi niewielki wyjątek. Nieoczekiwane jest również wyświetlanie wyjątków w oknie podglądu właściwości podczas debugowania.
Brandon
1
Nie powinieneś (trzeba) pisać kodu, który wrzuca moduł pobierania właściwości. Nie oznacza to, że musisz ukryć i przełknąć wszelkie wyjątki, które występują z powodu przejścia obiektu w nieobsługiwany stan.
Ben
4
Dlaczego nie możesz po prostu zrobić Room.Client.Id? Dlaczego chcesz zawinąć to w Room.ClientId? Jeśli ma sprawdzić klienta, to dlaczego nie coś takiego jak Room.HasClient {get {return client == null; }} to jest prostsze i nadal pozwala ludziom robić normalne Room.Client.Id, kiedy faktycznie potrzebują identyfikatora?
James

Odpowiedzi:

25

Pachnie, że powinieneś ograniczyć liczbę stanów, w których Roommoże znajdować się klasa.

Sam fakt, że pytasz o to, co zrobić, gdy Clientjest pusty, jest wskazówką, że Roomprzestrzeń stanu jest zbyt duża.

Aby uprościć sprawę, nie pozwoliłbym, aby Clientwłasność jakiejkolwiek Roominstancji była zawsze pusta. Oznacza to, że kod wewnątrz Roommoże bezpiecznie założyć, że Clientnigdy nie jest zerowy.

Jeśli z jakiegoś powodu w przyszłości Clientstanie nulloprzeć się pokusie, aby wspierać ten stan. Takie postępowanie zwiększy złożoność konserwacji.

Zamiast tego pozwól, aby kod zawiódł i szybko zawiódł. W końcu nie jest to obsługiwany stan. Jeśli aplikacja przejdzie w ten stan, przekroczyłeś już linię bez powrotu. Jedyną rozsądną rzeczą do zrobienia jest zamknięcie aplikacji.

Może się to zdarzyć (całkiem naturalnie) w wyniku nieobsługiwanego wyjątku odwołania zerowego.

MetaFight
źródło
2
Miły. Podoba mi się myśl „Aby uprościć sprawę, nie pozwolę, aby właściwość klienta dowolnej instancji pokoju była zawsze pusta.” To jest rzeczywiście lepsze niż kwestionowanie, co zrobić, gdy jest ona pusta
Michel,
@Michel, jeśli później zdecydujesz się zmienić to wymaganie, możesz zastąpić nullwartość NullObject(lub raczej NullClient), która nadal działałaby z istniejącym kodem i pozwoliła zdefiniować zachowanie nieistniejącego klienta, jeśli zajdzie taka potrzeba. Pytanie brzmi, czy przypadek „braku klienta” jest częścią logiki biznesowej, czy nie.
null
3
Całkowicie poprawne. Nie pisz ani jednego znaku kodu w celu obsługi nieobsługiwanego stanu. Po prostu pozwól mu rzucić NullRef.
Ben
@Ben Nie zgadzam się; Wytyczne MS wyraźnie stwierdzają, że podmioty pobierające właściwości nie powinny zgłaszać wyjątków. A zezwolenie na wyrzucenie referencji zerowej, gdy można zamiast tego rzucić bardziej znaczący wyjątek, jest po prostu leniwe.
Andy,
1
@ Zrozumienie powodu wytycznej pozwoli ci wiedzieć, kiedy nie ma ona zastosowania.
Ben
21

Kilka uwag:

a) Dlaczego istnieje getter specjalnie dla ClientId, skoro już istnieje publiczny getter dla samej instancji Client? Nie rozumiem, dlaczego informacja, że ​​ClientId jest, longmusi być wyryta w kamieniu w podpisie Pokoju.

b) Jeśli chodzi o drugą opinię, możesz wprowadzić stałą Invalid_Client_Id.

c) Jeśli chodzi o opinię pierwszą i trzecią (i co jest moim głównym punktem): pokój musi zawsze mieć klienta? Może to tylko semantyka, ale to nie brzmi dobrze. Być może bardziej odpowiednie byłoby posiadanie całkowicie oddzielnych klas dla Pokoju i Klienta oraz innej klasy, która je łączy. Może spotkanie, rezerwacja, obłożenie? (Zależy to od tego, co faktycznie robisz.) I na tej klasie możesz egzekwować ograniczenia „musi mieć pokój” i „musi mieć klienta”.

VolkerK
źródło
5
+1 za „Nie rozumiem np. Dlaczego informacja, że ​​ClientId jest długi, musi zostać wyryta w kamieniu w sygnaturze Pokoju”
Michel
Twój angielski jest dobry. ;) Już po przeczytaniu tej odpowiedzi nie wiedziałbym, że twoim ojczystym językiem nie jest angielski bez twojej odpowiedzi.
jpmc26
Punkty B i C są dobre; RE: a, jest to wygodna metoda, a fakt, że pokój ma odniesienie do klienta, może być jedynie szczegółem implementacji (można argumentować, że klient nie powinien być publicznie widoczny)
Andy
17

Nie zgadzam się ze wszystkimi trzema opiniami. Jeśli Clientnigdy nie może być null, to nawet nie pozwól, aby tak byłonull !

  1. Ustaw wartość Clientkonstruktora
  2. Wrzuć ArgumentNullExceptiondo konstruktora.

Więc twój kod byłby mniej więcej taki:

public class Room
{
    private Client theClient;

    public Room(Client client) {
        if(client == null) throw new ArgumentNullException();
        this.theClient = client;
    }

    public Client Client { 
        get { return theClient; }
        set 
        {
            if (value == null) 
                throw new ArgumentNullException();
            theClient = value;
        }
    }

    public long ClientId
    {
        get
        {
            return theClient.Id;
        }
    }
}
public class Client 
{
    public long Id { get; set; }
}

Nie ma to związku z twoim kodem, ale prawdopodobnie lepiej byłoby użyć niezmiennej wersji Room, robiąc theClient readonly, a następnie, jeśli klient się zmieni, tworząc nowy pokój. Zwiększy to bezpieczeństwo wątków w kodzie oprócz bezpieczeństwa zerowego innych aspektów mojej odpowiedzi. Zobacz tę dyskusję na temat zmiennych i niezmiennych

durron597
źródło
1
Czy nie powinny to być ArgumentNullException?
Mathieu Guindon,
4
Nie. Kod programu nigdy nie powinien wyrzucać a NullReferenceException, jest on generowany przez maszynę wirtualną .Net „, gdy następuje próba wyłuskiwania odwołania do obiektu zerowego” . Powinieneś rzucać an ArgumentNullException, który jest wyrzucany „, gdy odwołanie zerowe (Nic w Visual Basic) jest przekazywane do metody, która nie przyjmuje go jako prawidłowego argumentu”.
Pharap,
2
@Pharap Jestem programistą Java, który próbuje napisać kod C #, pomyślałem, że popełniam takie błędy.
durron597,
@ durron597 Powinienem odgadnąć, że na podstawie terminu „końcowy” (w tym przypadku jest to odpowiednie słowo kluczowe C # readonly). Właśnie edytowałbym, ale czułem, że komentarz mógłby lepiej wyjaśnić, dlaczego (dla przyszłych czytelników). Jeśli jeszcze nie udzieliłeś tej odpowiedzi, podałbym dokładnie to samo rozwiązanie. Niezmienność jest również dobrym pomysłem, ale nie zawsze jest to tak łatwe, jak się ma nadzieję.
Pharap,
2
Właściwości nie powinny generować wyjątków; zamiast metody ustawiającej zgłaszającej wyjątek ArgumentNullException, zamiast tego podaj metodę SetClient. Ktoś opublikował link do odpowiedzi na to pytanie.
Andy,
5

Należy unikać drugiej i trzeciej opcji - pobierający nie powinien uderzać dzwoniącego, z wyjątkiem tego, że nie ma nad nimi kontroli.

Powinieneś zdecydować, czy Klient może być kiedykolwiek zerowy. Jeśli tak, powinieneś umożliwić dzwoniącemu sprawdzenie, czy ma on wartość NULL, zanim uzyska do niego dostęp (np. Właściwość bool ClientIsNull).

Jeśli zdecydujesz, że Klient nigdy nie może mieć wartości NULL, ustaw go jako wymagany parametr dla konstruktora i wyślij tam wyjątek, jeśli zostanie przekazane NULL.

Wreszcie pierwsza opcja zawiera także zapach kodu. Powinieneś pozwolić Klientowi postępować z jego własną własnością ID. Wydaje się, że przesada koduje moduł pobierający w klasie kontenera, który po prostu wywołuje moduł pobierający w klasie zawartej. Po prostu ujawnij klienta jako właściwość (w przeciwnym razie skończysz duplikowaniem wszystkiego, co klient już oferuje ).

long clientId = room.Client.Id;

Jeśli Klient może mieć wartość zerową, wówczas przynajmniej ponosisz odpowiedzialność za dzwoniącego:

if (room.Client != null){
    long clientId = room.Client.Id;
    /* other code follows... */
}
Kent A.
źródło
1
Myślę, że „skończysz powielaniem wszystkiego, co klient już oferuje”, powinno być pogrubione lub przynajmniej kursywą.
Pharap,
2

Jeśli właściwość null Client jest obsługiwanym stanem, rozważ użycie NullObject .

Ale najprawdopodobniej jest to wyjątkowy stan, więc powinieneś uniemożliwić (lub po prostu nie bardzo wygodne) skończyć na zero Client:

public Client Client { get; private set; }

public Room(Client client)
{
    Client = client;
}

public void SetClient(Client client)
{
    if (client == null) throw new ArgumentNullException();
    Client = client;
}

Jeśli nie jest to obsługiwane, nie trać czasu na półpieczone roztwory. W tym przypadku:

return Client == null ? 0 : Client.Id;

„Rozwiązałeś” wyjątek NullReferenceEx tutaj, ale za wielką cenę! Zmusiłoby to wszystkich dzwoniących Room.ClientIddo sprawdzenia 0:

var aRoom = new Room(aClient);
var clientId = aRoom.ClientId;
if (clientId == 0) RestartRoombookingWizard();
else ContinueRoombooking(clientid);

Dziwne błędy mogą powstać w przypadku innych programistów (lub ciebie!), Zapominając o sprawdzeniu wartości zwracanej „ErrorCode” w pewnym momencie.
Graj bezpiecznie i szybko się nie powiedzie. Zezwól na zgłoszenie wyjątku NullReferenceException i „z wdziękiem” złap go gdzieś wyżej na stosie wywołań, zamiast pozwolić dzwoniącemu na jeszcze większy bałagan…

Z drugiej strony, jeśli tak bardzo chcesz ujawnić, Clienta także ClientIdpomyśleć o TellDontAsk . Jeśli poprosisz zbyt wiele o przedmioty zamiast powiedzieć im, co chcesz osiągnąć, możesz zakończyć łączenie wszystkiego, co utrudni później zmianę.

Laoujin
źródło
To NotSupportedExceptionjest źle wykorzystywane, powinieneś użyć ArgumentNullExceptionzamiast tego.
Pharap,
1

Począwszy od wersji c # 6.0, która jest już wydana, powinieneś to zrobić,

public class Room
{
    public Room(Client client)
    {
        this.Client = client;
    }

    public Client Client { get; }
}

public class Client
{
    public Client(long id)
    {
        this.Id = id;
    }

    public long Id { get; }
}

Podnoszenie własności Clientdo Roomjest oczywistym naruszeniem enkapsulacji i zasady DRY.

Jeśli chcesz uzyskać dostęp do Idjednego Clientz Roomnich,

var clientId = room.Client?.Id;

Uwaga korzystanie z Null Operator warunkowy , clientIdbędzie long?, jeśli Clientjest null, clientIdbędzie null, w przeciwnym razie clientIdbędzie miał wartość Client.Id.

Jodrell
źródło
ale taki rodzaj clientidjest długi?
Michel,
@Michel cóż, jeśli pokój musi mieć klienta, zaznacz pole zerowe w ctor. Wtedy var clientId = room.Client.Idbędzie zarówno bezpieczny, jak i long.
Jodrell,