IF setting variable, false first

[Readie]

I found this out yesterday by total guesswork, one of my annoyances with my code is I found I quite often ended up with something like this:

$sql = 'SELECT * FROM table';
$result = mysql_query($sql);
if($res = mysql_fetch_assoc($result)) {
  // Hundreds
  // of
  // lines
  // of
  // code
} else {
  echo 'Error';
}

Which makes it a bit difficult to see what that error is in response to. So, I tried this:

$sql = 'SELECT * FROM table';
$result = mysql_query($sql);
if(!($res = mysql_fetch_assoc($result))) {
  echo 'Error';
} else {
  // Hundreds
  // of
  // lines
  // of
  // code
}

Which, I feel, is a lot easier to read when you have a one-liner error message, and a much larger success script.

[rocknbil]

One of the standing rules in Perl, if your subroutine (function in PHP) is more than 25 lines, it needs to be split out into another subroutine.* :-) If you have hundreds of lines, wrapped in an if, maybe that's the legibility problem, not where your error pops in.

My bowels go weak when I have to fix code that has an "if" surrounding several hundred lines of code. :-( It's like going down the rabbit hole, and nowhere near as wondrous . . .

When I force myself to split out long blocks into functions I keep asking, "can this chunk be used elsewhere?" Half the time, that's a yes.

while($res = mysql_fetch_assoc($result)) {
$output .= output_rows($res,$some_parameter1,$some_parameter2);
}

if ($output) { echo '<table>' . $output . '</table>'; }
else { echo '<p>No results</p>'; }

... where $res accepts an array, and might do any number of things based on the additional parameters, the most likely of which is return a string containing a complete html row.

*Truth be told . . . on really long days or really frustrating circumstances, or a complex set of control structures makes it big no matter what, 25 is not enough, might get stretched to more, but one has to at least try, it always helps.

[Matthew1980]

Hi all,

It depends on the person and how their coding style is. I was recently told (by my java colleague) that good coding requires no commenting, the code should read by what it does.

My preference is to put the clause in the else.

Cheers.
MRb

[penders]

good coding requires no commenting

However, I don't think that was a suggestion not to comment good code! :)

[Readie]

Rock:

I tend to only use the if($res = mysql_fetch_assoc) when I've made sure only 1 line from the database will be returned. Otherwise I tend to do:

$sql = 'SELECT * FROM table';
$result = mysql_query($sql);
if($rows = mysql_num_rows($result)) {
  for($i = 0; $i < $rows; $i++) {
    // Stuff
  }
}

Matt:

Code should always be commented - you won't be able to remember exactly what each part of a script is doing when you come back a few months later, so comments will help you remember quicker.

Also, if you are in an environment where other people may need to alter your code, comments are a huge help as they don't have to try and follow unfamiliar logic.

I was driven to finding the above method because my (not quite manager... Team leader perhaps?) critisized the legibility of my code.

[rocknbil]

Right, I was just using "while" as an example, the point was, it's what's between the blocks; if it's 100 lines, it will make life easier to pull some of those out into separate functions. This not only helps legibility, it helps isolate problems that might come up.

Someone will always find something to pick at with your code. If the input is meant to improve style, you should listen, but IMO "legibility of your code" is arguable. I've found that many times, collaborators will resort to these kinds of comments if they don't want to take time to understand the logic (read: "you're an idiot, I'm the god of coding, all hail my style . . . ")

It's like dealing with musicians. There are those that can work with anyone and see the value in learning another's style, this is called eclecticism (or a lot like it.) The group as a whole rubs off on each other, everyone learns something new, everyone benefits, and the end product is unique.

Then there's those that no one can work with and they always seem to be on a solo gig with no backup. And just like musicians, it's tough to find coders you can work with, especially if you're thrown together without a choice.

[Matthew1980]

Hi there Readie,

Clarification: My colleague declared that! I personally comment where its needed for clarity, and for longevity. Come back to something after a few months not looking at it, you need that reminder of why you did what you did - that is my general rule of thumb.

I was driven to finding the above method because my (not quite manager... Team leader perhaps?) critisized the legibility of my code.

Maybe he's just seeing how you respond to criticism, to be truthful, it matters not how you structure your clauses so long as it works But as Rocknbil hints at - everyone has their own way of doing things - and as I say to our ISO/BSI inspector, no two people work the same way, it's impossible, if that happened, they would have a hard-drive for a brain and be called D.A.R.Y.L !

Rocknbil: Someone will always find something to pick at with your code Absolutely true, I work with three other coders (all different languages, but we all started with C) and we constantly criticise each other ;) One guy, practically eats/sleeps/dreams assembly language, and you HAVE to comment EVERY line in order to understand it, fortunately I am the only php coder <phew> but because it's close to java & C the other's still criticise me, sometimes with good reason.

Cheers,
MRb

[enigma1]

Try to make your code modular it will help you to read it, utilize it and manage it more efficiently. So rearranging your example a bit:

function process_something() {
$result = false;
$sql = 'SELECT * FROM table';
$result = mysql_query($sql);
if( !mysql_num_rows($result)) return $result;

$result = true;
// Process Query
while($res = mysql_fetch_assoc($result)) {
......
}
return $result;
}

So by returning on an empty query in this example, you avoid the long if/else clause which is difficult to track.

[Readie]

Rock:

Hmm, I can see your point. Sometimes when my manager is rushing me to meet a deadline for a low-priority client, throwing out something *that works* is more important than doing good coding - this is just a quick and simple technique for improving legibility, ideal for such situations.

Rock & Matt:

Mmm, perhaps he may have just been nitt-picking, but he has a point. Some of the code I did before cleaning up my style has meant that, if any of it breaks (and I'm smugly pleased that it hasn't yet) then it would have to come straight to me for fixing, simply because I'm the only one who could follow the code without taking hours over it.

Just Matt:

Point your colleague at my rant then :P

Engima:

That is an interesting way of doing things... I'm unlikely to switch to such a setup though, through stubborness and a desire for consistency - we do bespoke work ontop of a pre-existing framework when developing sites, and to make following the logic easier for each other we try and keep the coding style similar-ish to what's already there.

[Matthew1980]

we do bespoke work ontop of a pre-existing framework when developing sites, and to make following the logic easier for each other we try and keep the coding style similar-ish to what's already there

Good practice, no point trying to re invent the wheel every time there is a new client on the books.

Point your colleague at my rant then :P

She's a very stubborn java coder & very much a stickler for convention/efficiency/neatness, but she is a great person away from the desk! - I usually just nod for a quieter life. Lol!

Cheers,
MRb