3
\$\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\$
2
\$\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\$
  • \$\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 Jan 8 '15 at 6:49
1
\$\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 eseif 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\$
  • \$\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 Jan 8 '15 at 6:50
  • \$\begingroup\$ Ok. I will do what I can \$\endgroup\$ – Robert Rocha Jan 8 '15 at 14:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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