2009-10-30 13 views
6

Ho un esempio di codice che vedo spesso nei siti web che vorrei migliorare e che apprezzerebbe un po 'di aiuto. Spesso vedo 5-10 istruzioni if ​​annidate in un metodo page_load che mirano ad eliminare l'input dell'utente non valido, ma questo sembra brutto ed è difficile da leggere e mantenere.Come riordinare troppe istruzioni per la leggibilità

Come suggeriresti di pulire il seguente esempio di codice? La cosa principale che sto cercando di eliminare sono le dichiarazioni annidate.

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); 
} 

Come potete vedere, questo diventa abbastanza disordinato molto rapidamente! Ci sono schemi o pratiche che dovrei seguire in questo caso?

risposta

20

Utilizzando 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. utilizzare restituire o genera un errore

+0

Wont raggiungere quel caso poiché c'è un caso utente == null sopra di esso sir – lemon

+0

Il caso dell'utente == null è già controllato. L'ordine di cui sono scritte le affermazioni è significativo. Devi iniziare controllando gli oggetti per null, quindi valori illegali ecc. Ecc. –

+0

Penso che questo approccio mi piaccia, grazie per il consiglio. – NickGPS

3

Si può ripulire la nidificazione un po 'negando le condizioni e scrivere un altro, se a catena:

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

Questo è un approccio interessante in realtà. Mi piace perché rimuovi la necessità di più dichiarazioni di ritorno. Grazie! – NickGPS

1

Meglio di dividerlo in più metodi (funzioni) .E sarà facile understand.if qualche persona nuova legge il codice di lui/lei capisce la logica solo leggendo il nome del metodo stesso (Nota: nome metodo dovrebbe esprimere ciò che prova lo fa) codice .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

Buona idea, penso. Hai un esempio di come questo potrebbe aiutare a terminare l'esecuzione del metodo corrente? – NickGPS

+0

+1, questa è la migliore soluzione IMHO – rcampbell

Problemi correlati