I am sure every PHP developer has a colleague they hate because he/she coded the application they started maintaining. This is mostly because the code is old. New standards become common, they learn what they used to do was inefficient, unreadable, or complicated. As PHP is a language that changed significantly over the years, code written a few years ago will certainly look outdated.

That being said, this post is not about the old PHP code. This post shows how to be a bad programmer even when following standards. Please don’t be this person.

1. Too many nested if statements
2. Extra brackets and braces
3. Unnecessary casting
4. Useless checks
5. Slow PHP built-in functions
6. Long functions
7. Too many function arguments
8. Long lines
9. Long if-else blocks
10. Wrong function / class name casing
11. Lack of coding standards

Too many nested if statements

This is probably the one I hate the most. It also makes the lines too long because of the indentation caused by each of the nested if statements.

Example

function register() {
     if (!empty($_POST)) {
         $msg = '';
         if ($_POST['user_name']) {
             if ($_POST['user_password_new']) {
                 if ($_POST['user_password_new'] === $_POST['user_password_repeat']) {
                     if (strlen($_POST['user_password_new']) > 5) {
                         if (strlen($_POST['user_name']) < 65 && strlen($_POST['user_name']) > 1) {
                             if (preg_match('/^[a-z\d]{2,64}$/i', $_POST['user_name'])) {
                                 $user = read_user($_POST['user_name']);
                                 if (!isset($user['user_name'])) {
                                     if ($_POST['user_email']) {
                                         if (strlen($_POST['user_email']) < 65) {
                                             if (filter_var($_POST['user_email'], FILTER_VALIDATE_EMAIL)) {
                                                 create_user();
                                                 $_SESSION['msg'] = 'You have now registered so please login';
                                                 header('Location: ' . $_SERVER['PHP_SELF']);
                                                 exit();
                                             } else $msg = 'You must provide a valid email address';
                                         } else $msg = 'Email must be less than 64 characters';
                                     } else $msg = 'Email cannot be empty';
                                 } else $msg = 'Username already exists';
                             } else $msg = 'Username must be only a-z, A-Z, 0-9';
                         } else $msg = 'Username must be between 2 and 64 characters';
                     } else $msg = 'Password must be at least 6 characters';
                 } else $msg = 'Passwords do not match';
             } else $msg = 'Empty Password';
         } else $msg = 'Empty Username';
         $_SESSION['msg'] = $msg;
     } return register_form();
 }

Source: https://twitter.com/mortimergoro/status/476649081146994688

Alternatives

• Return early
• Invert the conditions and use elseif

function register()
 {
     if (empty($_POST)) {
         return register_form();
     }

     $msg = '';
     if (!$_POST['user_name']) {
         $msg = 'Empty Username';
     } elseif (!$_POST['user_password_new']) {
         $msg = 'Empty Password';
         …
     } else {
         $user = read_user($_POST['user_name']);
         if (!isset($user['user_name'])) {
             $msg = 'Username already exists';
             …
         } else {
             create_user();
             $_SESSION['msg'] = 'You have now registered so please login';
             header('Location: ' . $_SERVER['PHP_SELF']);
             exit();
         }
     }
     $_SESSION['msg'] = $msg;
     return register_form();
 }

Extra brackets and braces

Some people like to use brackets (parentheses) and (curly) braces excessively!

Before you ask, this doesn’t mean I don’t like having braces with single statement control structures. That is an exception as I like following standards and most of the PHP coding standards force you to always use braces.

Examples

You don’t need brackets in return statements. PHP Manual for return says:

Note that since return is a language construct and not a function, the parentheses surrounding its arguments are not required. It is common to leave them out, and you actually should do so as PHP has less work to do in this case.

return ($is_true + 5);

You don’t need brackets in include / require statements. From PHP Manual for include:

Because include is a special language construct, parentheses are not needed around its argument. Take care when comparing return value

require('Database.php');

I don’t get logic for this one at all. No programming language has this kind of switch-case structure as standard, haven’t seen it anywhere. Please comment if you like writing switch-case blocks this way.

switch ($value) {
     case('this'):
     {
         do_this();
         break;
     }
     case('that'):
     {
         do_that();
         break;
     }
 }

Enclosing conditions in brackets

if (($variable == 'yes') || ($variable == 'maybe')) {
...
}
if (($variable == 'value')) {
...
}

This one is not about bracket or brace but it is as pointless as those.

$newVar = "$thatvar";

Unnecessary casting

Some people like casting all the variables. Most of the time it’s unnecessary. See the example.

Example

trim always returns a string.
preg_replace returns an array if the subject parameter is an array, or a string otherwise.

$host = (string) trim($_SERVER['HTTP_HOST']);
preg_replace('/^www\./i', '', $host);

Useless checks

It is unnecessary to check if a variable is set before checking for it being non-empty. From PHP Manual for empty:

No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false.

Example

if (isset($_POST['user']) && !empty($_POST['user'])) {
...
}

