CODEWITHSUNDEEP
phplogic
Tattat
Tattat

Reputation: 15778

How to simplify this code? (too many if else statment)

Here is a simple code for user input information, the first if's condition is this->sanitize, to check the data is valid or not, another is this->haveDuplicateUser_nameAndEmail(), which is use to check the user name and email is existed in the db or not. the third one is this->addNewUser(), which add the user record to the db. 

    if(!$this->sanitize()){
        $this->printError();   //the data is not sanitize
        return;
    }else{       
        if($this->haveDuplicateUser_nameAndEmail()){ //duplicateUserNameAndPassword, cannot add new user
            $this->printError();
        }else{
            if($this->addNewUser()){ 
                $this->printSuccess(); //add user success
            }else{
                $this->printError(); //add user fail
            }
        }
    }

Upvotes: 1

Views: 586

Answers (5)

The Moof
The Moof

Reputation: 804

$is_valid = $this->sanitize() && !$this->haveDuplicateUser_nameAndEmail();
if($is_valid && $this->addNewUser()){
    $this->printSuccess();
}else{
    $this->printError();
}

You could also get away with doing it without the $is_valid variable, but I think it helps with readability of the code if you need to come back and maintain it later.

Upvotes: 0

delmet
delmet

Reputation: 1013

The whole thing is equivalent to:

    if(!$this->sanitize()){
        $this->printError();   //the data is not sanitize
        return;
    } else if($this->haveDuplicateUser_nameAndEmail()){       
        $this->printError();
    } else if($this->addNewUser()) {
        $this->printSuccess(); //add user success
    } else {
         $this->printError(); //add user fail
    }

I take it this is javascript and supports else ifs.

Upvotes: 0

Explosion Pills
Explosion Pills

Reputation: 191809

You can use exceptions to simplify the block you have presented to us. You will have to update the code of the respective methods to throw these exceptions based on their own internal, boolean logic. There is no non-if solution to say something like "Is this POST equal to an empty string?" in php.

If you do this, you are getting into the realm of using exceptions as gotos, which is generally frowned upon. I think you can debate it either way.

try {
   $this->sanitize();
   $this->haveDuplicateUser_nameAndEmail();
   $this->addNewUser();
   $this->printSuccess();
}
catch (SanitizeOrDuplicateException $sode) {
   $this->printError();
}

Upvotes: 1

paulsm4
paulsm4

Reputation: 121869

Assuming that first "return" was extraneous, then:

if ((!$this->sanitize()) 
     || ($this->haveDuplicateUser_nameAndEmail())
     || (!$this->addNewUser()) {
   $this->printError();   //the data is not sanitize
}
else
  $this->printSuccess(); //add user success

Or perhaps you want to return on ANY error? If so, just add "return".

But there's nothing really "wrong" with your first snippet. If it covers all conditions correctly - then go for it :)

Upvotes: 1

Patrick87
Patrick87

Reputation: 28332

Pseudocode:

  if not this->sanitize() or
     this->haveDuplicateUser_nameAndEmail() or
     not this->addNewUser() then
     this.printError()
  else then
     this.printSuccess()

Note: This assumes short-circuiting behavior or functions that aren't going to epic fail if previous conditions aren't satisfied.

Upvotes: 3

Related Questions