Czy rozsądnie jest strzec zerowania każdego pojedynczego, pozbawionego odniesień wskaźnika?

Przy nowej pracy dostaję oznaczenie w recenzjach kodu dla kodu w następujący sposób:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender) { }

void PowerManager::SignalShutdown()
{
    msgSender_->sendMsg("shutdown()");
}

Powiedziano mi, że ostatnia metoda powinna brzmieć:

void PowerManager::SignalShutdown()
{
    if (msgSender_) {
        msgSender_->sendMsg("shutdown()");
    }
}

tzn. muszę ustawić NULLosłonę wokół msgSender_zmiennej, nawet jeśli jest ona prywatnym członkiem danych. Trudno mi powstrzymać się od użycia przekleństw, aby opisać, co sądzę o tym „mądrości”. Kiedy proszę o wyjaśnienie, dostaję litanię horrorów o tym, jak jakiś młodszy programista, pewnego roku, pomylił się co do tego, jak klasa powinna działać, i przypadkowo usunął członka, którego nie powinien był (i ustawił NULLpóźniej , najwyraźniej), a rzeczy wybuchły w terenie zaraz po wydaniu produktu, a my „nauczyliśmy się twardej drogi, zaufaj nam”, że lepiej po prostu wszystkoNULL sprawdzić .

Dla mnie jest to programowanie z kultowym ładunkiem , proste i proste. Kilku współpracowników, którzy mają dobre intencje, poważnie stara się pomóc mi „zdobyć” i zobaczyć, jak to pomoże mi napisać bardziej solidny kod, ale ... Nie mogę się powstrzymać od poczucia, że ​​to oni go nie rozumieją .

Czy rozsądne jest, aby standard kodowania wymagał, aby każdy wskaźnik odsunięty od funkcji w NULLpierwszej kolejności był sprawdzany w pierwszej kolejności - nawet dla prywatnych członków danych? (Uwaga: aby nadać kontekstu, tworzymy elektronikę użytkową, a nie system kontroli ruchu lotniczego lub inny produkt „brak równości ludzi”).

EDYCJA : W powyższym przykładzie msgSender_współpracownik nie jest opcjonalny. Jeśli kiedykolwiek NULL, oznacza to błąd. Jedynym powodem, dla którego jest przekazywany do konstruktora, jest PowerManagertestowanie IMsgSenderpodklasy próbnej.

STRESZCZENIE : Dziękuję wszystkim za naprawdę świetne odpowiedzi na to pytanie. Zaakceptowałem ten z @aaronps głównie ze względu na jego zwięzłość. Wydaje się, że istnieje ogólna zgoda co do tego, że:

1. Nakazanie NULLstrażnikom każdego odsuniętego wskaźnika jest przesadą, ale
2. Możesz przejść całą debatę na bok, używając zamiast tego odwołania (jeśli to możliwe) lub constwskaźnika i
3. assertinstrukcje są bardziej oświeconą alternatywą dla NULLstrażników do sprawdzania, czy spełnione są warunki wstępne funkcji.

To zależy od „umowy”:

Jeśli PowerManager MUSI mieć prawidłową wartość IMsgSender, nigdy nie sprawdzaj wartości zerowej, pozwól jej umrzeć wcześniej.

Jeśli z drugiej strony, MOGĄ mieć IMsgSender, to musisz sprawdzić za każdym razem, tak proste.

Końcowy komentarz na temat historii młodszego programisty, problemem jest w rzeczywistości brak procedur testowych.

Wydaje mi się, że kod powinien brzmieć:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender)
{
    assert(msgSender);
}

void PowerManager::SignalShutdown()
{
    assert(msgSender_);
    msgSender_->sendMsg("shutdown()");
}

Jest to w rzeczywistości lepsze niż pilnowanie NULL, ponieważ wyraźnie pokazuje, że funkcja nigdy nie powinna być wywoływana, jeśli msgSender_jest NULL. Daje również pewność, że zauważysz, jeśli tak się stanie.

