1
\$\begingroup\$

Lets say I build functions.php and I have 3 functions inside of it that I use. Lets say that I build a 4th function, but what I really want to do is simply put the instructions of function 4 into function 1 for example. Is there anything wrong with this?

Consider my example here, where I have included a function inside of a function. I am aware that the variables inside of a function are local, and that you cannot call a variable inside a function outside of it, however I have tested this with a simple echo test and it seems to work. Is there anything wrong with doing this? If so, can you explain how you might go about doing this generally? You do not have to be specific.

functions.php

function validate_registration($password, $confirmpassword, $email) {
    if ((empty($email)) || (empty($password)) || (empty($confirmpassword))) {
        registration_form();
        echo "Please enter the required information:";
    }
    elseif ((filter_var($email, FILTER_VALIDATE_EMAIL)) && ($password == $confirmpassword)) {
        input_registration($email, $password);
    }
    elseif ((filter_var($email, FILTER_VALIDATE_EMAIL)) && ($password !== $confirmpassword)) {
        registration_form();
        echo "Your password does not match!";
    }
    else {
        registration_form();
        echo "You have not entered a valid email address!";
    }

}

the input_registration function:

function input_registration ($email, $password) {
$email_clean = htmlspecialchars($email);
$password_clean = htmlspecialchars($password);

$hash = md5($password_clean);

$db = new PDO("mysql:host=localhost;dbname=users", "root", "")

$statement = $db->prepare("INSERT into userinfo(email, hash) VALUES (:email, :hash)")

//continue with inputting info into database
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Off-topic, but an important sidenote: don't use MD5 for password hashing. Use Bcrypt, Scrypt, or PBKDF2 \$\endgroup\$
    – 1615903
    Jun 14, 2013 at 7:48
  • \$\begingroup\$ Show this simple echo test please \$\endgroup\$
    – user4035
    Jun 14, 2013 at 8:06
  • \$\begingroup\$ When I say simple echo test, I mean to say that if everything else is working properly and the logic works its way out correctly then the end result is just an command like echo "Looks good" instead of continuing with an SQL command I haven't written yet. \$\endgroup\$
    – the tao
    Jun 14, 2013 at 22:24

1 Answer 1

2
\$\begingroup\$

It is absolutely ok to call functions from other functions. There is nothing wrong with that. However, there's room for improvements in your code.

function.php:

  1. There is no need to put simple expressions within parenthesis in your conditions.
  2. The order of your conditions is irritating (negative, positive, negative, negative). Group them properly. That also helps to simplify the conditions.

    function validate_registration($password, $confirmpassword, $email)
    {
        if (empty($email) || empty($password) || empty($confirmpassword)) {
            registration_form();
            echo "Please enter the required information:";
        } elseif (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            registration_form();
            echo "You have not entered a valid email address!";
        }  elseif ($password !== $confirmpassword) {
            registration_form();
            echo "Your password does not match!";
        } else {
            input_registration($email, $password);
        }
    }
    

input_registration:

  1. Escape values on output using the right escaping function. Since you use prepared statements, no extra escaping is needed for the database.
  2. Create the database connection in central place, so you don't have to change several files and functions, if the database (or your password) changes.

    function input_registration($db, $email, $password)
    {
        $hash = md5($password); // Consider using a better hash algorithm
        $statement = $db->prepare("INSERT INTO userinfo(email, hash) VALUES (:email, :hash)");
    
        // Continue with inputting info into database
    }
    

Of course you have to change validate_registration to support the injection of the database dependency.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, I did not know you could use the negative logic for a filter, putting ! in front of a filter. The reason for my poor logic was because I did not know that. \$\endgroup\$
    – the tao
    Jun 14, 2013 at 21:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.