A little review and a little help pls :)

blabla296 · January 5, 2022, 2:03am

Hi there !

Here’s a little exercise I’ve got to do for my php class that just started so it’s very basic, sorry. I was just hoping you could point me out to some improvements that I could implement, where my logic is faulty or where I could’ve coded in a more practical (or beautiful ^^) way

<?php

var_dump($_POST);
 
    $bootypeLoc= MrPropre($_POST['typeloc']??"");
	$intAdulte= MrPropre($_POST['nbAdultes']??"");
	$intEnfants= MrPropre($_POST['nbEnfants']??"");
	$boodemiAdulte= MrPropre($_POST['demipensionadulte']??"");
	$boodemiEnfants= MrPropre($_POST['demipensionenfants']??"");
	$intAnnul= MrPropre($_POST['intAnnul']??"");
	$intPercent= MrPropre($_POST['intPercent']??"");

	$intCostAdulte = ssMult($intAdulte, $boodemiAdulte);
	$intCostEnfants = ssMult($intEnfants, $boodemiEnfants);
	
	$intSsTotal = ssAdd($bootypeLoc, $intAnnul, $intCostAdulte, $intCostEnfants);
	$intRemise = getPercentOfNumber($intSsTotal, $intPercent);
	$intTotal= ssAdd($intSsTotal, -$intRemise);
	
	
		
	// sanitize and validate int_var
	function MrPropre($data){
		if(isset($_POST['valider'])){
		$error = "";
		$data = filter_var($data, FILTER_SANITIZE_NUMBER_INT);
		if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
			$error = false;
			return $data;
			}
			else{
			$error = true;
			echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
			$data=""; 
		}
		}
	}

	// splat_Add_func
	function ssAdd(...$costs){
	if(isset($_POST['valider'])){
	$sum = 0;  
	foreach ($costs as $n){
		$sum += $n;  
		}  
		return $sum;  
	}
	}
	
	// Mult_func
	function ssMult($cost1, $cost2){
		if(isset($_POST['valider'])){
		return $cost1 * $cost2;  
		}
	}

	// Percent_func
	function getPercentOfNumber($number, $percent){
		if(isset($_POST['valider'])){
		return ($number / 100) * $percent;
		}
	}
	


?>

The input and output forms are on the same page as this code, I guess there’s no point in pasting html here. I just echo’d the result variables in the output form, nothing fancy.

The thing which annoys me the most is that isset condition I had to put at the start of each function. They were being executed at page loading and all the mathematical functions were giving me errors because there was no input data.
As a workaround, I prevented them from executing with isset until the submit button is clicked. Although it’s working as it is, it’s certainly not ideal… nor very aesthetic.

I know I don’t have to go through functions to make these simple calculations, but I’m really interested to learn how someone with more experience would do this differently while keeping the functions

Thanks in advance !

tracknut · January 5, 2022, 4:26am

If I’m understanding your dilemma, I would do it like this:

if(isset($_POST['valider'])){
//..... all the php code you're showing above to process the form after submitted
}
if (either the first display or you need to re-display the form because of validation issues) {
//....show the html form 
}

blabla296 · January 5, 2022, 4:46am

hey there,

I tried that first actually. Problem was, as part of the exercise, the input and output forms are actually hard-coded into the very same page as the php code I wrote, got the fields and everything. Meaning the echo’d php outputs are as well, which prevented me to use this method as I’d have to make all the variables global, for they would be in a isset function. Of course I was trying to avoid that. One solution would be to generate the output form as you suggested, but I don’t think that is the point of the exercise
Don’t know if I’m explaining very well… might be easier to understand if I post the whole thing actually ?

benanamen · January 5, 2022, 5:05am

The isset’s do not belong in the functions. Those are validation checks and should be done outside the function and would actually be not empty checks after you have trimmed the POST array. There is more to do but here is the basic idea.

In MrPropre function, the else is pointless. The script alert should be outside the function.

if ($_SERVER['REQUEST_METHOD'] === 'POST') {
    // trim post array

if (!empty($_POST['typeloc']))
{
MrPropre($_POST['typeloc'])
}

}

tracknut · January 5, 2022, 2:37pm

Yeah, I’m not following you. The input and output can be in this same page, and you can keep your validation functions (but remove the issets as benanamen mentions). If you don’t figure it out, create a small version of the code, with a form with a single input field, so you can post it here and get it working right before you make the form with all the fields.

Drummin · January 6, 2022, 4:33am

