Zgłaszaj wyjątek lub pozwól kodowi zawieść

Zastanawiam się, czy są jakieś plusy i minusy przeciwko temu stylowi:

private void LoadMaterial(string name)
{
    if (_Materials.ContainsKey(name))
    {
        throw new ArgumentException("The material named " + name + " has already been loaded.");
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );
}

Ta metoda powinna być dla każdego nameuruchomiona tylko raz. _Materials.Add()wyrzuci wyjątek, jeśli będzie wywoływany wiele razy dla tego samego name. Czy w rezultacie moja czujność jest całkowicie zbędna, czy też istnieją jakieś mniej oczywiste korzyści?

To C #, Unity, jeśli ktoś jest zainteresowany.

Zaletą jest to, że Twój „niestandardowy” wyjątek zawiera komunikat o błędzie, który ma znaczenie dla każdego, kto wywołuje tę funkcję, nie wiedząc, jak jest ona zaimplementowana (którym w przyszłości możesz być Ty!).

To prawda, że ​​w tym przypadku prawdopodobnie byliby w stanie odgadnąć, co oznaczał „standardowy” wyjątek, ale nadal dajesz do zrozumienia, że ​​naruszyli twoją umowę, zamiast natknąć się na jakiś dziwny błąd w kodzie.

Zgadzam się z odpowiedzią Ixreca . Warto jednak rozważyć trzecią alternatywę: uczynić funkcję idempotentną. Innymi słowy, wróć wcześnie zamiast rzucać ArgumentException. Jest to często preferowane, jeśli w innym przypadku będziesz zmuszony sprawdzać, czy został już załadowany przed LoadMaterialkażdym wywołaniem . Im mniej masz warunków wstępnych, tym mniej obciążenia poznawczego dla programisty.

Zgłoszenie wyjątku byłoby lepsze, jeśli jest to naprawdę błąd programisty, który powinien być oczywisty i znany w czasie kompilacji, jeśli materiał został już załadowany, bez konieczności sprawdzania w czasie wykonywania przed wywołaniem.

Podstawowe pytanie, które musisz zadać, brzmi: jaki ma być interfejs dla twojej funkcji? W szczególności, czy warunek _Materials.ContainsKey(name)jest warunkiem wstępnym funkcji?

Jeśli to nie warunek konieczny, funkcja musi zapewnić dobrze zdefiniowany wynik dla wszystkich możliwych dolin name. W takim przypadku wyjątek zgłoszony, jeśli namenie jest częścią, _Materialsstaje się częścią interfejsu funkcji. Oznacza to, że musi on być częścią dokumentacji interfejsu i jeśli kiedykolwiek zdecydujesz się zmienić ten wyjątek w przyszłości, będzie to przełomowa zmiana interfejsu .

Bardziej interesujące jest pytanie, co się stanie, jeśli będzie to warunkiem wstępnym. W tym przypadku sam warunek staje się częścią interfejsu funkcji, ale sposób, że funkcja zachowuje się, gdy ten warunek jest łamane to nie koniecznie częścią interfejsu.

W tym przypadku podejście, które opublikowałeś, sprawdzając naruszenia warunków wstępnych i zgłaszając błąd, jest tak zwane programowaniem defensywnym . Programowanie defensywne jest dobre, ponieważ powiadamia użytkownika wcześnie, kiedy popełnił błąd i wywołał funkcję z fałszywym argumentem. Jest to złe, ponieważ znacznie zwiększa obciążenie związane z konserwacją, ponieważ kod użytkownika może zależeć od tego, czy funkcja w określony sposób obsługuje naruszenia warunków wstępnych. W szczególności, jeśli okaże się, że kontrola czasu wykonywania stanie się kiedykolwiek wąskim gardłem wydajności w przyszłości (mało prawdopodobne w tak prostym przypadku, ale dość powszechnym w przypadku bardziej złożonych warunków wstępnych), możesz już nie być w stanie go usunąć.

Okazuje się, że te wady mogą być bardzo znaczące, co spowodowało, że program obronny miał złą reputację w niektórych kręgach. Jednak początkowy cel jest nadal aktualny: chcemy, aby użytkownicy naszej funkcji zauważyli wcześniej, kiedy popełnili błąd.

