5
\$\begingroup\$

I'm programming code with MySQLi prepared statements. I'm a beginner and just want to ask to experts if my code is correct.

function getPaymentMethodName( $id ) {
    global $mysqli;
    if ( $stmt = $mysqli->prepare( 'SELECT name FROM `payment_methods` WHERE id = ?' ) ) {
        if ( $stmt->bind_param( 'i', $id ) ) {
            if ( $stmt->execute() ) {
                $stmt->bind_result( $name);
                if ( $stmt->fetch() ) {
                    $stmt->close();
                    return $name;
                }
            }
        }
    }
    $stmt->close();
    return false;
}
\$\endgroup\$

4 Answers 4

4
\$\begingroup\$

Let mysqli do the error detection for you:

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

This makes mysqli throw exceptions when errors occur inside mysqli function calls, instead of failing silently and creating the need for you to manually detect these errors. You only need to call mysqli_report once before making mysqli function calls.

\$\endgroup\$
5
  • \$\begingroup\$ Although the recommendation to use exceptions is correct, DO NOT surround the whole function with a single try/catch block.It just makes no sense \$\endgroup\$ Commented Mar 12, 2020 at 19:39
  • \$\begingroup\$ @YourCommonSense Doesn't it make sense for his case if he just wants to return $name or on failure return false? \$\endgroup\$
    – potato
    Commented Mar 12, 2020 at 20:48
  • \$\begingroup\$ Nobody ever wants to return false on failure. Especially a beginner who just have no idea what does he want. \$\endgroup\$ Commented Mar 12, 2020 at 21:30
  • \$\begingroup\$ @YourCommonSense Fair enough, I deleted that part from my answer. \$\endgroup\$
    – potato
    Commented Mar 12, 2020 at 21:49
  • \$\begingroup\$ Thank you. Wrapping a code in a try catch to catch a possible error immediately is a practice as bad as frequently it is used. As a rule you don't want to catch, least you want to let it get away silently. \$\endgroup\$ Commented Mar 13, 2020 at 5:09
4
\$\begingroup\$

Let's sum up all the disparate suggestions from different answers.

  1. Globals are bad. Pass a connection variable as a function parameter.
  2. Such a staircase of conditional operators is a sign of bad design.
  3. Checking a statement for the error but doing nothing when it happens is an awful practice. Looks like you are trying to avoid getting the error message, but error messages in fact are your friends, not foes! Always get as much error information as possible, it will help you to fix it!
  4. Writing such a wall of code to perform a single primitive SQL query is just INSANE
  5. Tell mysqli to throw exceptions on errors.

Then have a tidy function like this

function getPaymentMethodName($mysqli, $id) {
    $stmt = $mysqli->prepare( 'SELECT name FROM `payment_methods` WHERE id = ?');
    $stmt->bind_param( 'i', $id );
    $stmt->execute();
    $stmt->bind_result($name);
    $stmt->fetch();
    return $name;
}
\$\endgroup\$
2
  • \$\begingroup\$ $stmt->close; should $stmt->close(); \$\endgroup\$ Commented Nov 30, 2022 at 11:23
  • \$\begingroup\$ BTW @Mantykora7 the answer was edited shortly after your suggestion \$\endgroup\$ Commented Mar 7, 2023 at 14:51
3
\$\begingroup\$

This is really pretty good. I would do a few things slightly differently.

  • Don't use globals. Pass the $mysqli parameter into your function. Globals cause nightmares for maintainers.

  • Do an explicit error check after each mysqli method invocation, and get rid of the cascading if statements. Check for a false return, then issue a message. In your logic you might also emit the error message and then return false. But you need to announce the error somehow or other or you'll have trouble debugging things.

For example: (Note, using die is a little sloppy, but not as bad as failing to report errors.)

 $stmt = $mysqli->prepare( 'SELECT name FROM `payment_methods` WHERE id = ?' );
 if ($stmt === false) {
         die "prepare failed:" . mysqli_connect_error();

 }
\$\endgroup\$
1
  • 1
    \$\begingroup\$ It's highly likely the OP was simply doing this for a non-production project, however I would still advise against using die. It's a bad habit to get into. It kills the page right there in execution, which clears any attempt at being user friendly, plus there are many alternate options. \$\endgroup\$
    – Alex L
    Commented Jan 8, 2015 at 6:49
2
\$\begingroup\$

Globals are dangerous.

The reason for this is that they can easily be overridden either by your code or somebody else's code that has the same name. The bug from global variables can be hard to trace. Good article: http://c2.com/cgi/wiki?GlobalVariablesAreBad

It would actually be better in each if elseif clause to return an exception to your code if there was a problem, handling the problem gracefully and providing you with detailed information. http://php.net/manual/en/class.mysqli-sql-exception.php

function getPaymentMethodName( $id, MySqli $mysql )
{
    # prepare
    if( ! ($stm = $mysql->prepare( "SELECT `name` FROM `payment_methods` WHERE id = ?") )
    {
        return false;
    } #bind params
    elseif( !$stm->bind_param( 'i', $id ) )
    {
        return false;
    } #execute
    elseif( !$stm->execute() )
    {
        return false;
    }

    #bind
    $stm->bind_result( $name );

    #fetch
    $stm->fetch();

    #close
    $stm->close();

    #return value
    return $name;

}
\$\endgroup\$
2
  • \$\begingroup\$ It would be excellent if you could explain to the OP the changes you've made and the reasoning behind them. And then bonus points if you reference others to back up your reasoning. \$\endgroup\$
    – Alex L
    Commented Jan 8, 2015 at 6:50
  • \$\begingroup\$ Ok. I will do what I can \$\endgroup\$
    – Robert
    Commented Jan 8, 2015 at 14:11

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.