4
\$\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\$
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();

 }
| improve this answer | |
\$\endgroup\$
  • 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 Jan 8 '15 at 6:49
3
\$\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.

| improve this answer | |
\$\endgroup\$
  • \$\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\$ – Your Common Sense Mar 12 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 Mar 12 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\$ – Your Common Sense Mar 12 at 21:30
  • \$\begingroup\$ @YourCommonSense Fair enough, I deleted that part from my answer. \$\endgroup\$ – potato Mar 12 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\$ – Your Common Sense Mar 13 at 5:09
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 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;

}
| improve this answer | |
\$\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
1
\$\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 does! 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();
    $stmt->close;
    return $name;
}
| improve this answer | |
\$\endgroup\$

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.