2009-10-30 11 views
6

Tengo un ejemplo de código que veo a menudo en sitios web que me gustaría mejorar y agradecería algo de ayuda. A menudo veo 5-10 declaraciones if anidadas en un método page_load que apuntan a eliminar la entrada de usuario no válida, pero esto se ve feo y es difícil de leer y mantener.Cómo ordenar demasiadas declaraciones if para la legibilidad

¿Cómo recomendaría limpiar el siguiente ejemplo de código? Lo principal que intento eliminar son las sentencias if anidadas.

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

Como puede ver, esto se pone muy sucio muy rápidamente! ¿Hay algún patrón o práctica que debería seguir en este caso?

Respuesta

20

Mediante el uso de Guard Clauses señor

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. utilizar devolverán o generar un error

+0

Wont llegar ese caso ya que hay un caso de usuario == nulo en la parte superior señor – lemon

+0

El caso de usuario == null ya está marcado. El orden en que se escriben las declaraciones es significativo. Tienes que comenzar revisando los objetos para valores nulos, luego ilegales, etc. –

+0

Creo que me gusta este enfoque, gracias por el consejo. – NickGPS

3

Puede limpiar el anidamiento un poco mediante la negación de las condiciones y escribir una cadena else if-:

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

Ese es un enfoque interesante en realidad. Me gusta porque elimina la necesidad de declaraciones de devoluciones múltiples. ¡Gracias! – NickGPS

1

mejor dividir en múltiples métodos (funciones) .Es será fácil de understand.if alguna nueva persona lee el código de él/ella entiende la lógica de la sola lectura de la misma (Nota: nombre de método debe expresar lo que prueba que hace) nombre del método de código .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

Buena idea, creo. ¿Tiene un ejemplo de cómo esto podría ayudar a terminar la ejecución del método actual? – NickGPS

+0

+1, esta es la mejor solución en mi humilde opinión – rcampbell

Cuestiones relacionadas