Dlatego wielu programistów proponuje obecnie nieco inne podejście do tego rodzaju problemów. Zamiast zgłaszać wyjątek, używają mechanizmu przypominającego asercję do sprawdzania warunków wstępnych. Oznacza to, że w kompilacjach debugowania można sprawdzić warunki wstępne, aby pomóc użytkownikom wcześnie wychwytywać błędy, ale nie są one częścią interfejsu funkcji. Różnica na pierwszy rzut oka może wydawać się subtelna, ale w praktyce może mieć ogromną różnicę.

Technicznie wywołanie funkcji z naruszonymi warunkami wstępnymi jest niezdefiniowanym zachowaniem. Ale implementacja może zdecydować o wykryciu tych przypadków i natychmiastowym powiadomieniu użytkownika, jeśli tak się stanie. Wyjątki niestety nie są dobrym narzędziem do implementacji tego, ponieważ kod użytkownika może na nie zareagować i dlatego może zacząć polegać na ich obecności.

Szczegółowe wyjaśnienie problemów z klasycznym podejściem obronnym, a także możliwą implementację sprawdzania warunków wstępnych w stylu asertywnym, można znaleźć w wykładzie Johna Lakosa Defensive Programming Done Right z CppCon 2014 ( Slajdy , wideo ).

Jest tu już kilka odpowiedzi, ale oto odpowiedź, która uwzględnia Unity3D (odpowiedź jest bardzo specyficzna dla Unity3D, większość z nich zrobiłbym inaczej w większości kontekstów):

Ogólnie rzecz biorąc, Unity3D nie wykorzystuje tradycyjnie wyjątków. Jeśli rzucisz wyjątek w Unity3D, nie będzie to tak jak w przeciętnej aplikacji .NET, a mianowicie nie zatrzyma programu, t większość możesz skonfigurować edytor do pauzy. Zostanie tylko zalogowany. Może to łatwo doprowadzić grę do nieprawidłowego stanu i stworzyć efekt kaskady, który utrudnia wyśledzenie błędów. Tak więc powiedziałbym w przypadku Unity, pozwolenie na Addrzucenie wyjątku jest szczególnie niepożądaną opcją.

Ale badanie szybkości wyjątków nie jest w niektórych przypadkach przypadkiem przedwczesnej optymalizacji z powodu tego, jak Mono działa w Unity na niektórych platformach. W rzeczywistości Unity3D na iOS obsługuje niektóre zaawansowane optymalizacje skryptów, a wyłączone wyjątki * to efekt uboczny jednego z nich. Jest to naprawdę coś do rozważenia, ponieważ te optymalizacje okazały się bardzo cenne dla wielu użytkowników, co pokazuje realistyczny przypadek rozważenia ograniczenia stosowania wyjątków w Unity3D. (* zarządzane wyjątki od silnika, a nie kod)

Powiedziałbym, że w Unity możesz chcieć przyjąć bardziej wyspecjalizowane podejście. Jak na ironię, bardzo odrzucona odpowiedź w momencie pisania tego , pokazuje jeden sposób, w jaki mógłbym wdrożyć coś takiego właśnie w kontekście Unity3D (gdzie indziej coś takiego jest naprawdę nie do przyjęcia, a nawet w Unity jest dość nieeleganckie).

Innym podejściem, które rozważam, w rzeczywistości nie jest wskazanie błędu w stosunku do dzwoniącego, ale raczej użycie Debug.LogXXfunkcji. W ten sposób otrzymujesz to samo zachowanie, co zgłaszanie nieobsługiwanego wyjątku (z powodu sposobu, w jaki obsługuje je Unity3D) bez ryzyka narażenia czegoś w dziwnym stanie na linii. Zastanów się także, czy to naprawdę jest błąd (czy próba załadowania tego samego materiału dwa razy niekoniecznie oznacza błąd w twoim przypadku? A może to przypadek, w którym Debug.LogWarningjest to bardziej odpowiednie).