Wspólna „mądrość” twoich kolegów po cichu zignoruje ten błąd, z nieprzewidywalnymi rezultatami.

Ogólnie błędy są łatwiejsze do naprawienia, jeśli zostaną wykryte bliżej ich przyczyny. W tym przykładzie proponowana ochrona NULL doprowadziłaby do braku ustawiania komunikatu zamknięcia, co może, ale nie musi, powodować zauważalny błąd. Trudniej byłoby pracować wstecz do tej SignalShutdownfunkcji, niż gdyby cała aplikacja właśnie umarła, tworząc poręczny elegancki ślad lub zrzut rdzenia wskazujący bezpośrednio na SignalShutdown().

Jest to trochę sprzeczne z intuicją, ale zawieszanie się, gdy tylko coś jest nie tak, sprawia, że ​​kod jest bardziej niezawodny. Wynika to z faktu, że faktycznie znajdują się problemy i mają one również bardzo oczywiste przyczyny.

Jeśli msgSender nie musi być null, należy umieścić nullsprawdzanie tylko w konstruktorze. Odnosi się to również do wszelkich innych danych wejściowych do klasy - umieść kontrolę integralności w punkcie wejścia do „modułu” - klasa, funkcja itp.

Moją ogólną zasadą jest sprawdzanie integralności między granicami modułów - w tym przypadku klasą. Ponadto klasa powinna być na tyle mała, aby możliwe było szybkie mentalne zweryfikowanie integralności czasów życia członków klasy - zapewniając, że zapobiegnie się takim błędom, jak niewłaściwe usuwanie / zerowanie przypisań. Kontrola zerowa przeprowadzana w kodzie w poście zakłada, że ​​każde nieprawidłowe użycie faktycznie przypisuje null do wskaźnika - co nie zawsze ma miejsce. Ponieważ „nieprawidłowe użycie” z natury oznacza, że ​​nie mają zastosowania żadne założenia dotyczące zwykłego kodu, nie możemy być pewni, że wychwycimy wszystkie rodzaje błędów wskaźnika - na przykład nieprawidłowe usunięcie, przyrost itp.

Ponadto - jeśli masz pewność, że argument nigdy nie może być pusty, rozważ użycie referencji, w zależności od użycia klasy. W przeciwnym razie rozważ użycie surowego wskaźnika std::unique_ptrlub std::shared_ptrzamiast niego.

Nie, sprawdzanie każdego odwołania do wskaźnika dla istoty wskaźnika nie jest rozsądne NULL.

Kontrole zerowe są cenne dla argumentów funkcji (w tym argumentów konstruktora), aby zapewnić spełnienie warunków wstępnych lub podjąć odpowiednie działania, jeśli nie podano opcjonalnego parametru, i są one przydatne do sprawdzenia niezmiennika klasy po odsłonięciu wewnętrznych elementów klasy. Ale jeśli jedynym powodem, dla którego wskaźnik ma wartość NULL, jest istnienie błędu, wówczas sprawdzanie nie ma sensu. Ten błąd równie łatwo mógł ustawić wskaźnik na inną niepoprawną wartość.

Gdybym miał do czynienia z taką sytuacją, zadałbym dwa pytania:

• Dlaczego błąd tego młodszego programisty nie został wykryty przed wydaniem? Na przykład podczas przeglądu kodu lub w fazie testowania? Wprowadzenie wadliwego urządzenia elektroniki użytkowej na rynek może być równie kosztowne (jeśli weźmie się pod uwagę udział w rynku i dobrą wolę), jak wydanie wadliwego urządzenia używanego w krytycznych dla bezpieczeństwa aplikacjach, więc oczekiwałbym, że firma poważnie podchodzi do testowania i inne działania związane z kontrolą jakości.
• Jeśli sprawdzanie wartości zerowej nie powiedzie się, jakiej obsługi błędów oczekuje się ode mnie, czy mógłbym po prostu napisać assert(msgSender_)zamiast kontroli wartości zerowej? Jeśli po prostu włączysz sprawdzanie wartości zerowej, mógłbyś zapobiec awarii, ale mogłeś stworzyć gorszą sytuację, ponieważ oprogramowanie kontynuuje działanie przy założeniu, że operacja miała miejsce, podczas gdy w rzeczywistości operacja ta została pominięta. Może to spowodować niestabilność innych części oprogramowania.

