2010-10-15 7 views
6

Obecnie mam następującyC# to ten projekt "Prawidłowe"?

 if (!RunCommand(LogonAsAServiceCommand)) 
      return; 

     if (!ServicesRunningOrStart()) 
      return; 

     if (!ServicesStoppedOrHalt()) 
      return; 

     if (!BashCommand(CreateRuntimeBashCommand)) 
      return; 

     if (!ServicesStoppedOrHalt()) 
      return; 

     if (!BashCommand(BootstrapDataBashCommand)) 
      return; 

     if (!ServicesRunningOrStart()) 
      return; 

byłoby czystsze to zrobić? czy to jest bezpieczne?

 if (
      (RunCommand(LogonAsAServiceCommand)) 
     && (ServicesRunningOrStart()) 
     && (ServicesStoppedOrHalt()) 
     && (BashCommand(CreateRuntimeBashCommand)) 
     && (ServicesStoppedOrHalt()) 
     && (BashCommand(BootstrapDataBashCommand)) 
     && (ServicesRunningOrStart()) 
     ) 
     { 
       // code after "return statements" here 
     } 
+5

Zupełnie tak samo i sprawy stylu, z wyjątkiem tego, że pierwsze jest łatwiejsze do debugowania (i ustawiania punktów przerwania). –

+0

Zgadzam się z @Kirk Woll, gdy są zoptymalizowane, powinny być takie same, jeśli kompilator/jitter jest wystarczająco inteligentny. – leppie

Odpowiedz

1

Powinieneś trzymać się tego, co jest bardziej czytelne i zrozumiałe.

O ile nie jest to naprawdę nieefektywne.

+0

+1 To dołącza do wyrażenia "keep it simple"! =) –

3

Czy pierwszy kod jest zbiorem instrukcji OR?

if ( 
     (!RunCommand(LogonAsAServiceCommand)) 
    || (!ServicesRunningOrStart()) 
    || (!ServicesStoppedOrHalt()) 
    || (!BashCommand(CreateRuntimeBashCommand)) 
    || (!ServicesStoppedOrHalt()) 
    || (!BashCommand(BootstrapDataBashCommand)) 
    || (!ServicesRunningOrStart()) 
+1

Jeśli robisz to jako instrukcje OR, uzyskujesz efektywność zwracania, gdy tylko którykolwiek z warunków jest spełniony, zamiast czekać, czy wszystkie są. –

+6

Zarówno '&&' jak i '||' są zwarte, więc nie ma żadnej realnej korzyści. – Ani

+0

+1 za wydajność i poprawienie drugiego rozwiązania OP. – BrunoLM

2

Wszystko jest w porządku. Jednak z punktu widzenia stylu sugerowałbym, że skoro jest to seria komend, można użyć funkcji List<Action>, aby je przytrzymać i sterować danymi funkcji.

(new Action[] { func1, func2, .... }).ToList().ForEach(a => a()); 

Oczywiście ponieważ mają różne funkcje podpisów może trzeba zrobić kilka z nich ...

znowu jest to kwestia stylu ....

+1

To jest miłe podejście, ale nadal nie bardzo przyjazne debugowanie. Zwłaszcza, gdy wypełniasz listę lambdą w każdym miejscu. Normalne metody powinny być jednak w porządku. – leppie

+0

zgodził się na komentarz do debugowania. Mam bardzo dużo danych w moim kodzie, ale sprawia to, że jest bardziej przyjazny dla testów jednostkowych. Również używam rejestrowania więcej niż debugowanie do analizy w dzisiejszych czasach –

+0

Tego rodzaju rzeczy debugują znacznie lepiej w vs2010 :) –

4

Kiedy patrzę na pierwszym podejściem, moją pierwszą myślą jest to, że powodem, dla którego kod został napisany w ten sposób, jest to, że operacja ta prawdopodobnie ma skutki uboczne. Od nazw metod, zakładam, że robią? Jeśli tak, zdecydowanie skorzystam z pierwszego podejścia, a nie z drugiego; Wydaje mi się, że czytelnicy kodu mogą uznać za bardzo mylące wyrażenie o zwarciu, które jest używane do warunkowego wykonywania efektów ubocznych.

Jeśli nie ma żadnych skutków ubocznych, zrobi to; oba są doskonale czytelne. Jeśli okaże się, że istnieje kilka innych warunków lub warunki różnią się w różnych sytuacjach, można spróbować czegoś takiego:

Func<bool>[] conditions = ... 
if(conditions.Any(condn => condn())) 
{ 
    ... 
} 

Naprawdę nie iść z takim podejściem w Twoim przypadku jednak.

+0

co masz na myśli przez "efekty uboczne"? – bevacqua

+0

http://en.wikipedia.org/wiki/Side_effect_(computer_science). W rzeczywistości chodzi o to, że w tym przypadku wybór zwarcia, a nie zwarcia, wprowadza zauważalną, funkcjonalną różnicę w stanie programu. – Ani

+2

+1 Za zapewnienie dobrej reguły decydowania, które oświadczenie będzie bardziej czytelne. Eric Lippert ma stanowisko w sprawie SO gdzieś omawia tę zasadę w kontekście operatora trójskładnikowego, ale nie mogę tego znaleźć. – Brian

-1

Jeśli przy użyciu tego kodu wielokrotnie Proponuję umieścić ją w osobnym metody takie jak:

public static class HelperClass 
{ 
    public static bool ServicesRunning() 
    { 
    // Your code here... 
    } 
} 

i nazywając je, gdy są potrzebne

public class SomeClass 
{ 
    // Constructors etc... 
    public void SomeMethod() 
    { 
    if(HelperClass.ServicesRunning()) 
    { 
     // Your code here... 
    } 
    } 
} 
+0

huh, wyraźnie to już robię? – bevacqua

+0

Cóż, to nie było dokładnie jasne, że to był twój projekt. Ale jeśli to już robisz, mogę tylko powiedzieć, że zgadzam się z tym projektem. Korzystanie z długich &&/|| instrukcje if łatwo się brudzą (moim zdaniem). – Mantisen

Powiązane problemy