General if/else structuring help

SamC · January 8, 2018

This is a general programming question. Is there a better way to structure something like this?

if ($input->post->sendMe) {
  if ($v->validate()) {
    if ($captcha->verifyResponse() === true) {

      $message = "
        <html>
          <body>
            <p><b>Name:</b> {$name}</p>
            <p><b>Email:</b> {$email}</p>
            <p><b>Message:</b></p>
            <p>{$message}</p>
          </body>
        </html>
        ";

      $mail = wireMail();

      $mail->to($contactFormRecipient)
      ->from($email, $name)
      ->subject('Email from website...')
      ->bodyHTML($message);

        if ($mail->send()) {
          $session->flashMessage = "Thanks for your message!";
          $session->sent = true;
          $session->redirect($pages->get($contactPageID)->url);
        }
        else {
          $session->flashMessage = "Sorry, an error occured. Please try again.";
        }

      }
    else {
      $session->flashMessage = 'Recaptcha must be complete.';
    }
  }
  else {
    $session->flashMessage = 'Please fill out the fields correctly.';
  }
}

It's probably trivial to someone with more experience but I find it tricky to follow (even though it's short). Is four nested ifs considered too many?

Just after some advice about different techniques on how to write something like this.

heldercervantes · January 8, 2018

Maybe invert the IFs and put the errors on top.

I mean:

...
if (!$v->validate()) {
	$session->flashMessage = 'Recaptcha must be complete.';
} else {
	...
}
...

This may make it more readable. Also some comments along the way can't hurt.

In terms of performance, 4 nested ifs start to sound like too much, but in this case I wouldn't worry about it because it's a form submit. If it was something that triggered every time a page was loaded, maybe I'd look into ensuring the best performance possible, but I don't think this would justify 2 extra hours of your time. Think how much your time costs, how many milliseconds you could shave off it, and ask yourself if it's worth it.

rick · January 9, 2018

To me it's a preference. For example, in user registration I use this structure:

if( $input->post('register') == '1' ) { // is our form submitted
    if($session->CSRF->hasValidToken()) { // is it our form
        if( $input->post->text('g-recaptcha-response') == '' ) { // are they beast or human
            $regError = "Invalid Captcha response.";
        } else {
            $regEmail = $input->post->email( 'regEmail' );
            if( $users->get( "email=$regEmail" )->id ) {
                $regError = "You cannot register more than one account with the same email address.";
            } else {
			...
            }
        }
    } else {
        header("Status: 401", true, 401);
    }
}

Since php does a short circuit test, it stops evaluating conditionals as soon as a value is known. So it doesn't really matter, unless you get into nesting elseif or use switch statements.

Whatever code style reads best for you and your team is how you should do it. Just my $.02

adrian · January 9, 2018

If you are in a function or a loop you can put those negative conditionals first and "return", "continue", or "break" - this can help to reduce nesting.

SamC · January 9, 2018

Thanks for the replies, some things to think about

FrancisChung · January 9, 2018

You could apply something like the Pipeline Design Pattern to streamline a set of sequential work, which I think is what you're trying to achieve @SamC ?

Having said that, it's not something I've used in PHP itself but I've used in other languages like C#.
The advantage is better readability and maintainability and no more long nested if - else statements.

https://medium.com/@aaronweatherall/the-pipeline-pattern-for-fun-and-profit-9b5f43a98130

An alternative method of getting rid of if - else statements is using functional programming paradigms like Lambda expressions and using optional types and using defensive programming techniques.

There's a great course on it on Pluralsight on this called Advanced Defensive Programming Techniques. It can get a bit heavy but it's profoundly changed my coding views in a positive way. Even if you're not familiar with C#, I would recommend having a look at this course if you really want to advance yourself as a coder.

https://www.pluralsight.com/courses/advanced-defensive-programming-techniques

SamC · January 9, 2018

10 hours ago, FrancisChung said:

