Jump to content

General if/else structuring help


SamC
 Share

Recommended Posts

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.

Link to comment
Share on other sites

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.

  • Like 1
Link to comment
Share on other sites

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

 

 

  • Like 2
Link to comment
Share on other sites

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

  • Like 1
  • Thanks 1
Link to comment
Share on other sites

10 hours ago, FrancisChung said:

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 :) 

Link to comment
Share on other sites

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 installerhttps://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...

  • Like 1
Link to comment
Share on other sites

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. 
 

  • Like 3
Link to comment
Share on other sites

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.

  • Like 1
Link to comment
Share on other sites

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. 

 

 

  • Like 2
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

 

Link to comment
Share on other sites

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>";
}
Link to comment
Share on other sites

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

Link to comment
Share on other sites

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.

  • Like 1
Link to comment
Share on other sites

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)

form-example.jpg

  • Like 3
  • Haha 1
Link to comment
Share on other sites

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.

  • Like 1
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

 

  • Like 2
Link to comment
Share on other sites

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.

  • Like 4
Link to comment
Share on other sites

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!

form-example-new.jpg

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

Gonna get my feedback messages in there now.

  • Like 2
Link to comment
Share on other sites

 Share

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...