Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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();
}
share|improve this question

1 Answer 1

up vote 10 down vote accepted

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.

share|improve this answer
    
Do I need printf("%s instead of echo? –  glapa.wojciech Jun 22 at 19:12
1  
@glapa.wojciech You don't need it, but I do prefer string formating to lots of string concatenation. –  unholysampler Jun 22 at 22:03
    
@unholysampler: with echo, you don't need concatenation at all: echo $foo, ' a string ', $int, PHP_EOL; just pushes all the tokens to the echo language construct without concatenating the strings first... similar to how C++'s std::COUT works –  Elias Van Ootegem Jun 23 at 14:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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