CODEWITHSUNDEEP
if-statementcoding-style
NickGPS
NickGPS

Reputation: 1519

How to tidy up too many if statements for readability

I have an example of some code that I see often in websites that I'd like to improve and would appreciate some help. Often I see 5-10 nested if-statements in a page_load method which aim to eliminate invalid user input, but this looks ugly and is hard to read and maintain.

How would you recommend cleaning up the following code example? The main thing I'm trying to eliminate is the nested if statements. 

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

As you can see, this gets quite messy very quickly! Are there any patterns or practices that I should be following in this case?

Upvotes: 6

Views: 4208

Answers (3)

DonX
DonX

Reputation: 16367

Better to split it into multiple methods(functions) .It will be easy to understand.If some new person reads the code he/she understands the logic just by reading the method name itself(Note: Method name should express what test it does).Sample code :

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

Upvotes: 1

rsp
rsp

Reputation: 23373

You can clean up the nesting a bit by negating the conditions and write an if-else chain:

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.
}

Upvotes: 3

lemon
lemon

Reputation: 9205

By using 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. use either return or throw an error

Upvotes: 20

Related Questions