HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpModerate

Have I prepared this prepared statement well?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisstatementpreparedwellhave

Problem

This is a prepared statement (I think). Have I done this well?

function wyswietl($rekord, $tabela) {
    $lacz = lacz_bd();

    if (!$zap_preparowane = $lacz->prepare("SELECT {$rekord} FROM {$tabela} where email = ?"))                 {echo "Prepare nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->bind_param("s",$_SESSION['prawid_uzyt']))                                           {echo "BindPar nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}     
    if (!$zap_preparowane->execute())                                                                          {echo "Execute nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->store_result())                                                                     {echo "StoreRe nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
    if (!$zap_preparowane->bind_result($binded_result))                                                        {echo "BindRes nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;}
        while ($zap_preparowane->fetch()) {
             echo $binded_result;     //htmlspecialchars? printf("%s ?
        }
    $zap_preparowane->free_result();
    $zap_preparowane->close();

    $lacz->close();
}

Solution

Single line if statements combined with column aligned spacing:

Never ever do this. Never. A quick glance at the code you posted looks like this:

function wyswietl($rekord, $tabela) {
    $lacz = lacz_bd();

    if (!$zap_preparowane = $lacz->prepare("SELECT {$rekord} FROM {$tabela} where email = ?"))
    if (!$zap_preparowane->bind_param("s",$_SESSION['prawid_uzyt']))
    if (!$zap_preparowane->execute())
    if (!$zap_preparowane->store_result())
    if (!$zap_preparowane->bind_result($binded_result))
        while ($zap_preparowane->fetch()) {
             echo $binded_result;     //htmlspecialchars? printf("%s ?
        }
    $zap_preparowane->free_result();
    $zap_preparowane->close();

    $lacz->close();
}


My first thought was "why are all these if statements chained together with no indentation?". Then I noticed the horizontal scrollbar. Now, I see that the code does something much different. You should not make someone reading your code have to work this hard to know what the code is doing.

On top of that, I don't like the use of single line if statements and column aligned spacing, in general. Single line if statements lead to people missing the executed line because a line for the if clause and a line for the true clause is so much more prevalent. Column aligned spacing is easy enough to write the first time. But when you have to come back and edit the code, you might have to make changes to lines just to maintain the spacing.

Repeated code:

The code that prints the error message is repeated code. You can easily make it a function.

function log_error($zap_preparowane, $header) {
  echo $header ." nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;
}


Now if you want to change how the errors are handled, it is easily done in one place.

The error handling doesn't stop execution of the code. If you fail to properly prepare the SQL statement, it is unlikely that anything else will execute correctly.

Code Snippets

function wyswietl($rekord, $tabela) {
    $lacz = lacz_bd();

    if (!$zap_preparowane = $lacz->prepare("SELECT {$rekord} FROM {$tabela} where email = ?"))
    if (!$zap_preparowane->bind_param("s",$_SESSION['prawid_uzyt']))
    if (!$zap_preparowane->execute())
    if (!$zap_preparowane->store_result())
    if (!$zap_preparowane->bind_result($binded_result))
        while ($zap_preparowane->fetch()) {
             echo $binded_result;     //htmlspecialchars? printf("%s ?
        }
    $zap_preparowane->free_result();
    $zap_preparowane->close();

    $lacz->close();
}
function log_error($zap_preparowane, $header) {
  echo $header ." nieudane: (" . $zap_preparowane->errno . ") " . $zap_preparowane->error;
}

Context

StackExchange Code Review Q#54958, answer score: 10

Revisions (0)

No revisions yet.