2009-10-30 19 views
6

Mam przykład kodu, który widzę często na stronach, które chciałbym poprawić i byłby wdzięczny za pomoc. Często widzę 5-10 zagnieżdżonych instrukcji if w metodzie page_load, które mają na celu wyeliminowanie nieprawidłowych danych wprowadzanych przez użytkownika, ale wygląda to paskudnie i jest trudne do odczytania i utrzymania.Jak posprzątać zbyt wiele instrukcji do czytelności

Jak poleciłbyś oczyścić poniższy przykład kodu? Najważniejszą rzeczą, którą próbuję wyeliminować, są zagnieżdżone instrukcje if.

string userid = Request.QueryString["userid"]; 

if (userid != ""){ 
    user = new user(userid); 

    if (user != null){ 
     if (user.hasAccess){ 
      //etc. 
     } 
     else{ 
      denyAccess(INVALID_ACCESS); 
     } 
    } 
    else{ 
     denyAccess(INVALID_USER); 
    } 
} 
else{ 
    denyAccess(INVALID_PARAMETER); 
} 

Jak widać, robi się bardzo niechlujnie bardzo szybko! Czy są jakieś wzorce lub praktyki, które powinienem zastosować w tym przypadku?

Odpowiedz

20

Korzystając Guard Clauses sir

string userid = Reuest.QueryString["userid"]; 

if(userid==null) 
return denyAccess(INVALID_PARAMETER); 

user = new user(userid); 
if(user==null) 
return denyAccess(INVALID_USER); 

if (!user.hasAccess) 
return denyAccess(INVALID_ACCESS); 

//do stuff 

PS. używać albo zwrócić lub wygeneruje błąd

+0

Wont dotrzeć ten przypadek, ponieważ jest na nim użytkownik == pusty przypadek sir – lemon

+0

Sprawa użytkownika == null jest już zaznaczone. Kolejność, w jakiej są napisane, jest znacząca. Musisz zacząć od sprawdzenia obiektów dla wartości null, później nielegalnych itp. –

+0

Myślę, że podoba mi się to podejście, dzięki za radę. – NickGPS

3

można oczyścić gniazdowania trochę przez negując warunki i napisać if-else Łańcuch:

string userid = Reuest.QueryString["userid"]; 

if (userid == "") { 
    denyAccess(INVALID_PARAMETER); 

} else if (null == (user = new user(userid))){ 
    denyAccess(INVALID_USER); 

} else if (!user.hasAccess){ 
    denyAccess(INVALID_ACCESS); 

} else { 
    //etc. 
} 
+0

To naprawdę interesujące podejście. Podoba mi się, ponieważ usuwasz potrzebę wielokrotnych instrukcji zwrotnych. Dzięki! – NickGPS

1

Lepiej podzielić ją na wiele sposobów (funkcji) .To będzie łatwo understand.If jakaś nowa osoba odczytuje kod on/ona rozumie logikę tylko poprzez czytanie sama nazwa (Uwaga: nazwa metody powinny wyrazić co badanie to robi) metoda kodu .Sample:

string userid = Request.QueryString["userid"]; 

if(isValidParameter(userId)){ 
    User user=new User(userId); 
    if(isValidUser(user)&&isUserHasAccess(user)){ 
     //Do whatever you want 
    } 
} 

private boolean isUserHasAccess(User user){ 
    if (user.hasAccess){ 
     return true; 
    }else{ 
     denyAccess(INVALID_ACCESS); 
     return false; 
    } 
} 

private boolean isValidUser(User user){ 
    if(user !=null){ 
     return true; 
    }else{ 
    denyAccess(INVALID_USER); 
    return false; 
    } 
} 


private boolean isValidParameter(String userId){ 
    if(userid !=""){ 
     return true; 
    }else{ 
denyAccess(INVALID_PARAMETER); 
    return false; 
    } 
} 
+0

Dobry pomysł, myślę. Czy masz przykład, w jaki sposób może to pomóc w zakończeniu wykonywania obecnej metody? – NickGPS

+0

+1, to jest najlepsze rozwiązanie IMHO – rcampbell

Powiązane problemy