A jeśli chodzi o korzystanie z Debug.LogXXfunkcji takich jak funkcje zamiast wyjątków, nadal musisz zastanowić się, co się stanie, gdy wyjątek zostanie zgłoszony z czegoś, co zwraca wartość (np. GetMaterial). Podchodzę do tego, przekazując zero i rejestrując błąd ( znowu tylko w Unity ). Następnie używam kontroli zerowej w moich MonoBehaviors, aby upewnić się, że jakakolwiek zależność, taka jak materiał, nie jest wartością zerową, i wyłączam MonoBehavior, jeśli jest. Przykładem prostego zachowania wymagającego kilku zależności jest coś takiego:

    public void Awake()
    {
        _inputParameters = GetComponent<VehicleInputParameters>();
        _rigidbody = GetComponent<Rigidbody>();
        _rigidbodyTransform = _rigidbody.transform;
        _raycastStrategySelector = GetComponent<RaycastStrategySelectionBehavior>();

        _trackParameters =
            SceneManager.InstanceOf.CurrentSceneData.GetValue<TrackParameters>();

        this.DisableIfNull(() => _rigidbody);
        this.DisableIfNull(() => _raycastStrategySelector);
        this.DisableIfNull(() => _inputParameters);
        this.DisableIfNull(() => _trackParameters);
    }

SceneData.GetValue<>jest podobny do twojego przykładu, ponieważ wywołuje funkcję w słowniku, która zgłasza wyjątek. Ale zamiast zgłaszania wyjątku używa tego, Debug.LogErrorco daje ślad stosu, tak jak normalny wyjątek, i zwraca wartość null. Kontrole, które następują *, wyłączą to zachowanie, zamiast pozwolić, aby istniało nadal w nieprawidłowym stanie.

* czeki wyglądają tak z powodu małego pomocnika, którego używam, który drukuje sformatowaną wiadomość, gdy wyłącza obiekt gry **. Proste sprawdzanie wartości zerowej z ifpracą tutaj (** czeki pomocnika są kompilowane tylko w kompilacjach Debugowania (takich jak aserts). Używanie lambd i takich wyrażeń w Unity może mieć negatywny wpływ na wydajność)

Lubię dwie główne odpowiedzi, ale chcę zasugerować, że można poprawić nazwy twoich funkcji. Jestem przyzwyczajony do Javy, więc YMMV, ale metoda „dodaj” powinna, IMO, nie zgłaszać wyjątku, jeśli element już tam jest. Powinien dodać element ponownie lub, jeśli miejscem docelowym jest Zestaw, nic nie rób. Ponieważ nie jest to zachowanie Materials.Add, należy zmienić nazwę na TryPut lub AddOnce lub AddOrThrow lub podobne.

Podobnie twoja nazwa LoadMaterial powinna zostać zmieniona na LoadIfAbsent lub Put lub TryLoad lub LoadOrThrow (w zależności od tego, czy użyjesz odpowiedzi nr 1 czy nr 2), czy też czegoś takiego.

Postępuj zgodnie z konwencjami nazewnictwa C # Unity, bez względu na to, jakie są.

Będzie to szczególnie przydatne, jeśli masz inne funkcje AddFoo i LoadBar, w których można dwukrotnie ładować to samo. Bez jasnych nazw deweloperzy będą sfrustrowani.

Wszystkie odpowiedzi dodają cenne pomysły, chciałbym je połączyć:

Zdecyduj o planowanej i oczekiwanej semantyce LoadMaterial()operacji. Istnieją co najmniej następujące opcje:

• Warunek wstępny dla name∉ LoadedMaterials : →

  Kiedy warunek jest łamane, efekt LoadMaterial()jest nieokreślona (jak w odpowiedzi przez ComicSansMS ). Pozwala to na największą swobodę we wdrażaniu i przyszłych zmianach LoadMaterial(). Lub,