As you need the variables to echo to the form they should be defined regardless of POST. But there is no need to call the function if POST is not set or if a particular POST['KEYS'] are empty. So check for !empty() and call the function or set variable value as empty.

    $bootypeLoc = (!empty($_POST['typeloc']) ? MrPropre($_POST['typeloc']) : '');

Then when you want to multiply, check both values for !empty() before calling the function.

	$intCostAdulte = (!empty($intAdulte) && !empty($boodemiAdulte) ? ssMult($intAdulte, $boodemiAdulte) : '');

When adding things up you would check if ANY of the values are !empty() using “OR” condition between each variable.


	$intSsTotal = ((!empty($bootypeLoc) || !empty($intAnnul) || !empty($intCostAdulte) || !empty($intCostEnfants)) ? ssAdd($bootypeLoc, $intAnnul, $intCostAdulte, $intCostEnfants) : '');

As already mentioned, remove if(isset($_POST['valider'])){ from your functions.
Anyway, this is my take on it.

<?php
	
    $bootypeLoc = (!empty($_POST['typeloc']) ? MrPropre($_POST['typeloc']) : '');
    $intAdulte = (!empty($_POST['nbAdultes']) ? MrPropre($_POST['nbAdultes']) : '');
    $intEnfants = (!empty($_POST['nbEnfants']) ? MrPropre($_POST['nbEnfants']) : '');
    $boodemiAdulte = (!empty($_POST['typeloc']) ? MrPropre($_POST['demipensionadulte']) : '');
    $boodemiEnfants = (!empty($_POST['demipensionenfants']) ? MrPropre($_POST['demipensionenfants']) : '');
    $intAnnul = (!empty($_POST['intAnnul']) ? MrPropre($_POST['intAnnul']) : '');
    $intPercent = (!empty($_POST['intPercent']) ? MrPropre($_POST['intPercent']) : '');
																						 
	$intCostAdulte = (!empty($intAdulte) && !empty($boodemiAdulte) ? ssMult($intAdulte, $boodemiAdulte) : '');
	$intCostEnfants = (!empty($intEnfants) && !empty($boodemiEnfants) ? ssMult($intEnfants, $boodemiEnfants) : '');
	
	
	$intSsTotal = ((!empty($bootypeLoc) || !empty($intAnnul) || !empty($intCostAdulte) || !empty($intCostEnfants)) ? ssAdd($bootypeLoc, $intAnnul, $intCostAdulte, $intCostEnfants) : ''); 
	$intRemise = (!empty($intSsTotal) && !empty($intPercent) ? getPercentOfNumber($intSsTotal, $intPercent) : '');	
	$intTotal= (!empty($intSsTotal) || !empty($intRemise) ? ssAdd($intSsTotal, -$intRemise) : '');	
		
		
	// sanitize and validate int_var
	function MrPropre($data){
		$error = "";
		$data = filter_var($data, FILTER_SANITIZE_NUMBER_INT);
		if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
			$error = false;
			return $data;
		}
		else{
			$error = true;
			echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
			$data=""; 
		}
	}

	// splat_Add_func
	function ssAdd(...$costs){
		$sum = 0;  
		foreach ($costs as $n){
			$sum += $n;  
		}  
		return $sum;
	}
	
	// Mult_func
	function ssMult($cost1, $cost2){
		return $cost1 * $cost2; 
	}

	// Percent_func
	function getPercentOfNumber($number, $percent){
		return ($number / 100) * $percent;
	}

?>

Drummin · January 6, 2022, 9:36am

Correction

$boodemiAdulte = (!empty($_POST['demipensionadulte']) ? MrPropre($_POST['demipensionadulte']) : '');

rpkamp · January 6, 2022, 10:29pm

I’m going to take out this one function as it’s the biggest:

function MrPropre($data){
	if(isset($_POST['valider'])){
	$error = "";
	$data = filter_var($data, FILTER_SANITIZE_NUMBER_INT);
	if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
		$error = false;
		return $data;
	} else {
		$error = true;
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
		$data=""; 
	}
	}
}

So, as others have already pointed out, the $_POST check shouldn’t be here, it needs to go:

function MrPropre($data){
	$error = "";
	$data = filter_var($data, FILTER_SANITIZE_NUMBER_INT);
	if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
		$error = false;
		return $data;
	} else {
		$error = true;
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
		$data=""; 
	}
}

Next thing is that $error starts as an empty string and can be assigned either true or false, but is never actually used for anything. Since it’s not used we don’t need it and can throw it out:

