Question

I've learned from Stack Overflow that some of the code I've been writing so far is considered "bad practice". Like using tons of global variables, create_function() etc. I'm curious to know more about this.

Post examples of bad practices in PHP and why. Please only post one bad practice per answer, this way the worst can get the most votes.

Answer 1

for($i = 0; $i < count($array); $i++)

God kills a kitten every time you call count() inside a loop.

Answer 2

Failing to filter/escape database input or screen output when submitted by users:

mysql_query("INSERT INTO table (name, email) VALUES ({$_POST['name']}, {$_POST['email']})");

This is downright dangerous.

Answer 3

Using the @ modifier to suppress warnings & errors, particularly when talking to a database. The proper way is to make sure errors aren't displaying on screen, but are being logged where you address them.

@mysql_connect(...)

Answer 4

Not indenting, in my opinion.

function awesome($someone){
if($someone == 'you')
{
echo 'it is you';
}
else
{
switch($someone){
case 'him':
echo 'that guy';
break;
case 'her':
echo 'that girl';
break;
default:
echo 'someone else';
break
}
}
}

It's just impossible to read if you have to update someone's old code as I do all the time in my horrible job.

Answer 5

Mixing business logic with presentation logic. This has its roots in PHP being in the beginning pretty much a templating system for HTML. Unfortunately you still see a lot of code like:

<?php session_start(); ?>
<html>
<?php connectToDataBase() ?>
<head>
<?php doSomeStuffNotRelatedToDisplayingHtmlAtAll() ?>
<table>
 <?php $result = mysql_query()
  while($row = mysql_fetch_assoc($result)) {
   if($row['condition']) {?> <td></td> <?php }
  }
?>

</table>
...

Answer 6

Using register_globals = On. This results in big headache. http://php.net/manual/en/security.globals.php

Answer 7

Worst practice ever?

echo "$var";

It really hurts me when I see it somewhere.

Answer 8

Simple: magic_quotes ^[1] being turned on on.

Why? Because escaping cannot be done automatically. People think they are safe since it's on, and wind up with \ in their database, or major vulnerabilities because it doesn't work how they think it does...

[1] http://www.php.net/manual/en/security.magicquotes.what.php

Answer 9

commenting the obvious

// return false

return false;

using eval()

too many arguments, just pass in an object or array

// too many arguments
public function doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6)
{
   ...
}

// better, also put type and maybe some comments on usage for others.
public function doSomething(array $args)
{
   ...
}

not quoting array keys

//bad
echo $yourArray[someKey];

//good
echo $yourArray['someKey'];

functions that do too much, "Swiss Army function (?)"

//bad
public function someFunction(string $arg)
{
   switch($arg)
   {
      case 'update':
         ...
         break;

      case 'delete':
         ...
         break;

      default:
         ...
         break;
   }
}

// good, separate functions-- keep it clean and simple
public function update(){...}

public function delete(){...}

public function someFunction(){...}

Answer 10

I can't stand it when people abbreviate variables.... avoid abbreviating to make your code more readable.

$ml = 123;
for($x = 0; $x < $ml; $x++) {
    //code here
}

You have NO IDEA what $ml is... why not just called it $minimumLines or something where a developer can look at your code and know what that value is.

Answer 11

Some of the things you have been told to be "bad practices" are just memes gone awry and overgeneralized on Stackoverflow. Here are a few antithesis:

Effective PHP Cargo Cult Programming ^[1]

