Bad practice!

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

User avatar
phice
Moderator
Posts: 1416
Joined: Sat Apr 20, 2002 3:14 pm
Location: Dallas, TX
Contact:

Post by phice »

LiLpunkSkateR wrote:and the double quotes are much easier IMO to see (especially when cluttered in other code)
Exactly.

It's easier to see quotes when you can just look for the " instead of sifting through the ' in the sentence.

ie:

Code: Select all

<?php
echo 'we were walking'.$through.'the water, '.$and.' then we reached our '.$point;
?>
vs.

Code: Select all

<?php
echo "we were walking " . $through . " the water, " . $and . " then we reached out " . $point;
?>
Spaces in quotes = good. Spaces in if/for/foreach/while = bad

ie:

Code: Select all

<?php
if( ( $this == "that" ) || ( $that == "this" ) ) {
   echo "this = that";
}
?>
... just ugly. ;)
Image Image
Steveo31
Forum Contributor
Posts: 416
Joined: Sun Nov 23, 2003 9:05 pm
Location: San Jose CA

Post by Steveo31 »

hawleyjr wrote: Which would you rather read?
I would rather read

Code: Select all

<table width="100%" border="1">
    <tr>
        <td><?php echo 'phpStuff with '.$var; ?></td>
    </tr>
</table>
;) :D

I am starting to learn to use single quotes for strings. I agree in that it keeps code cleaner, variables out of strings, etc. The tabs issue is a new one... I've always done a tab. It's easier and faster than hitting <space> 4 times.
d3ad1ysp0rk
Forum Donator
Posts: 1661
Joined: Mon Oct 20, 2003 8:31 pm
Location: Maine, USA

Post by d3ad1ysp0rk »

I know why I use double quotes now!

You can't use \n w/single quotes! :P
malcolmboston
DevNet Resident
Posts: 1826
Joined: Tue Nov 18, 2003 1:09 pm
Location: Middlesbrough, UK

Post by malcolmboston »

this annoys me to death

Code: Select all

$user = "";
$pass = "";
$server = "";
$db = ""; 
$link = mysql_connect($server,$user,$pass); 

if(!$link) 
die("could not connect to MySQL"); 
print "Successfully conected to Server<p>"; 

//Connect to database 

mysql_select_db($db:, $link) 
or die ("Couldn't open $db: ".mysql_error() );
why not just use

Code: Select all

$user = "";
$pass = "";
$server = "";
$db = "";
mysql_connect ($server, $user, $pass) or die(mysql_error());
mysql_select_db($db) or die(mysql_error());
dunno why i hate the first example, its just lots of completely unnecessary code
malcolmboston
DevNet Resident
Posts: 1826
Joined: Tue Nov 18, 2003 1:09 pm
Location: Middlesbrough, UK

Post by malcolmboston »

also the fact theres no PHPc
meaning no client-side version of PHP, i know it will never happen but you could do some truly amazing things if there was...........
User avatar
twigletmac
Her Royal Site Adminness
Posts: 5371
Joined: Tue Apr 23, 2002 2:21 am
Location: Essex, UK

Post by twigletmac »

IMHO, single over double quotes is a question of consistency - choose a way and stick with it (personally I like my variables out of strings so I can see them). You shouldn't really ever need to use things like \n in quoted strings, heredoc seems much more suited to that type of thing.

Mac
Roja
Tutorials Group
Posts: 2692
Joined: Sun Jan 04, 2004 10:30 pm

Post by Roja »

malcolmboston wrote:this annoys me to death

Code: Select all

$user = "";
$pass = "";
$server = "";
$db = ""; 
$link = mysql_connect($server,$user,$pass); 

if(!$link) 
die("could not connect to MySQL"); 
print "Successfully conected to Server<p>"; 

//Connect to database 

mysql_select_db($db:, $link) 
or die ("Couldn't open $db: ".mysql_error() );
why not just use

Code: Select all

$user = "";
$pass = "";
$server = "";
$db = "";
mysql_connect ($server, $user, $pass) or die(mysql_error());
mysql_select_db($db) or die(mysql_error());
dunno why i hate the first example, its just lots of completely unnecessary code
I'll tell you why I would use a variation of the first example.. because it would be easier to expand. In other words, imagine you wanted to output a pretty html page for the database connection failing.

In a slight variation of the first example:

Code: Select all

if(!$link) 
{
ob_start;
$smarty = new smarty;
$smarty->output('db-error.tpl');
ob_end;
//die("could not connect to MySQL"); 
}
Obviously non-functional code, but you get the idea.. multiple commands can be added without any muss/fuss. In the second example, you have to rewrite the "either/or" code - possibly changing it to the above form.