Ten przykład wydaje się bardziej dotyczyć czasu życia obiektu niż tego, czy parametr wejściowy ma wartość null †. Ponieważ wspominasz, że zawszePowerManager musi mieć poprawny , przekazanie argumentu przez wskaźnik (umożliwiając w ten sposób możliwość zerowego wskaźnika) wydaje mi się błędem projektowym ††.IMsgSender

W takich sytuacjach wolałbym zmienić interfejs, aby wymagania osoby dzwoniącej były egzekwowane przez język:

PowerManager::PowerManager(const IMsgSender& msgSender)
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    msgSender_->sendMsg("shutdown()");
}

Przepisanie tego w ten sposób mówi, że PowerManagernależy zachować odniesienie IMsgSenderprzez cały okres jego istnienia. To z kolei ustanawia również dorozumiany wymóg, który IMsgSendermusi istnieć dłużej niż PowerManager, i neguje potrzebę jakiegokolwiek sprawdzania wskaźnika zerowego lub stwierdzeń w środku PowerManager.

Możesz również napisać to samo za pomocą inteligentnego wskaźnika (poprzez boost lub c ++ 11), aby jawnie wymusić IMsgSenderżycie dłuższe niż PowerManager:

PowerManager::PowerManager(std::shared_ptr<IMsgSender> msgSender) 
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    // Here, we own a smart pointer to IMsgSender, so even if the caller
    // destroys the original pointer, we still have a valid copy
    msgSender_->sendMsg("shutdown()");
}

Ta metoda jest preferowana, jeśli jest możliwe, że IMsgSendernie można zagwarantować, że jej żywotność będzie dłuższa niż PowerManager(tj x = new IMsgSender(); p = new PowerManager(*x);.).

† W odniesieniu do wskaźników: gwałtowne sprawdzanie wartości null sprawia, że ​​kod jest trudniejszy do odczytania i nie poprawia stabilności (poprawia wygląd stabilności, co jest znacznie gorsze).

Gdzieś ktoś ma adres do przechowywania pamięci IMsgSender. Zadaniem tej funkcji jest upewnienie się, że alokacja się powiodła (sprawdzenie wartości zwracanych przez bibliotekę lub poprawna obsługa std::bad_allocwyjątków), aby nie przekazywać niepoprawnych wskaźników.

Ponieważ PowerManagernie jest właścicielem IMsgSender(tylko pożycza go na jakiś czas), nie jest odpowiedzialny za przydzielanie ani niszczenie tej pamięci. To kolejny powód, dla którego wolę referencję.

†† Ponieważ jesteś nowy w tej pracy, oczekuję, że włamujesz się do istniejącego kodu. Więc z wady projektowej mam na myśli, że wada jest w kodzie, z którym pracujesz. Dlatego osoby, które oflagują Twój kod, ponieważ nie sprawdza zerowych wskaźników, tak naprawdę same się zgłaszają do pisania kodu, który wymaga wskaźników :)

Podobnie jak wyjątki, warunki ochrony są użyteczne tylko wtedy, gdy wiesz, co zrobić, aby naprawić błąd lub jeśli chcesz przekazać bardziej znaczący komunikat wyjątku.

Połknięcie błędu (bez względu na to, czy został złapany jako wyjątek, czy kontrola bezpieczeństwa) jest właściwą rzeczą, gdy błąd nie ma znaczenia. Najczęstsze miejsce, w którym widzę błędy podczas połykania, to kod rejestrowania błędów - nie chcesz zawieszać aplikacji, ponieważ nie możesz zalogować się do komunikatu o stanie.

Za każdym razem, gdy wywoływana jest funkcja i nie jest to zachowanie opcjonalne, powinna zawieść głośno, nie po cichu.

