Rozważmy następujący kod:

IEnumerable<Employee> employees=GetAllEmployees();
if (employees.Count() > 0)
{
    // jakis kod
}

Załóżmy, że chcemy wykonać jakiś kod jeśli istnieją w bazie pracownicy. Ponadto nie mamy pojęcia co dokładnie zwraca GetAllEmployees – wiemy, że jest to IEnumerable<Employee>. Jeśli jest to np. LINQ – SQL, rozwiązanie jest bardzo niewydajne. Lepiej zastąpić to metodą Any():

IEnumerable<Employee> employees=GetAllEmployees();
if (employees.Any())
{
    // jakis kod
}

Count musi pobrać liczbę wszystkich wierszy w bazie lub wykonać foreach po każdym obiekcie. Any wykonuje po prostu MoveNext i jeśli operacja udała się to znaczy, że istnieje przynajmniej jeden obiekt. W przypadku LINQ to Objects, nie ma to znaczenia ponieważ jest tam optymalizacja która zwraca .Length przy wywołaniu Count(). W praktyce jednak nigdy nie wiemy co może zwrócić nam metoda więc lepiej pisać od razu optymalny kod.

W postach oznaczonych “Code review” zamierzam przedstawiam dobre i złe praktyki pisania kodu. Posty przeważnie będą składały się z fragmentu kodu i opisu co jest w nim źle. Dziś spójrzmy na:

StreamReader streamReader = new StreamReader(File.Open(path, FileMode.Open));
string header = streamReader.ReadLine();
string dataPoints = streamReader.ReadLine();
string[] dataPointArray = dataPoints.Split(new char[]{';’});
streamReader.Close();

Co w tym jest jest nie tak? W przypadku gdy np. ReadLine wyrzuci wyjątek, nie będziemy mieli opcji zamknięcia pliku. Dużo lepiej wszelkie operacje IO przechowywać w wyrażeniu using:

using(StreamReader streamReader = new StreamReader(File.Open(path, FileMode.Open)))
{
    string header = streamReader.ReadLine();
    string dataPoints = streamReader.ReadLine();
    string[] dataPointArray = dataPoints.Split(new char[]{';’});
}

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!

Dzisiaj prosta i częsta sprawa – obsługa zdarzeń.  Zdefiniujmy fikcyjne zdarzenie:

class SampleClass
{
   public event EventHandler EventHappened;
}

Następnie mamy jakąś metodę, która po wykonaniu własnej logiki, wywołuje zdarzenie. Jak powinno to zostać prawidłowo wykonane? Zacznijmy od niepoprawnego podejścia:

class SampleClass
{
   public event EventHandler EventHappened;
   private void Method()
   {
       EventHappened(this,new EventArgs());
   }
 }

Metoda spowoduje wyjątek jeśli żadne zdarzenie nie zostało doczepione. Rozwiązaniem jest klasyczne podejście, zgodne z guidelines:

class SampleClass
{
   public event EventHandler EventHappened;
   private void Method()
   {
       OnEventHappened(new EventArgs());
   }
   virtual protected void OnEventHappened(EventArgs e)
   {
       if (EventHappened != null)
           EventHappened(this, e);
   }
}

Stworzyliśmy więc wirtualną metodę protected zaczynająca się od przedrostka On, przyjmującą jako parametr wejściowy argumenty zdarzenia. W ciele metody sprawdzamy, czy są jakieś subskrypcje podpięte i następnie wywołujemy zdarzenie.

Rozwiązanie poprawne, zgodne z guidelines i większości przypadków właśnie w taki sposób obsługuję zdarzenia. Chciałbym jednak pokazać inne podejście,  które przede wszystkim pomaga w dwóch kwestiach:

1. Powyższa implementacja nie jest thread-safe. Jeśli dwa wątki niefortunnie wywołają OnEventHappened może dojść do problemu z synchronizacją warunku IF.
2. Niektórzy programiści są bardzo przeczuleni na punkcie IF w programowaniu obiektowym. Zwykle możemy się ich pozbyć poprzez polimorfizm lub Null Object pattern.

Rozwiązaniem jest przypisanie po prostu domyślnej wartości:

 public event EventHandler EventHappened = delegate { };  

Całość:

class SampleClass
{
   public event EventHandler EventHappened = delegate { };  
   private void Method()
   {
       OnEventHappened(new EventArgs());
   }
   virtual protected void OnEventHappened(EventArgs e)
   {
       EventHappened(this, e);
   }
}

Rozwiązanie polega na wykorzystaniu Null Object Pattern. Nie potrzebujemy już instrukcji IF – zawsze jest podpięte przynajmniej jedno zdarzenie.