function MrPropre($data){
	$data = filter_var($data, FILTER_SANITIZE_NUMBER_INT);
	if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
		return $data;
	} else {
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
		$data=""; 
	}
}

Now that last assignment of $data doesn’t do anything, since $data isn’t being used after that line, we can throw that out too:

function MrPropre($data){
	$data = filter_var($data, FILTER_SANITIZE_NUMBER_INT);
	if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
		return $data;
	} else {
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
	}
}

Now we see that $data is created and only accessed once, which means we can inline it, like so:

function MrPropre($data){
	if (filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false){
		return filter_var($data, FILTER_SANITIZE_NUMBER_INT);
	} else {
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
	}
}

Which has the additional benefit of not running that filter_var to sanitize for nothing of the input is invalid.

Now on to that condition, filter_var($data, FILTER_VALIDATE_INT) === 0 || !filter_var($data, FILTER_VALIDATE_INT) === false, that’s just a fancy way of saying anything that isn’t false, and can be simplified to filter_var($data, FILTER_VALIDATE_INT) !== false, that’s the exact same thing:

function MrPropre($data){
	if (filter_var($data, FILTER_VALIDATE_INT) !== false){
		return filter_var($data, FILTER_SANITIZE_NUMBER_INT);
	} else {
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
	}
}

Now lastly what I’d like to do is flip the if statement around so we start out with the unhappy path first, and continue with the happy path if the unhappy path isn’t being hit:

function MrPropre($data){
	if (filter_var($data, FILTER_VALIDATE_INT) === false){
		echo '<script>alert("Veuillez ne rentrer que des chiffres et remplir tous les champs svp") </script>';
		return;
	}

	return filter_var($data, FILTER_SANITIZE_NUMBER_INT);
}

This has a nice addition of getting rid of the else.

Structured this way it becomes easier to read in case you want to do several checks:

if X isn’t true we bail
if Y isn’t true we bail
if Z isn’t true we bail
in any other case we’re okay and return some value

Lastly, I’d rename the function to something more useful, like sanitizeInteger.

blabla296 · January 6, 2022, 10:54pm

Wow that’s so cool ! Thank you all so much for the answers & tips ! Really appreciated !

Ok so I tested it out, got some of my old issues back by checking if the values are !empty().
For example, there are 2 possible input values for $boodemiAdulte : 125 and 0. These values are sent from the value attribute of a radio button group si it’s either one of them. If the user chooses yes (aka 125), everything works well. Although, if he chooses no, then 0 is sent as a string and I’m getting errors in the math functions.

Therefore, I used isset instead, which looks like this :

	$floattypeLoc = (!empty($_POST['typeloc']) ? MrPropre($_POST['typeloc']) : '');
    $intAdulte = (!empty($_POST['nbAdultes']) ? MrPropre($_POST['nbAdultes']) : '');
    $intEnfants = (!empty($_POST['nbEnfants']) ? MrPropre($_POST['nbEnfants']) : '');
    $floatdemiAdulte = (isset($_POST['demipensionadulte']) ? MrPropre($_POST['demipensionadulte']) : '');
    $floatdemiEnfants = (isset($_POST['demipensionenfants']) ? MrPropre($_POST['demipensionenfants']) : '');
    $intAnnul = (isset($_POST['intAnnul']) ? MrPropre($_POST['intAnnul']) : '');
	$intPercent = (isset($_POST['intPercent']) ? MrPropre($_POST['intPercent']) : '');
	
	$intCostAdulte = (!empty($intAdulte) && isset($floatdemiAdulte) ? ssMult($intAdulte, $floatdemiAdulte) : '');
	$intCostEnfants = (!empty($intEnfants) && isset($floatdemiEnfants) ? ssMult($intEnfants, $floatdemiEnfants) : '');
	
	$intSsTotal = ((!empty($floattypeLoc) || !empty($intAnnul) || !empty($intCostAdulte) || !empty($intCostEnfants)) ? ssAdd($floattypeLoc, $intAnnul, $intCostAdulte, $intCostEnfants) : ''); 
	$intRemise = (!empty($intSsTotal) && isset($intPercent) ? getPercentOfNumber($intSsTotal, $intPercent) : '');
	$intTotal= (!empty($intSsTotal) || !empty($intRemise) ? ssAdd($intSsTotal, -$intRemise) : '');

This actually works fine, thanks all for taking the time to help

system · April 11, 2022, 10:37pm

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.