Edycja: myśląc o twojej historii młodszego programisty, brzmi to tak, jakby zdarzyło się, że prywatny członek został ustawiony na zero, kiedy to nigdy nie powinno było mieć miejsca. Mieli problem z niewłaściwym pisaniem i próbują to naprawić, sprawdzając poprawność podczas czytania. To jest wstecz. Zanim go zidentyfikujesz, błąd już się pojawił. Egzekwowalne rozwiązanie do sprawdzania kodu / kompilatora nie stanowi warunków ochrony, ale pobiera i ustawia lub członków stałych.

Jak zauważyli inni, zależy to od tego, czy msgSendermoże być legalne NULL . Poniżej założono, że nigdy nie powinno być NULL.

void PowerManager::SignalShutdown()
{
    if (!msgSender_)
    {
       throw SignalException("Shut down failed because message sender is not set.");
    }

    msgSender_->sendMsg("shutdown()");
}

Proponowana „poprawka” przez innych członków zespołu narusza zasadę Dead Programs Tell No Lies . Błędy są naprawdę trudne do znalezienia. Metoda, która dyskretnie zmienia swoje zachowanie na podstawie wcześniejszego problemu, nie tylko utrudnia znalezienie pierwszego błędu, ale także dodaje drugi błąd.

Młodzieniec siał spustoszenie, nie sprawdzając, czy ma wartość zerową. Co się stanie, jeśli ten fragment kodu spowoduje spustoszenie, kontynuując działanie w nieokreślonym stanie (urządzenie jest włączone, ale program „myśli”, że jest wyłączone)? Być może inna część programu zrobi coś, co jest bezpieczne tylko wtedy, gdy urządzenie jest wyłączone.

Każde z tych podejść pozwoli uniknąć cichych awarii:

1. Używaj asercji zgodnie z sugestią podaną w tej odpowiedzi , ale upewnij się, że są włączone w kodzie produkcyjnym. Mogłoby to oczywiście powodować problemy, gdyby inne twierdzenia zostały napisane przy założeniu, że zostaną wyłączone z produkcji.

2. Zgłaszaj wyjątek, jeśli jest on pusty.

Zgadzam się z zatrzymywaniem wartości null w konstruktorze. Ponadto, jeśli element członkowski jest zadeklarowany w nagłówku jako:

IMsgSender* const msgSender_;

Wówczas wskaźnika nie można zmienić po inicjalizacji, więc jeśli był w porządku podczas budowy, będzie w porządku przez cały okres życia obiektu zawierającego go. (Obiekt zwrócił się będzie nie być const).

To jest całkowicie niebezpieczne!

Pracowałem pod kierownictwem starszego programisty w bazie kodu C z najbardziej podejrzanymi „standardami”, którzy nalegali na to samo, aby ślepo sprawdzać wszystkie wskaźniki pod kątem wartości zerowej. Deweloper ostatecznie zrobiłby takie rzeczy:

// Pre: vertex should never be null.
void transform_vertex(Vertex* vertex, ...)
{
    // Inserted by my "wise" co-worker.
    if (!vertex)
        return;
    ...
}

Kiedyś próbowałem usunąć taką kontrolę warunku raz w takiej funkcji i zastąpić ją, assertaby zobaczyć, co się stanie.

Ku mojemu przerażeniu znalazłem tysiące linii kodu w bazie kodu, które przekazywały wartości zerowe do tej funkcji, ale gdzie programiści, prawdopodobnie zdezorientowani, pracowali i dodawali więcej kodu, aż wszystko zadziałało.

Ku mojemu dalszemu przerażeniu odkryłem, że problem ten występuje we wszystkich miejscach w bazie kodów, sprawdzając, czy nie ma żadnych wartości zerowych. Baza kodów rozrosła się przez dziesięciolecia, aby polegać na tych kontrolach, aby móc po cichu naruszać nawet najbardziej udokumentowane warunki wstępne. Usuwając te śmiertelne kontrole na korzyść asserts, ujawnią się wszystkie logiczne błędy ludzkie w ciągu dziesięcioleci w bazie kodu, a my się w nich utopimy.