I always use full bracketed logic sections for *all* decision branches - ensuring the code can easily be changed in the future and extended in new directions with a minimum of changes hitting CVS. (I'm a huge CVS fan, and want each change to be unique, specific, and obvious whenever possible).
malcolmboston
DevNet Resident
Posts: 1826
Joined: Tue Nov 18, 2003 1:09 pm
Location: Middlesbrough, UK

Post by malcolmboston »

malcolmboston wrote:this annoys me to death

Code: Select all

$user = "";
$pass = "";
$server = "";
$db = ""; 
$link = mysql_connect($server,$user,$pass); 

if(!$link) 
die("could not connect to MySQL"); 
print "Successfully conected to Server<p>"; 

//Connect to database 

mysql_select_db($db:, $link) 
or die ("Couldn't open $db: ".mysql_error() );
why not just use

Code: Select all

$user = "";
$pass = "";
$server = "";
$db = "";
mysql_connect ($server, $user, $pass) or die(mysql_error());
mysql_select_db($db) or die(mysql_error());
dunno why i hate the first example, its just lots of completely unnecessary code

I'll tell you why I would use a variation of the first example.. because it would be easier to expand. In other words, imagine you wanted to output a pretty html page for the database connection failing.

In a slight variation of the first example:

Code: Select all

if(!$link) 
{
ob_start;
$smarty = new smarty;
$smarty->output('db-error.tpl');
ob_end;
//die("could not connect to MySQL"); 
}
i always use customised error pages but heres how i do it

Code: Select all

<?php
include ("cfg/error_cfg.php");
// defined in error config would be 2 variables
// $connect_error = "<html>...<body><h1>some text</body></html>";
// $database_error = "<html>...<body><h1>some text</body></html>";
$user = "";
$pass = "";
$server = "";
$db = "";
mysql_connect ($server, $user, $pass) or die($connect_error);
mysql_select_db($db) or die($database_error);
?>
still gives you customisable error messages (and pages deciding on whether you decide to create the variables with tags in them) without the unnecessary amount of code,

Just my personal preference anyway....
Roja
Tutorials Group
Posts: 2692
Joined: Sun Jan 04, 2004 10:30 pm

Post by Roja »

malcolmboston wrote: i always use customised error pages but heres how i do it

Code: Select all

<?php
include ("cfg/error_cfg.php");
// defined in error config would be 2 variables
// $connect_error = "<html>...<body><h1>some text</body></html>";
// $database_error = "<html>...<body><h1>some text</body></html>";
$user = "";
$pass = "";
$server = "";
$db = "";
mysql_connect ($server, $user, $pass) or die($connect_error);
mysql_select_db($db) or die($database_error);
?>
still gives you customisable error messages (and pages deciding on whether you decide to create the variables with tags in them) without the unnecessary amount of code,

Just my personal preference anyway....
But then you are just avoiding the issue - which is that you cant expand the functionality of the "if/then/else/or/when" logic branches.

What if you wanted to for example, write to an error log file, then display the page, and then do a redirect to a report-bug-to-admin page? Thats three actions - you'd need brackets again, and would have to rewrite the code.

Thats why I always fully expand logic branches. Its longer, absolutely. But it gives fantastic flexibility, and - FOR ME - its easier to read and troubleshoot.
malcolmboston
DevNet Resident
Posts: 1826
Joined: Tue Nov 18, 2003 1:09 pm
Location: Middlesbrough, UK

Post by malcolmboston »

lol, well thats why your better than me :roll:
Roja
Tutorials Group
Posts: 2692
Joined: Sun Jan 04, 2004 10:30 pm

Post by Roja »

malcolmboston wrote:lol, well thats why your better than me :roll:
Not at all.. there are many that could (quite correctly) argue that my code is not 'easy to read', not 'tight', and way too over-generalized (see refactoring for purpose).

Everyone has their own "best" way of doing things. Some actually *are* truly better across the board, but that is fairly rare. :)

I've said it before, and I'll say it again - if there was only one RIGHT way to do things, php would be Python. :)
Steveo31
Forum Contributor
Posts: 416
Joined: Sun Nov 23, 2003 9:05 pm
Location: San Jose CA

Post by Steveo31 »

In addition to malcomboston, overkill use of variables:

Code: Select all

<?php

$user = "";
$pass = "";
$server = "";
$db = "";
$table = "";
mysql_connect ($server, $user, $pass) or die(mysql_error());
mysql_select_db($table);

?>
Wow.. I find it way faster, more efficient to do

Code: Select all

<?php

mysql_connect('server', 'user', 'pass') or die(mysql_error());
mysql_select_db('tablename');

?>
:)
User avatar
Ixplodestuff8
Forum Commoner
Posts: 60
Joined: Mon Feb 09, 2004 8:17 pm
Location: Queens, New York

Post by Ixplodestuff8 »

phice wrote:
Spaces in quotes = good. Spaces in if/for/foreach/while = bad

ie:

Code: Select all

<?php
if( ( $this == "that" ) || ( $that == "this" ) ) {
   echo "this = that";
}
?>
... just ugly. ;)
That's how I do it, it looks cleaner to be and less jumbled up to me with the spacing, but I don't think either way is bad practice, just a matter of preference ;)
d3ad1ysp0rk
Forum Donator
Posts: 1661
Joined: Mon Oct 20, 2003 8:31 pm
Location: Maine, USA

Post by d3ad1ysp0rk »

I do a small amount of spacing in statements:

Code: Select all

if(($this == "that") || ($that == "this")){
echo "this = that";
}
for($i=100;$i>0;$i--){
echo $i . " bottles of beer on the wall, " . $i . " bottles of beer. <br />";
echo "knock one down, spin it around, " .($i-1). " bottles of beer on the wall!<br />";
}
while($var!="value"){
//do stuff
}
foreach($val as $var){
//weeee
}
d3ad1ysp0rk
Forum Donator
Posts: 1661
Joined: Mon Oct 20, 2003 8:31 pm
Location: Maine, USA

Post by d3ad1ysp0rk »

edit to above for loop:

Code: Select all

for($i=100;$i>0;$i--){
  if($i != 1){ 
    echo $i . " bottles of beer on the wall, " . $i . " bottles of beer. <br />"; 
    echo "knock one down, spin it around, " .($i-1). " bottles of beer on the wall!<br />"; 
  }
  else {
    echo $i . " bottle of beer on the wall, " . $i . " bottle of beer. <br />";
    echo "knock it down, spin it around, no more bottles of beer on the wall!";
  }
}
;)
Post Reply