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ć NULL
osł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ł NULL
póź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 NULL
pierwszej 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 PowerManager
testowanie IMsgSender
podklasy 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:
- Nakazanie
NULL
strażnikom każdego odsuniętego wskaźnika jest przesadą, ale - Możesz przejść całą debatę na bok, używając zamiast tego odwołania (jeśli to możliwe) lub
const
wskaźnika i assert
instrukcje są bardziej oświeconą alternatywą dlaNULL
strażników do sprawdzania, czy spełnione są warunki wstępne funkcji.
źródło
null
i nic nie robienie jest tylko sposobem na przeniesienie błędu w dół wykonania, co znacznie utrudnia powrót do źródła.Odpowiedzi:
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.
źródło
assert()
w każdym elemencie, która nie uwzględnia wskaźnika, jest kompromisem, który prawdopodobnie uspokoi wiele osób, ale nawet to wydaje mi się „spekulatywną paranoją”. Lista rzeczy, nad którymi nie mam kontroli, jest nieskończona, a kiedy pozwolę sobie zacząć się martwić o nie, staje się naprawdę śliska. Nauka zaufania do moich testów, moi koledzy - i mój debugger - pomogli mi lepiej spać w nocy.Wydaje mi się, że kod powinien brzmieć:
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
SignalShutdown
funkcji, niż gdyby cała aplikacja właśnie umarła, tworząc poręczny elegancki ślad lub zrzut rdzenia wskazujący bezpośrednio naSignalShutdown()
.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.
źródło
Jeśli msgSender nie musi być
null
, należy umieścićnull
sprawdzanie 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_ptr
lubstd::shared_ptr
zamiast niego.źródło
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:
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.źródło
Ten przykład wydaje się bardziej dotyczyć czasu życia obiektu niż tego, czy parametr wejściowy ma wartość null †. Ponieważ wspominasz, że zawsze
PowerManager
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:
Przepisanie tego w ten sposób mówi, że
PowerManager
należy zachować odniesienieIMsgSender
przez cały okres jego istnienia. To z kolei ustanawia również dorozumiany wymóg, któryIMsgSender
musi istnieć dłużej niżPowerManager
, i neguje potrzebę jakiegokolwiek sprawdzania wskaźnika zerowego lub stwierdzeń w środkuPowerManager
.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
:Ta metoda jest preferowana, jeśli jest możliwe, że
IMsgSender
nie można zagwarantować, że jej żywotność będzie dłuższa niżPowerManager
(tjx = 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ługastd::bad_alloc
wyjątków), aby nie przekazywać niepoprawnych wskaźników.Ponieważ
PowerManager
nie jest właścicielemIMsgSender
(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 :)
źródło
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.
źródło
Jak zauważyli inni, zależy to od tego, czy
msgSender
może być legalneNULL
. Poniżej założono, że nigdy nie powinno być NULL.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:
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.
Zgłaszaj wyjątek, jeśli jest on pusty.
źródło
Zgadzam się z zatrzymywaniem wartości null w konstruktorze. Ponadto, jeśli element członkowski jest zadeklarowany w nagłówku jako:
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).
źródło
NULL
.const
wskaźnikiem nie ma potrzebyassert()
wychodzenia poza konstruktor, więc ta odpowiedź wydaje się nawet nieco lepsza niż @Kristof Provost (który był również wybitny, ale rozwiązał to pytanie również pośrednio). Mam nadzieję, że inni głosują na to, ponieważ to naprawdę nie ma sensuassert
w każdej metodzie, kiedy można po prostu zrobić wskaźnikconst
.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:
Kiedyś próbowałem usunąć taką kontrolę warunku raz w takiej funkcji i zastąpić ją,
assert
aby 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.
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.
assert
jest 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ąć.źródło
assert
zarówno jako mechanizm dokumentacji, aby ustalić, jakie są warunki wstępne, a nie tylko sposób na wymuszenie przerwania. Ale podoba mi się również rodzaj błędu asercji, który pokazuje dokładnie, który wiersz kodu spowodował awarię, a warunek asercji nie powiódł się („dokumentowanie awarii”). Może się przydać, jeśli w końcu użyjemy kompilacji debugowania poza debuggerem i zostaniemy zaskoczeni.Na przykład Objective-C traktuje każde wywołanie metody
nil
obiektu 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()
lubdelete
połączenieNULL
w C i C ++ jest gwarantowane, że nie działa.W twoim przykładzie prawdopodobnie warto umieścić w konstruktorze twierdzenie, które
msgSender
nie ma wartości null. Jeśli konstruktor nazywa metodę namsgSender
od razu, to nie takie twierdzenie byłoby konieczne, gdyż to właśnie tam w każdym razie awarii. Ponieważ jednak jest on tylko przechowywanymsgSender
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
.źródło
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.
źródło
NULL
to sposób, aby to zrobić, ale doceniam usłyszenie opinii kogoś spoza mojego miejsca pracy, który w zasadzie mówi: „Aha. Nie wydaje się to zbyt nierozsądne”.Użycie wskaźnika zamiast odwołania powiedziałoby mi, że
msgSender
jest 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,PowerManager
któ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żę.źródł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_
naNULL
, chceszSignalShutdown
ale kontynuowanie reszty działania, lubW 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. Zabezpieczenieif
zerowe 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.
źródło