https://medium.com/@aaronweatherall/the-pipeline-pattern-for-fun-and-profit-9b5f43a98130

I do like this indeed! Looks just like when you pipe gulp tasks. Not sure I can be bothered setting this up right now as it would distract from my JS and I haven't dug deep on PHP classes yet. However, this looks awesome, thanks

bernhard · January 9, 2018

maybe something like this?

// early exits
if(!$input->post('register')) return;
if(!$session->CSRF->hasValidToken()) { header("Status: 401", true, 401); return; }

// define variables
$errors = [];
$regEmail = $input->post->email( 'regEmail' );

// check for errors
if(!$input->post->text('g-recaptcha-response')) $errors[] = "Invalid Captcha response.";
if( $users->get( "email=$regEmail" )->id ) $errors[] = "You cannot register more than one account with the same email address.";

// return error-array or true
if(count($errors)) return $errors
return true;

nice read @FrancisChung

reminds me a little bit of this section in my new kickstart installer: https://gitlab.com/baumrock/kickstart/blob/master/kickstart.php#L139-146

Maybe I'm just too lazy for learning new things but I don't really think that the pipeline is that much better than using ifs and separate methods in a regular class...

bernhard · January 9, 2018

21 hours ago, rick said:

Whatever code style reads best for you and your team is how you should do it. Just my $.02

sorry, took the wrong code snippet, but totally agree on that

SamC · January 9, 2018

11 minutes ago, bernhard said:

sorry, took the wrong code snippet

Gave @rick a few ideas though

FrancisChung · January 9, 2018

19 minutes ago, bernhard said:

Maybe I'm just too lazy for learning new things but I don't really think that the pipeline is that much better than using ifs and separate methods in a regular class...

Should never be too lazy for learning. Keep feeding that curious inner child inside you!

I think all the myriad of intermediate and advanced techniques, paradigms, theories etc that we programmers use boils down to 2 objectives we strive for.
Readability and ease of (future) maintenance.

Something that helps achieve the two is proper encapsulation of code & logic, something I think I am still learning better ways to do after all these years!

The Pipeline is merely one way of encapsulating your code & logic in a certain way that helps achieve readability. Object Orientated Programming was born because of this need for encapsulating code.

You could certainly just use if statements and functions to achieve this as well, perhaps encapsulating some of the logic into smaller functions like below.

$cond1 = CheckSendMe();	//$input->post->sendMe. 
$cond2 = ValidateV(); //$v->validate
$cond3 = VerifyCaptcha();	//$captcha->verifyResponse()

if ($cond1 && cond2 && cond3) {
	ConstructEmail();
	if (SendMail())
		DisplaySendMsg()
	else
		DisplaySendFailMsg()
}

I guess the different ways of encapsulating code will yield different results and your mileage will definitely vary a lot depending on what and how you use it.

SamC · January 9, 2018

4 minutes ago, FrancisChung said:

Should never be too lazy for learning. Keep feeding that curious inner child inside you!

Totally this.

