Nieprawidłowa obsługa wyjątków może przynieść więcej problemów niż pożytku. O obsłudze błędów można byłoby napisać artykuł, jednak w poście chciałbym skupić się wyłącznie na kilku aspektach.
1. Pierwszym problemem jest fakt, że część programistów używa wyjątków do sytuacji po prostu niewyjątkowych. Jak sama nazwa mówi, wyjątek powinien być zastosowany gdzie może zdarzyć się coś niespodziewanego. Spójrzmy więc na poniższy fragment:
int number; try { number = int.Parse(text); } catch { number = -1; }
Załóżmy, że mamy aplikację w której użytkownik może wpisywać w pole jakiś tekst a następnie musimy to zamienić na liczbę. To, że użytkownik wpiszę tekst zamiast liczby nie jest wyjątkowe i stanowi normalny workflow. Powinniśmy zatem skorzystać z alternatywy np.:
int number; if(int.TryParse(text, out number) == false) number = -1;
Innym często spotykanym scenariuszem jest metoda przeszukująca bazę danych. Jeśli nie ma danego wiersza, nie powinniśmy wyrzucać wyjątków a po prostu NULL lub Null object pattern. W końcu jeśli implementujemy wyszukiwarkę, może się zdarzyć że szukana fraza nie istnieje w bazie danych:
Article FindArticle(string text) { Article article = ... // jakis SELECT * FROM if(article == null) throw new Exception("Article not found"); // ZLE! Powinnismy zwrocic po prostu null }
2. Nie należy również umieszczać w klauzuli Try\Catch zbyt dużej ilości kodu. Po pierwsze nie jest to optymalne rozwiązanie. Ważniejszym jednak powodem jest fakt, że wyrzucony i obsłużony wyjątek powinien dotyczyć prostych (pojedynczych) operacji. Zbyt ogólna informacja nie daje żadnych korzyści podczas debuggowania lub przeglądania wygenerowanych logów.
3. Rozważmy następujący fragment aplikacji:
try { // jakas logika } catch(Exception ex) { // obsluga wyjatku }
Typ Exception jest najbardziej ogólnym wyjątkiem. Powinniśmy raczej łapać poszczególne typy wyjątków i uzależnić od nich obsługę np:
try { // } catch(ConnectionException ce) { // nie mozna nawiazac polaczenie, moze warto zaimplementowac mechanizm recovery? } catch(Exception e) { // Nie wiadomo do konca co sie stalo - wykonujemy odpowiedni wpis // w pliku logow i np. zamykamy aplikacje }
Drugi Catch lapie wszystko, pierwszy tylko szczególny typ wyjątku.
4. Łapanie wyjątku i ignorowanie go:
try { // logika } catch(Exception e} { // brak jakiegokolwiek kodu }
Powyższy kod jest skrajnie zły – ignorując wyjątek pozwalamy aplikacji nadal funkcjonować i zapominamy po prostu, że to co chciał wykonać użytkownik tak naprawdę nie zostało wykonane. Skoro łapiemy wyjątek powinniśmy przynajmniej wykonać log.
6. Pomijanie stacktrace:
try { // logika } catch(Exception e) { // jakas logika, wykonanie logow itp. throw e;// ZLE!, throw new Exception("msg") rowniez jest niepoprawne }
Co się stanie z stacktrace? W momencie ponownego wyrzucenia, nowy zostanie wygenerowany z klauzuli catch. Stary zostanie po prostu usunięty i nie będziemy w stanie zobaczyć, w której metodzie tak naprawdę został wyrzucony wyjątek. Prawidłowe podejście, przechowujące prawidłowy stacktrace:
try { // logika } catch(Exception e) { // jakas logika, wykonanie logow itp. throw; }
Ewentualnie:
try { // logika } catch(Exception e) { // jakas logika, wykonanie logow itp. throw new WrappedException("custom par 1",e); }
W pewnych sytuacjach ukrywanie stacktrace ma sens – np. gdy nie chcemy dostarczyć hacker’owi jakiś informacji. Jednak myślę, że jest to rzadki przypadek dla większości z nas ( tak jak napisałem kiedyś, dostarczając bibliotekę DLL już dajemy potencjalnemu włamywaczowi sporo informacji).
To kilka pospolitych błędów. Jeśli macie jeszcze jakieś inne “kwiatki” to zachęcam do komentowania!
Mimo iż napisałeś o tym dodam, że sterowanie przepływem w “catch” kosztuje sporo czasu. Kiedy liczą się milisekundy i np w petli mamy wiele sytuacji w której wstawiamy logikę w catch to taki kod wykonuje się sporo (zauważalnie) dłużej.
Ponieważ nie jestem doświadczonym programistą niejednokrotnie miałem nieodpartą chęć wstawienia logiki typu return itp. Czy można powiedzieć, że zawsze należy unikać “return wartość” w catch?
W samym Return nie ma nic zlego. Chodzi o to, zeby nie upychac zbyt wiele. Bo co da nam wyjatek NULL reference jesli polowe aplikacji mamy w takim catchu:)? (wiem skrajny przypadek)
W starszych aplikacjach często spotyka się konstrukcję:
try
{
throw new Exception();
}
catch
{
}
Nie jest ona wcale błedna. Otóż zgłoszenie pierwszego wyjatku w aplikacji trwa troche dłużej niż pozostałych. W starszych wersjach .Net (1.1 i 2.0) różnica ta była znaczna, więc zgloszenie wyjątku podczas wyświetlania splash-screena miało sens.
Odnośnie pkt 6.
Problem pojawia się gdy musimy przerzucić wyjątek pomiędzy wątkami.
Przykład takiej sytuacji to implementacja SynchronizationContext.
W takiej sytuacji możemy:
1) Rzucić nowy wyjątek z źródłowym w InnerException. Jest to najwydajniejsze rozwiązanie (najszybciej się wykonuje). Jednak w ten sposób tracimy informacje o źródłowym wyjątku tj. mamy o nim informacje jedynie przez Exception.InnerException. Powoduje to konieczność łapania WrappedException zamiast np IOException którego się spodziewamy.
2) Przerzucić oryginalny wyjątek i pogodzić się z nadpisanym stack trace’em.
lub… 😉
3) Alternatywnie możemy wykonać Exception.PreserveStackTrace na oryginalnym wyjątku tuż przed jego przerzuceniem.
Gdzie:
—
public static void PreserveStackTrace(this Exception exception)
{
preserveStackTrace_.Invoke(exception, null);
}
private static readonly MethodInfo preserveStackTrace_ = typeof(Exception).GetMethod(“InternalPreserveStackTrace”, BindingFlags.Instance | BindingFlags.NonPublic);
—
czyli:
—
// Wątek 1
Exception ex_;
try
{
throw IOException();
}
catch(Exception ex)
{
ex_ = ex;
}
// Wątek 2
try
{
ex_.PreserveStackTrace();
throw ex_;
}
catch(IOException ex)
{
// ex ma cały stack trace 😉
}
—
Minusem tego rozwiązania jest pogorszenie się wydajności obsługi takiego wyjątku… jednak jest to akceptowalne w tym przypadku gdyż jak sam napisałeś w pkt 1. wyjątki powinny być stosowane jedynie dla określenia sytuacji niespodziewanych 😉
Źródło:
http://weblogs.asp.net/fmarguerie/archive/2008/01/02/rethrowing-exceptions-and-preserving-the-full-call-stack-trace.aspx
@Wojciech(szogun1987)
Dzieki za ciekawostke.
Dzis juz nie trzeba takiej blednej i brzydkiej kostrukcji robic.
Poza tym mowa o obsludze bledow a nie o omijaniu bledow frameworka:)
@Adrian:
Dzieki za wpis – bardzo ciekawa uwaga.
Z spadkiem prędkość przy użyciu try-catch może być różnie: http://stackoverflow.com/questions/8928403/try-catch-speeding-up-my-code
ad. 1
Co jak co, ale gdy masz dostać liczbę a user wklepuje tekst to JEST to sytuacja wyjątkowa. To, że to jest najczęstszy błąd jaki może user spowodować nie oznacza, że to jest coś zwykłego i normalnego. Jak myślisz – dlaczego metoda Int32.Parse może rzucić m.in. wyjątkiem typu FormatException? Bo autorzy uznali, że przy zamianie tekstu na liczbę podanie tekstu nie będącego liczbą JEST sytuacją wyjątkową. Przecież mogli by zamiast tego zwracać typ Nullable i wartość NULL.
Zgadzam się natomiast, że lepiej w tym wypadku użyć Int32.TryParse(). Jednak nie zawsze istnieją takie alternatywne metody. W SharePoincie 2007 było pełno metod, które rzucały wyjątkiem gdy nic nie znalazły, przez co trzeba było pisać własne wrappery, które w takim wypadku zwracały NULL. Rozwiązywało to problem, o którym piszesz dalej w tym samym punkcie. W SP2010 potworzyli trochę metod “TryGetSomething”, więc jest lepiej, ale chyba nadal nie ma tego wszędzie.
Z punktem 2. się zgadzam, ale często jest tak, że prawie każda metoda może rzucić błędem – co w takie sytuacji? Każda linijka do osobnego bloku?
ad. 4
Jest zły, chociaż czasem jest to zamierzone – np. wiemy, że ta metoda może rzucić wyjątek, ale robi to w niewyjątkowej sytuacji 😉 Są metody, które czasem “by design” rzucają wyjątkiem. Było coś takiego w ASP.NET, ze względu na zgodność z ASP 1.1 i w Azure przy wyciąganiu BLOB-ów (albo sprawdzaniu czy istnieją; nie pamiętam). W takim wypadku wyjątek nas nie interesuje; logowanie go – w sumie też nie (chociaż nie zaszkodzi).
ad. 6
Kurde, tego nie wiedziałem – muszę zapamiętać 🙂
Z resztą generalnie się zgadzam.
Hello,
1. Walidacja danych to nie jest sytuacja wyjatkowa i nie powinna polegac na lapaniu i wyrzucaniu wyjatkow – bardzo zle podejscie. Kazdy projekt powinien zawierac walidacje wiec z definicji nie jest to sytuacja wyjatkowa a bardzo dobrze obsluzona w kodzie (tak przynajmniej powinno byc).
2. Kazda linika moze wyrzucic wyjatek?To niemozliwe, pokaz mi taki projekt. Zawsze mozna podzielic to na bloki, zagniezdzic try\catch w funkcji. Jeden try\catch to rowniez bad practice.
4. Ale to przeciez wynika z bledow architektonicznych a nie z good practise.
Pozdrawiam
Piotr
ad. 1
W takim razie – co powinna robić walidacja? Może nie powinna rzucać wyjątków, ale łapać IMO powinna – chociażby po to, żeby móc wyświetlić “wprowadziłeś niepoprawne dane”, itp.
Poza tym walidacja może wyjątkowa nie jest, ale służy obsłudze wyjątkowych sytuacji. Nie mówię, że jest coś nadzwyczajnego w tym, że program dostaje niepoprawne dane. Ale jest to sytuacja wyjątkowa. “Normalny workflow” to moim zdaniem (!) sytuacja, w której program dostaje poprawne dane i działa jak należy. Każda sytuacja odbiegająca od normy, którą trzeba dodatkowo obsłużyć niezależnie od funkcjonalności jest wyjątkowa.
ad. 2
Np. mamy metodę, która wywołuje 5 innych, z czego pierwsza, trzecia i piąta rzucają (mogą rzucić) wyjątkiem. Są to metody z zewnętrznej biblioteki, więc nie da rady w nich nic zmienić no i chcemy mieć logowanie błędów. Czy w takiej sytuacji robić 3 try-catch’e czy objąć wszystko 1? Czy np. rozbijać to na kilka metod i dać bloki tam?
ad. 4
Tak, ale nie zmienia to faktu że takie sytuacje istnieją i nie mamy na nie wpływu a coś trzeba z tym zrobić 😉 Sam nigdy nie byłem zachwycony, gdy wywołanie jakiejś funkcji podczas normalnego działania rzucało mi wyjątkiem.
1. Roznie mozna intepretowac ale ogolna zasada jest taka ze exception jest dla “really exceptional situation”. Chodzi tu o performacne a walidacje da sie zrobic bez problemu nie uzywajac watkow. Jak widze w kodzie cos w stylu if(t is not string) then throw Exception mysle ze to naduzycie (nie tylko ja…).
2. Oba rozwiazana sa sluzne zarowno jeden try i 3 lub wiecej catchy jak i try dla kazdej metody. Zalezy to juz od konkretnego przypadku (co te metody robia, czy recovery wyglada tak samo itp.) Co do logowania bledow to robi sie je w globalnym miejscu.
3. Ale to sa hacki\workaroundy a post jest o best practice:)
ad. 1
Tak, to zdecydowanie nadużycie 🙂 Nie ukrywam jednak, że chodzi mi głównie właśnie o sytuacje, gdzie mamy do czynienia z “zewnętrzym” kodem i niektórych rzeczy obejść się (chyba) nie da. W końcu własny kod można napisać tak, że nigdy nie rzuci żadnego wyjątku, choćby nie wiem co się działo.
ad. 3.
No ok 😛