Potrzebne były tylko dwa pozornie niewinne wiersze kodu, takie jak ten + czas, i zespół, aby zamaskować tysiąc nagromadzonych błędów.

Są to rodzaje praktyk, które uzależniają błędy od innych błędów, aby oprogramowanie działało. To scenariusz koszmaru . Sprawia również, że każdy błąd logiczny związany z naruszeniem takich warunków wstępnych pokazuje tajemniczo milion wierszy kodu z dala od rzeczywistej strony, w której wystąpił błąd, ponieważ wszystkie te testy zerowe po prostu ukrywają błąd i ukrywają go, aż dotrzemy do miejsca, które zapomniało aby ukryć błąd.

Po prostu ślepe sprawdzanie wartości zerowych w każdym miejscu, w którym wskaźnik zerowy narusza warunek wstępny, jest dla mnie całkowitym szaleństwem, chyba że twoje oprogramowanie jest tak krytyczne pod względem misji w przypadku stwierdzeń i awarii produkcyjnych, że potencjał tego scenariusza jest lepszy.

Czy rozsądne jest, aby standard kodowania wymagał, aby każdy wskaźnik odsunięty od funkcji w pierwszej kolejności był sprawdzany pod kątem NULL - nawet prywatnych członków danych?

Więc powiedziałbym, absolutnie nie. To nawet nie jest „bezpieczne”. Może być odwrotnie i maskować wszelkiego rodzaju błędy w bazie kodu, które z biegiem lat mogą prowadzić do najbardziej przerażających scenariuszy.

assertjest drogą do przejścia tutaj. Naruszenie warunków wstępnych nie powinno pozostać niezauważone, w przeciwnym razie prawo Murphy'ego może się łatwo rozpocząć.

Na przykład Objective-C traktuje każde wywołanie metody nilobiektu jako no-op, którego wartość jest zerowa. Istnieje kilka zalet tej decyzji projektowej w Objective-C, z powodów sugerowanych w twoim pytaniu. Teoretyczna koncepcja ochrony zerowej przed każdym wywołaniem metody ma swoje zalety, jeśli jest dobrze nagłośniona i konsekwentnie stosowana.

To powiedziawszy, kod żyje w ekosystemie, a nie próżni. Zachowane z zachowaniem zerowej ochrony byłoby w C ++ nie-idiomatyczne i zaskakujące , dlatego powinno być uważane za szkodliwe. Podsumowując, nikt nie pisze w ten sposób kodu C ++, więc nie rób tego! Jako kontrprzykład zauważ jednak, że wywołanie free()lub deletepołączenie NULLw C i C ++ jest gwarantowane, że nie działa.

W twoim przykładzie prawdopodobnie warto umieścić w konstruktorze twierdzenie, które msgSendernie ma wartości null. Jeśli konstruktor nazywa metodę na msgSenderod razu, to nie takie twierdzenie byłoby konieczne, gdyż to właśnie tam w każdym razie awarii. Ponieważ jednak jest on tylko przechowywany msgSender do wykorzystania w przyszłości, nie byłoby oczywiste, biorąc pod uwagę ślad stosu, SignalShutdown()jak powstała wartość NULL, więc zapewnienie w konstruktorze znacznie ułatwi debugowanie.

Co więcej, konstruktor powinien zaakceptować const IMsgSender&odwołanie, które nie może być NULL.

Powodem, dla którego jesteś proszony o unikanie zerowych dereferencji, jest upewnienie się, że kod jest niezawodny. Przykłady młodszych programistów dawno temu są tylko przykładami. Każdy może złamać kod przez przypadek i spowodować zerowe odwołanie - szczególnie w przypadku globali i globali klasowych. W C i C ++ jest to jeszcze bardziej możliwe przypadkowo, dzięki możliwości bezpośredniego zarządzania pamięcią. Możesz być zaskoczony, ale takie rzeczy zdarzają się bardzo często. Nawet przez bardzo kompetentnych, bardzo doświadczonych i bardzo starszych programistów.