• efekty wywołania LoadMaterial(name)z name∈ LoadedMaterials są określone; zarówno:

  • specyfikacja stwierdza, że ​​zgłaszany jest wyjątek; lub

  • specyfikacja stwierdza, że wynik jest idempotent (jak w odpowiedzi przez Karla Bielefeldta )

    • zwracając prawdopodobnie „ma wartość logiczną zbiór changed” (jak proponowane przez User949300 i zaproponował (w niektórych interpretacjach) przez Mrlveck .

Decydując się na semantykę, musisz wybrać implementację. Proponowane są następujące opcje i uwagi:

• Rzucać niestandardową wyjątku (jako sugerowane przez Ixrec ) →

  Zaletą jest to, że „niestandardowy” wyjątek zawiera komunikat o błędzie, który ma znaczenie dla każdego, kto wywołuje tę funkcję - Ixrec i User16547

  • Aby uniknąć kosztów wielokrotnego sprawdzania name∉ LoadedMaterials , możesz postępować zgodnie z radą Marc van Leeuwen :

    ... Zamiast tego rozważ zanurzenie się w _Materials.Dodaj bezwarunkowo, a następnie złap potencjalny błąd i w module obsługi rzuć inny.

• Niech Dictionay . Dodaj wyjątek →

  Kod zgłaszania wyjątku jest zbędny. - Jon Raynor

  Chociaż większość wyborców zgadza się bardziej z Ixrec

  Dodatkowym powodem wyboru tej implementacji są:

  • osoby dzwoniące mogą już obsługiwać ArgumentExceptionwyjątki oraz
  • aby uniknąć utraty informacji o stosie.

  Ale jeśli te dwa powody są ważne, możesz również uzyskać niestandardowy wyjątek ArgumentExceptioni użyć oryginału jako wyjątku łańcuchowego.

• Dokonaj LoadMaterial()idempotent jak anwser przez Karla Bielefeldta i upvoted najczęściej (75 razy).

  • zwracając prawdopodobnie „ma wartość logiczną zbiór changed” (jak proponowane przez User949300 i zaproponował (w niektórych interpretacjach) przez Mrlveck .

  Opcje implementacji tego zachowania:

  • Sprawdź za pomocą Dictionary.ContainsKey ()

  • Zawsze wywołuj Dictionary.Add () łap ArgumentExceptiongo, gdy klucz do wstawienia już istnieje i zignoruj ​​ten wyjątek. Udokumentuj, że ignorowanie wyjątku jest tym, co zamierzasz zrobić i dlaczego.

    • Kiedy LoadMaterials()jest (prawie) zawsze wywoływany raz dla każdego name, pozwala to uniknąć wielokrotnego sprawdzania kosztów name∉ LoadedMaterials por. Marc van Leeuwen . Jednak,
    • gdy LoadedMaterials()jest często nazywany tym samym wiele razy, powoduje nameto (kosztowny) koszt rzucania ArgumentExceptioni rozwijania stosu.

  • Pomyślałem, że istnieje TryAddanalogiczna metoda TryGet (), która pozwoliłaby uniknąć kosztownego generowania wyjątków i odwijania stosów w przypadku nieudanych wywołań Dictionary.Add.

    Ale ta TryAddmetoda wydaje się nie istnieć.

Kod zgłaszania wyjątku jest zbędny.

Jeśli zadzwonisz:

_Materials.Add (nazwa, Resources.Load (ciąg.Format („Materiały / {0}”, nazwa)) jako Materiał

tym samym klawiszem wyrzuci

System.ArgumentException

Wiadomość brzmi: „Element z tym samym kluczem został już dodany”.

ContainsKeyWyboru jest zbędny, ponieważ w zasadzie takie samo zachowanie jest osiągnięty bez niego. Jedyny element, który się różni, to faktyczny komunikat w wyjątku. Jeśli istnieje prawdziwa korzyść z posiadania niestandardowego komunikatu o błędzie, kod ochronny ma pewną zaletę.

W przeciwnym razie prawdopodobnie w tym przypadku zreformowałbym strażnika.

Myślę, że jest to bardziej pytanie o projekt API niż pytanie dotyczące konwencji kodowania.

Jaki jest oczekiwany wynik (umowa) połączenia:

LoadMaterial("wood");

Jeśli osoba dzwoniąca może oczekiwać / ma gwarancję, że po wywołaniu tej metody ładowany jest materiał „drewno”, to nie widzę powodu, aby rzucać wyjątek, gdy ten materiał jest już załadowany.

Jeśli podczas ładowania materiału wystąpi błąd, na przykład nie można otworzyć połączenia z bazą danych lub w repozytorium nie ma materiału „drewno”, wówczas należy zgłosić wyjątek, aby powiadomić osobę dzwoniącą o tym problemie.

Dlaczego nie zmienić metody, aby zwracała wartość „prawda”, jeśli dodano materię, i „fałsz”, jeśli tak nie było?

private bool LoadMaterial(string name)
{
   if (_Materials.ContainsKey(name))

    {

        return false; //already present
    }

    _Materials.Add(
        name,
        Resources.Load(string.Format("Materials/{0}", name)) as Material
    );

    return true;

}