The problem with the if (a && b && c) approach (I'm on a phone, best I can do!) is that the errors are different depending on what's wrong, hence the nesting.

I don't like it so will def think of a way that suits me better. I like the idea of smaller functions as this is similar to the functional (JS) programming I'm learning at the mo. I'm becoming quite fond of it and finally made it to some lightbulb moments! I studied java for 6 months a couple of years ago and actually liked the OO approach too.

I guess experience will highlight my personal preferences.

FrancisChung · January 9, 2018

38 minutes ago, SamC said:

The problem with the if (a && b && c) approach (I'm on a phone, best I can do!) is that the errors are different depending on what's wrong, hence the nesting.

The idea with the validation functions is to encapsulate the checking logic and the error capture logic inside the function. You could even throw the error there immediately but I don't know what your requirements are.

function CheckSendMe($errorMsg) {
	if ($input->post->sendMe) {
		return true
	else
	{
		$errorMsg = $errorMsg."Error MSg here"; //appending error msgs here
		return false;
	}
}

...

$cond1 = CheckSendMe($errorMsg);
$cond2 = ValidateV($errorMsg);

...

if ($cond1 && $cond2 && $cond3) {
	....
}
else
	DisplayErrorMsg($errorMsg);

The idea here is to minimise any long nested if-else chains and favouring readability over pre optimizations or micro optimizations.

Perhaps if you're the only coder and or you're working in a small team or you're very familiar with the codebase then you may think this approach may seem like overkill.

Until you work on a huge codebase that is alien to you, previously worked on a by a long list of programmers with varying skill levels, and the sight of a huge function with a dozen if-else statements nested coming your way will probably change your mind quickly about readability.

SamC · January 10, 2018

I'm trying this out this evening and tomorrow. Having a hard time at the moment, but this is simply down to inexperience in organising code so the practice will do me good. I'm trying to break things down, so once I've cracked this, I'll be much happier with my progress.

FrancisChung · January 10, 2018

@SamC, if you're struggling with any aspects no matter how trivial. give me a shout and I can try and help out. I've quite enjoyed the discussions we're having so far.

SamC · January 10, 2018

1 hour ago, FrancisChung said:

@SamC, if you're struggling with any aspects no matter how trivial. give me a shout and I can try and help out. I've quite enjoyed the discussions we're having so far.

It's comments like this that make me appreciate this awesome forum! Thanks for the offer

One quick question which is quite in line with the thing I'm struggling with at the moment:

function CheckSendMe($errorMsg) {
  if ($input->post->sendMe) {
    return true
  }
  else
  {
  // how to use $errorMsg outside this function?
  $errorMsg = $errorMsg."Error MSg here"; //appending error msgs here
  return false;
  }
}

bernhard · January 10, 2018

30 minutes ago, SamC said:

// how to use $errorMsg outside this function?

that's why you should start playing around with classes

<?php
class Formchecker {
  public $errors = [];
  
  public function checkemail($email) {
    if(!$email) { // or any other great check
      $this->errors[] = 'Please provide an email';
      return false;
    }
    return true;
  }
  
  public function checkname($name) {
    if(!$name) { // or any other great check
      $this->errors[] = 'Please provide a name';
      return false;
    }  
    return true;
  }
}

$checker = new Formchecker();
$mail = $checker->checkemail($input->post->email);
$name = $checker->checkname($input->post->name);

// if($mail AND $name) {
if(!count($checker->errors)) {
  // create new page and send email
}
else {
  echo 'Please correct the following...<br>';
  foreach($checker->errors as $error) echo "$error<br>";
}

FrancisChung · January 10, 2018

36 minutes ago, SamC said:


function CheckSendMe($errorMsg) {
  if ($input->post->sendMe) {
    return true
  }
  else
  {
  // how to use $errorMsg outside this function?
  $errorMsg = $errorMsg."Error MSg here"; //appending error msgs here
  return false;
  }
}

You want to define $errorMsg outside the function, where your main logic resides.

I think I should also pass the message via reference not via value which is the default method of passing arguments. Funny enough, php.net has a very similar example there.

http://php.net/manual/en/functions.arguments.php#functions.arguments.by-reference

FrancisChung · January 10, 2018

15 minutes ago, bernhard said:

that's why you should start playing around with classes


<?php
class Formchecker {
  public $errors = [];
  
  ...
  
  public function NoErrors() {
    return !count($errors);
  }
  
  public function DisplayErrors() {
    foreach($checker->errors as $error) echo "$error<br>";
  }
}

$checker = new Formchecker();
$mail = $checker->checkemail($input->post->email);
$name = $checker->checkname($input->post->name);

// if($mail AND $name) {
if($checker->NoErrors) {
  // create new page and send email
}
else {
  echo 'Please correct the following...<br>';
  $checker->DisplayErrors();
}

You could take Bernhard's example and encapsulate it even further ...
I've added 2 new functions NoErrors & DisplayErrors to encapsulate and hide away more logic.

SamC · January 10, 2018

You're both right, I'm gonna try a class, my code is just getting ridiculous now lol, seems miles worse than my original one.

<?php namespace ProcessWire;

function generateFlashMessage($isValidated, $recaptchaResponse, $session, $feedback, $feedbackEmptyArr) {
  $str = "";

  if (!$isValidated) {
    array_push($feedbackEmptyArr, $feedback["errorValidation"]);
  }
  
  if (!$recaptchaResponse) {
    array_push($feedbackEmptyArr, $feedback["errorRecaptcha"]);
  }

  if ($isValidated && $recaptchaResponse) {
    if (!$session->sent) {
      array_push($feedbackEmptyArr, $feedback["errorSend"]);
    }
    else {
      array_push($feedbackEmptyArr, $feedback["success"]);
    }
  }

  if (count($feedbackEmptyArr)) {
    $str = '<ul class="mb-0">';
    foreach ($feedbackEmptyArr as $value) {
      $str .= '<li>' . $value . '</li>';
    }
    $str .= '<ul>';
  }

  return $str;
}

function createMessage($name, $email, $message) {
  $msg = "
    <html>
      <body>
        <p><b>Name:</b> {$name}</p>
        <p><b>Email:</b> {$email}</p>
        <p><b>Message:</b></p>
        <p>{$message}</p>
      </body>
    </html>
  ";
  return $msg;
}

function sendMail($contactFormRecipient, $name, $email, $message) {
  $mail = wireMail();
  
  $mail->to($contactFormRecipient)
  ->from($email, $name)
  ->subject('Email from website...')
  ->bodyHTML($message);

  return $mail->send();
}

function checkField($name, $v, $isSubmitted) {
  if($isSubmitted) {
    return $v->errors($name);
  }
}

wireIncludeFile("./vendor/vlucas/valitron/src/Valitron/Validator.php");
$captcha = $modules->get("MarkupGoogleRecaptcha");
$contactFormRecipient = "EMAIL_RECIPIENT";
$isSubmitted = $input->post->sendMe;

$feedbackEmptyArr = [];
$session->flashMessage = $feedbackEmptyArr;

$feedback = [
  "errorSend" => "Sorry, an error occured. Please try again.",
  "errorValidation" => "Please fill out the fields correctly.",
  "errorRecaptcha" => "Recaptcha must be complete.",
  "success" => "Thanks for your message!"
];

$name = $sanitizer->text($input->post->name);
$email = $sanitizer->email($input->post->email);
$message = $sanitizer->textarea($input->post->message);

$v = new \Valitron\Validator(array(
  "name" => $name,
  "email" => $email,
  "message" => $message
  )
);

$v->rule("required", ["name", "email", "message"]);
$v->rule("email", "email");

// validate form - true/false
$isValidated = $v->validate();
// verify recaptcha - true/false
$recaptchaResponse = $captcha->verifyResponse();

// if form is submitted
if($isSubmitted) {

  if ($isValidated && $recaptchaResponse) {
    $msg = createMessage($name, $email, $message);
    sendMail($contactFormRecipient, $name, $email, $msg);
    $session->sent = true;
  }

}
?>




<div id="form-top" class="mb-5"></div>

<h2>Send me a message</h2>

  <?php if($isSubmitted):?>
    <div class="alert <?php echo $session->sent ? 'alert-success' : 'alert-danger'?>" role="alert">
        <?= generateFlashMessage($isValidated, $recaptchaResponse, $session, $feedback, $feedbackEmptyArr); ?>
    </div>
  <?php endif;?>

    <form id="contact-form" method="post" action="#form-top">

      <div class="row">
          <div class="form-group col-sm-12 col-lg-6 py-2 <?= checkField('name', $v, $isSubmitted) ? 'has-danger' : ''?>">
              <label for="name">Name (required)</label>
              <input class="form-control" name="name" id="name" type="text" value="<?php if ($name) echo $name; ?>">
          </div>

          <div class="form-group col-sm-12 col-lg-6 py-2 <?= checkField('email', $v, $isSubmitted) ? 'has-danger' : ''?>">
              <label for="email">Email (required)</label>
              <input class="form-control" name="email" id="email" type="text" value="<?php if ($email) echo $email; ?>">
          </div>
      </div>

      <div class="form-group py-2 <?= checkField('message', $v, $isSubmitted) ? 'has-danger' : ''?>">
          <label for="message">Message (required)</label>
          <textarea class="form-control" name="message" id="message" rows="8"><?php if ($message) echo $message; ?></textarea>
      </div>

      <div>
          <label for="recaptcha">Recaptcha (required)</label>
          <!-- Google Recaptcha code START -->
          <?php echo $captcha->render(); ?>
          <!-- Google Recaptcha code END -->
      </div>

      <div class="form-group">
          <button type="submit" class="btn btn-primary" name="sendMe" value="1">Submit suggestion</button>
      </div>

    </form>

<?php
    $session->remove("flashMessage");
    $session->sent = false;
    echo $captcha->getScript();
?>

It's one of those time where I learn best... by seeing how not to do something! I still got some ways to go here methinks. Terrible code, great learning experience. This will for sure be one of those posts that I look back on and laugh.

On a plus note, it works (and I'm actually writing code and thinking about structure now which is awesome)

FrancisChung · January 10, 2018

58 minutes ago, SamC said:

It's one of those time where I learn best... by seeing how not to do something! I still got some ways to go here methinks

Everyone starts somewhere at the bottom and make their way up. Your code wasn't that terrible, just needed a small house clean

1 hour ago, SamC said:

On a plus note, it works (and I'm actually writing code and thinking about structure now which is awesome)

Glad to hear this. Makes it all worthwhile.

SamC · January 11, 2018

So one problem I'm having this morning is that the validation happens via another class:

wireIncludeFile("./vendor/vlucas/valitron/src/Valitron/Validator.php");

...which I wanted to use in my own class:

class FormChecker {

  // where to send the email
  public $contactFormRecipient = "";

  // every available notice
  public $notices = [
    "errorSend" => "Sorry, an error occured. Please try again.",
    "errorValidation" => "Please fill out the fields correctly.",
    "errorRecaptcha" => "Recaptcha must be complete.",
    "success" => "Thanks for your message!"
  ]; 

  // push required notices into array
  public $feedback = [];

  // where to send email
  public function setContactFormRecipient($emailAddress) {
    $this->contactFormRecipient = $emailAddress;
  }

  public function checkField($name) {
    // $v is undefined here
    return $v->errors($name);
  }

}

...and quick test:

$checker->setContactFormRecipient("myemail@test.com");
echo $checker->contactFormRecipient; // myemail@test.com

$checker->checkField("name"); // $v is undefined in class

Can I extend or implement the Valitron class so the variables are available in my own class rather than passing everything as parameters? i.e. it works like this but there must be a better way:

class FormChecker {

  public function checkField($name) {
    return $v->errors($name);
  }

}

$v = new \Valitron\Validator(array(
  "name" => $name,
  "email" => $email,
  "message" => $message
  )
);

$v->rule("required", ["name", "email", "message"]);
$v->rule("email", "email");

$checker->checkField("name", $v);

It's making more sense now I'm getting into it but variable scope seems to be my biggest issue right now.

bernhard · January 11, 2018

one way ~~(maybe the best?)~~ is to extend the class: https://stackoverflow.com/a/8089092/6370411

another way is to store the other class as property of your new class:

<?php
class Checker {
  private $validator;

  public function __construct($validator) {
    $this->validator = $validator;
  }

  public function doSomethingWithValidator($name) {
    return $this->validator->errors($name);
  }
}

require_once('Yourvalidator.php');
$validator = new Yourvalidator();
$checker = new Checker($validator);
$checker->doSomethingWithValidator($input->post->name);

FrancisChung · January 11, 2018

1 of OO Programming's Commandments is : "Thou shalt favour composition over inheritance (or extending a class)"
It can seem complex, especially if you're starting out on OO concepts.

https://stackoverflow.com/questions/49002/prefer-composition-over-inheritance

https://medium.com/humans-create-software/composition-over-inheritance-cb6f88070205

The Medium.com has one of the best explanations I've seen.

Quote

But the really big problem with inheritance is that you’re encouraged to predict the future. Inheritance encourages you to build this taxonomy of objects very early on in your project, and you are most likely going to make design mistakes doing that, because humans cannot predict the future (even though it feels like we can), and getting out of these inheritiance taxonomies is a lot harder than getting out of them.

@bernhard's code above is an example of composition.

SamC · January 11, 2018

Thanks for the info on this thread, it's appreciated greatly. I'm moving forward on this now, the composition example makes sense to me. I'm at this stage:

<?php namespace ProcessWire;

class FormChecker {

  // 3rd party validator object
  private $validator;

  public function __construct($validator) {
    $this->validator = $validator;
  }

  public function checkForm() {
    return $this->validator->validate(); 
  }

  public function checkField($name) {
    return $this->validator->errors($name);
  }

}

// require Valitron class
require_once("./vendor/vlucas/valitron/src/Valitron/Validator.php");

// get sanitized POST values
$name = $sanitizer->text($input->post->name);
$email = $sanitizer->email($input->post->email);
$message = $sanitizer->textarea($input->post->message);

// create new valitron
$v = new \Valitron\Validator(array(
  "name" => $name,
  "email" => $email,
  "message" => $message
  )
);

// create valitron rules
$v->rule("required", ["name", "email", "message"]);
$v->rule("email", "email");

// save as property on new $checker object
$checker = new FormChecker($v);

// has form been submitted?
$isSubmitted = $input->post->sendMe;

// does form validate after submission
$formValidates = $checker->checkForm();

$result = ($isSubmitted && $formValidates) ? "SUCCESS" : "FAIL";
echo "Form validation was a: " . $result;

?>




<div id="form-top" class="mb-5"></div>

<h2>Send me a message</h2>

  <form id="contact-form" method="post" action="#form-top">

    <div class="row">
        <div class="form-group col-sm-12 col-lg-6 py-2 <?= $checker->checkField('name') ? 'has-danger' : ''?>">
            <label for="name">Name (required)</label>
            <input class="form-control" name="name" id="name" type="text" value="<?php if ($name) echo $name; ?>">
        </div>

        <div class="form-group col-sm-12 col-lg-6 py-2 <?= $checker->checkField('email') ? 'has-danger' : ''?>">
            <label for="email">Email (required)</label>
            <input class="form-control" name="email" id="email" type="text" value="<?php if ($email) echo $email; ?>">
        </div>
    </div>

    <div class="form-group py-2 <?= $checker->checkField('message') ? 'has-danger' : ''?>">
        <label for="message">Message (required)</label>
        <textarea class="form-control" name="message" id="message" rows="8"><?php if ($message) echo $message; ?></textarea>
    </div>

    <div>
        <label for="recaptcha">Recaptcha (required)</label>
        <!-- Google Recaptcha code START -->
        <?php // echo $captcha->render(); ?>
        <!-- Google Recaptcha code END -->
    </div>

    <div class="form-group">
        <button type="submit" class="btn btn-primary" name="sendMe" value="1">Message me!</button>
    </div>

  </form>

And behold, so far so good!

Excitement levels rising now, I like where this is heading.

Gonna get my feedback messages in there now.

Sign In

General if/else structuring help

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Recently Browsing 0 members