Nie musisz zerować wszystkiego, ale musisz chronić się przed dereferencjami, które mają przyzwoite prawdopodobieństwo bycia zerowym. Dzieje się tak zwykle, gdy są one przydzielane, używane i usuwane z funkcji w różnych funkcjach. Możliwe, że jedna z pozostałych funkcji zostanie zmodyfikowana i zepsuje twoją funkcję. Możliwe jest również, że jedna z pozostałych funkcji może zostać wywołana nie w porządku (tak jak w przypadku dealokatora, który można wywołać oddzielnie od destruktora).

Wolę podejście, które mówią ci współpracownicy w połączeniu z używaniem asercji. Awaria w środowisku testowym, więc bardziej oczywiste jest, że istnieje problem do naprawienia i bezproblemowo podczas produkcji.

Powinieneś także używać niezawodnego narzędzia do poprawiania kodu, takiego jak coverity lub fortyfikacja. I powinieneś zająć się wszystkimi ostrzeżeniami kompilatora.

Edycja: jak wspomnieli inni, cicha awaria, jak w kilku przykładach kodu, jest również ogólnie rzeczą niewłaściwą. Jeśli twoja funkcja nie może odzyskać wartości zerowej, powinna zwrócić błąd (lub zgłosić wyjątek) do swojego obiektu wywołującego. Osoba dzwoniąca jest odpowiedzialna za ustalenie kolejności połączeń, odzyskanie lub zwrócenie błędu (lub zgłoszenie wyjątku) do osoby dzwoniącej i tak dalej. W końcu albo funkcja jest w stanie z wdziękiem odzyskać i przejść dalej, z wdziękiem odzyskać i zakończyć się niepowodzeniem (na przykład baza danych nie powiedzie się w transakcji z powodu błędu wewnętrznego dla jednego użytkownika, ale nie kończy się automatycznie), lub funkcja ustali, że stan aplikacji jest uszkodzony i nie można go odzyskać, a aplikacja kończy działanie.

Użycie wskaźnika zamiast odwołania powiedziałoby mi, że msgSenderjest to tylko opcja i tutaj sprawdzanie wartości null byłoby prawidłowe. Fragment kodu jest zbyt wolny, aby na to zdecydować. Może są inne elementy, PowerManagerktóre są cenne (lub testowalne) ...

Wybierając pomiędzy wskaźnikiem a odniesieniem, dokładnie ważę obie opcje. Jeśli muszę użyć wskaźnika dla członka (nawet dla członków prywatnych), muszę zaakceptować tę if (x)procedurę za każdym razem, gdy go odłożę.

To zależy od tego, co chcesz się wydarzyć.

Napisałeś, że pracujesz nad „urządzeniem elektroniki użytkowej”. Jeśli jakiś błąd zostanie wprowadzony przez ustawienie msgSender_na NULL, chcesz

• urządzenie do kontynuowania, pomijanie, SignalShutdownale kontynuowanie reszty działania, lub
• urządzenie ulega awarii, zmuszając użytkownika do ponownego uruchomienia?

W zależności od wpływu, że sygnał wyłączenia nie jest wysyłany, opcja 1 może być dobrym wyborem. Jeśli użytkownik może nadal słuchać swojej muzyki, ale wyświetlacz nadal pokazuje tytuł poprzedniego utworu, może to być lepsze niż całkowite uszkodzenie urządzenia.

Oczywiście, jeśli wybierzesz opcję 1, assert(zalecane przez innych) jest niezbędne, aby zmniejszyć prawdopodobieństwo, że taki błąd wkradnie się niezauważony podczas programowania. Zabezpieczenie ifzerowe jest właśnie dostępne w celu ograniczenia awarii w zastosowaniach produkcyjnych.

Osobiście wolę też podejście „wczesne awarie” dla kompilacji produkcyjnych, ale rozwijam oprogramowanie biznesowe, które można łatwo naprawić i zaktualizować w przypadku błędu. W przypadku urządzeń elektroniki użytkowej może to nie być takie proste.