It is unnecessary to check if an argument passed into a function is set.

function example_func($variable)
{
    if (isset($variable) && $variable == 'foo') {
        ...
    }
}

example_func($variable);

Slow PHP built-in functions

isset vs is_null

Function isset is 20 times faster to check whether a variable is null compared to is_null function.

time vs strtotime(‘now’)

time is 2 times faster than strtotime(‘now’) to get current unix timestamp.

isset vs array_key_exists

isset is 18 times quicker than array_key_exists.
Note: Unlike array_key_exists, isset returns false if the value is null.

Long functions

When functions are too long it is extremely difficult to read how they work. If you split them into smaller functions, it is easier to understand / remember the tasks performed in them. Essentially what you’re doing is grouping the tasks and naming the groups.

Example

This method I found in Smarty template engine is 345 lines! PHPDoc of the method says it “fetches a rendered Smarty template”. I don’t want to read 345 lines to understand how it does it.

public function fetch($template = null, $cache_id = null, $compile_id = null, $parent = null, $display = false, $merge_tpl_vars = true, $no_output_filter = false)
{
    if ($template === null && $this instanceof $this->template_class) {
        $template = $this;
    }
    if ($cache_id !== null && is_object($cache_id)) {
        $parent = $cache_id;
        $cache_id = null;
    }
    if ($parent === null && ($this instanceof Smarty || is_string($template))) {
        $parent = $this;
    }
    ... // 326 lines not shown
    } else {
        if ($merge_tpl_vars) {
            // restore local variables
            $_template->tpl_vars = $save_tpl_vars;
            $_template->config_vars = $save_config_vars;
        }
        // return fetched content
        return $_output;
    }
}

Source: smarty_internal_templatebase.php

Too many function arguments

Having too many function arguments make it significantly harder to use the function. Consider accepting an object that contains the options or using builder pattern instead.

Example

This function, again in Smarty source code, accepts 9 arguments. Will you be able to remember the argument order? The line is extremely long as well.

function smarty_function_html_checkboxes_output($name, $value, $output, $selected, $extra, $separator, $labels, $label_ids, $escape = true)
{
...
}

Long lines

I often split my screen into 2 columns whilst programming. That means if a line of code is longer than 80 characters I either have to scroll or enable word wrapping. Don’t do this to me.
GitHub shows around 125 characters of text without scrollbars.

I try to add line breaks after 85 characters.

Example

This line in twig source code is 235 characters long:

$node->setNode('display_start', new Twig_Node(array(new Twig_Profiler_Node_EnterProfile($this->extensionName, Twig_Profiler_Profile::TEMPLATE, $node->getAttribute('filename'), $varName), $node->getNode('display_start'))));

Source: Twig/lib/Twig/Profiler/NodeVisitor/Profiler.php#L39

Long if-else blocks

Long if-else blocks should be avoided as they can make the code harder to read. They can usually be replaced by an array of inputs and outputs or a more readable switch-case block.

Example

if ($visible) {
    if ($type == "text") {
        $field->setFieldType("myTextField");
        $count++;
    } else if ($type == "number") {
        $field->setFieldType("myNumberField");
        $count++;
    } else if ($type == "decimal") {
        $field->setIsDecimal(true);
        $field->setFieldType("myNumberField");
        $count++;
    } else if ($type == "string") {
        $field->setFieldType("myTextField");
        $count++;
    } else if ($type == "date") {
        $field->setFieldType("myDateField");
        $count++;
    } else if($type == "range") {
        $field->setFieldType("myCheckbox");
        $count++;
    } else if ($type == "combobox") {
        $field->setUrl($url);
        $field->setFieldType("myCombobox");
        $count++;
    } else if ($type == "interval") {
    }
}

Wrong function / class name casing

As you may know in PHP function and class names are case-insensitive.
This means you can type sTrPoS instead strpos and it will still work.

Example

Class myClass {
Static Public FuncTioN myFunc() {}
}
myclass::MyFunc();

Lack of coding standards

Last but definitely not least, to me having a coding standard is an important part of a well-written code.

Example

This example demonstrates what I’d call a disgusting block of code.
I hope no one gets given this kind of code to work on.

if(true) {
    print "can do";
} else if (true)
{
    echo 'can do';
}elseif( TRUE ){ exit; }
function a ( $a , $b )
{
    $a->myMethod(array(
    'y' => 'b'))
    ->chainIt();
    return $a;
}
function b($a, $b) {
    return ($b->myMethod(['y'=>'b'])->
    chainIt());
}

Feedback

Feel free to comment below for the points you don’t agree or additional points that annoy you.
Thanks for reading my short rant!

Reading Material

Visit these links if you want to be liked 🙂

• PSR-2 – Coding Style Guide
• PSR-12 – Coding Style Guide
• PEAR Coding Standards
• Symfony Framework Coding Standards
• Zend Framework Coding Standards
• PHP: The Right Way
• Refactorings – Scrutrinizer