(I'm so going to get downvoted.)

[1] http://web135.srv3.sysproserver.de/milki.erphesfurt.de./EPCCP.pdf

Answer 12

Failing to escape user submitted data:

echo $_POST['key'];

Answer 13

Believe the user is your friend and he'll never try to hack you.

Remember:

You HAVE to assume your visitor is a maniac serial killer, out to destroy your application. And you have to prevent it

Answer 14

Debugging and writing code without E_NOTICE ^[1] active.

Why: A fair number of developers (including those who should know better) think that notices are not important and hence can be ignored. However, a fair number of bugs sneak through because of miss-spellings or typos. Undefined variables are errors and should be treated as such.

Better: Always write and debug code with all errors levels running (E_ALL). Only on production servers should you limit the error level as appropriate.

Mario's edit to add a counter-argument: That's plain wrong. PHP is a dynamic language and dynamically typed. Handling undefined variables is one of the core features, as it's optimized for receiving forms. Notices are debug messages and not errors (potential problems != faults). They should be turned on in the development stage. They should neither be ignored, nor habitually suppressed, but evaluated in each specific case (there are syntax notices and informational notices, should not be lumped together). Using isset($_GET["var"]) ? $_GET["var"] : NULL is functionally eqivalent to @$_GET["var"] - everything else is a lie. Or a micro optimization (which are only ever appropriate in loops).

[1] http://www.php.net/manual/en/errorfunc.constants.php

Answer 15

When i see this i wish i never started using PHP :

function something ($optional_var1 = 1, $optional_var2 = 2, $must_provide_var) {
    ...
}

Or another thing is when people abuse dynamic typing : $a = "1" + "2"; and expect integer

Answer 16

Whoa, this list could grow really really long as ways of screwing things up are vast.

From those commonly encountered:

Mixing functionality (typically db queries) with HTML
Not sanitizing inputs (including type and range checking, which is commonly ignored)
Extremely poor design decisions due to poor knowledge of both PHP and OOP principles
Reinventing the wheel instead of searching in the common library
Nesting DB queries (made me punch a guy once)
Poor naming conventions
Misleading documentation
Not taking advantage of subtle nuances of PHP (for example single and double quoted strings)
Relying on multiple switches instead of using introspection and metaprogramming
Abuse of poorly understood libraries
Obfuscation of simple code by overusing design patterns and best practises
Not adhering to corporate/project coding standard, mixing various styles
Wasting of memory by not taking advantage of references
Crippling code with unbecoming references
Using deprecated features
Not choosing appropriate tools (frameworks, ORMs)
Mixing exceptions based code with return values indications (awwww)
Ignoring exceptions or handling them sluggishly (leading to unpredictable behaviour and difficult debugging)
Improper use of testing tools

... I could go on and on. I've seen terrible things :/. Just use your brain and get to know your tools and you are going to be OK

Answer 17

Using eval() ^[1].

Always remember: eval() is evil().

Just like this guy:

enter image description here

[1] http://php.net/manual/en/function.eval.php

Answer 18

Passing every input inside a 'sanitizer' function, without care about what that input mean.

$an_aspected_integer_id = my_super_mega_iper_sanitizer_function($_GET['id']);
// (int)$_GET['id']; looks bad!?

Using a lot of if-else short declaration

 $myvar = $x == $y ? $y <= $z ? $y : $x != $j ? $k >= $j ? $k : $j : $x : "omg_i_did_again" ;

Answer 19

Writing a 30,000 line Web application, with:

Zero defined functions (only functions used are built-ins)
Direct variable insertion into SQL strings without quoting
Circular includes
Includes-as-a-function-call
Recursive Includes

I could go on and on and on.

Maybe I could sum it up simply and generically: Writing it ugly, bad. Not bothering to incrementally improve: just plain offensive.

Answer 20

People should stop using die() like, a typical example die("MySQL error:".mysql_error()) use PDO and try/catch exceptions

Answer 21

Writing all your strings like:

'foo'.$bar.'baz'.$biz.'buz'."\n"

Because ' quoted strings are faster* than " quoted strings.

The bottom line, write readable code. Write appropriate code. Don't worry about premature optimizations and micro-optimizations.

* Note: They aren't really faster. The benchmarks between them are quite inconsistent and both are well within the margin of error of each other.

Answer 22

Really, I'm not making this up...

if($_SESSION['username']=='name-of-lead-developer')echo "<a href='".$URlBuildString."' target='_blank'>Here it is</a>";

This wasn't git-stashed, this was live, in production, code.

Answer 23

Using echo, return, ... with braces, like return(5);
Adhering to a coding standard over then Zend or PEAR or a close derivate
Using a non-English language in code (well, strings excluded)
Using eval instead of PHP dynamic language features, like $function() or new $class (eval is okay to use, but no if there is a native possibility to do it.)
if ($a == $b) { return true } else { return false }

Answer 24

Not understanding the different ways PHP handles references ^[1] and blindly using & for creating references with objects.

Reason: passing around variable references can get quite messy and lead to very hard-to-debug errors.

Instead, just let PHP internally handle the reference for you. It's far cleaner, and far easier to debug when something goes wrong...

[1] http://stackoverflow.com/questions/3611986/in-php-can-someone-explain-cloning-vs-pointer-reference/3612129#3612129

Answer 25

Not PHP specific, but uninitialized variables:

if ($foo) {
    $bar = $foo;
}
echo $bar;

Why: it makes debugging much harder while making the code less clear.

Instead, always pre-declare your variables (where you're going to conditionally assign them):

$bar = '';
if ($foo) {
    $bar = $foo;
}
echo $bar;

Answer 26

Writing PHP4 style classes - no visibility modifiers so all member variables and functions are public.

Not writing unit (or other types of) tests. That usually also means little refactoring (it's hard to refactor code without tests).

Overusing magic methods - e.g. __get(), __call(). These methods can do cool tricks but also obfuscate class.

Premature sanitization of input values:

escaping all $_GET, $_POST, $_REQUEST arrays with mysql_real_escape_string()
storing htmlspecialchars() escaped string to database

Answer 27

$i = $_POST['key']
if($i == '') $i = $_GET['key']

Answer 28

I would have to say ... almost anything on http://php.net (the "documentation section in particular).

While some people claim it's a great source for information (and there is lots of information) -- the problem is there is lots of bad information mixed in with the good -- but unlike SO, where the same applies, there is no real moderation. This site has done as much to pollute and destroy new developers -- if not more -- as it has done to encourage good programming. None of the "competing" languages (Perl, PHP, Ruby, etc.) can claim the same.

Reading the "accepted" user comments (often complete with code to avoid) makes me cringe.

Answer 29

Open Source libraries without PHPDoc (or equivalent) tags, so there's no API documentation and you have to wade through the code itself to find out what it actually does... only marginally worse is PHPDoc tags that don't match the actual code.

I know it's hard work keeping tags up to date, and boring as well; but it's a necessary part of maintaining any open source library.

Answer 30

Using variable variables ^[1] in all cases:

Why? You should use an array instead:

for ($i = 0; $i < 10; $i++) {
    $var = "foo" . $i;
    $$var = $i;
}

[1] http://php.net/manual/en/language.variables.variable.php

Answer 31

It's not the worst thing ever, but I really do hate it when people put the "then" part of an if statement without braces. Let me just show an example.

if($this == $that)
doSomething();

I hate this. It bugs me. Please use a set of braces...

if($this == $that)
{
    doSomething();
}

It's small, but it really makes a difference to me. I go hunting for a closing brace that's not there. Put an else in the mix and it's actually worse.

if($this == $that)
    doSomething();
else
    doSomethingElse();
//this drives me nuts

Answer 32

if(true == $var)
not using unset() enough
doing things like select * from table when only need id
$object =& new Something() in 5.x
<div><?php $rows = $DB->query('....'); foreach.... ?></div> data handling in the view
output in model layers
wrapping lots of code in ifs like

if($true){..code...}else{error(); return;}

could be

if(!$true){something(); return;} ...code...

loads of nested if/foreach/etc
etc

the list of bad practices is 100x bigger that best practice :)

Answer 33

Using a CMS with deprecated functions in it...

Answer 34

I feel a little bad about submitting this as it is a real example that I found in a project; however, hopefully this will help someone else...

// accept DateTime or String as date
$this->birthDate = is_object($birthDate) ? $birthDate : (!empty($birthDate) ? new \DateTime($birthDate) : null);

Problems with the above code:

It tries to do too much. It should delegate to two different methods. One that takes a string, the other that takes a DateTime. This way, the API is clear and easier to to test. Also, a user of this API might want to directly call one of the specific methods rather than go through the proxy method.
It checks that $birthDate is an object rather than check that it is a DateTime object. This can cause some subtle bugs.
Two levels of nested ternary statements...not so hot.
Doesn't really check the string correctly. It assumes anything that is not empty (could be #1) is OK. It should at least wrap "new DateTime(...)" in a try/catch or even "date_create === false".
Not a single unit test was written.

Answer 35

the & before variable in function parameters list is one of those - when you call it without variable it produces error