PHP and writing clean code

Question

Hello

Im trying to find best practices to write PHP.

I just wonder is this a bad habit.

For example, processing variables.

$var = 1
$var = doSomething($var);
$var = doSomething2($var);
$var = doSomething3($var);

It looks a bit awful.

Here is a example of a real code that I just did:

$this->rSum = explode(",", $this->options["rSum"]);
$this->rSum = array_combine(array_values($this->rSum), array_fill(0, count($this->rSum), 0));

If someone could pass me some good tutorials of writing cleaner code generally it would be nice!

Its me again asking stupid questions. :)

Answer 1

1. Zend Framework Coding Standard
2. PEAR Coding Standard

The most important, be consistent.

Answer 2

Hi Pirkka,

Smart code is not necessarily good code in my opinion. I'd personally prefer clean, simple, easy to understand code. your 2 liner will make your pear think hard, as opposed to your init "bad" code.

That's just my take anyway.

Answer 3

In addition to the coding standards, you can use PHP_CodeSniffer to get general hints on your existing code.

Answer 4

By the way..

How much automatic processing can there be in models?

I have a model that has a execute method and when I call it, it does a lot of things like reading a definition file and making database queries.

For example

$object = new Object()
$object->setFile("example.txt");
$object->execute();

// Then i can fetch things from it
echo $object->getName();

Answer 5

Agreed with Jim Li, I also prefer readable code over micro-optimisation or smart but ugly one-liner code.

The only trouble I had with your first example is that it uses procedural oriented functions, I would be better rewritten OOP (functions calls like this could be chained and stay easy to read).

PHP hasn't a strong OOP aim, as it uses mainly procedural statements in its API. But I prefer writing my code in OO way, well decoupled and organisated, rather than having a whole lot of functions with plenty of arguments to get them work together.

Answer 6

I really like your (real) code and it's usually kinda hard for me to like other persons code (I haven't had much time to dig into ZF but PEAR for instance [they also have their coding standards] is just awful IMO), the first example you gave just seems stupid but regarding the second one, it is really easy to understand at least for me and from the short snippet you provided you seem to have a consistent coding style and be using whitespaces in the right amount and at right places - that counts a lot for clean code (if you don't believe me just take a look at some Perl snippets).

I would only like to point out three things:

1. Semantics: Although rSum is not a awful name for a property it isn't quite clear what values does it hold, perhaps you could find a more descriptive name for that property?
2. Variable Reuse: As I said before your first example seems stupid but it's actually smart to reuse variables for two main reasons:
   1. You don't waste memory.
   2. And you don't pollute the scope you're working with.

3. Your second "real" example could be cleaner and faster if you use the right functions:

   $this->rSum = array_flip(explode(",", $this->options["rSum"]));

EDIT: I just noticed the code I provided above is not quite what you're doing (the 0 was not processed by my brain), here is another working alternative:

$this->rSum = array_fill_keys(explode(",", $this->options["rSum"]), 0);

There seems to be a lot of people here that don't like one-liners I however, believe that the the above code is clear, efficient and descriptive - but that may be just me... =)

Answer 7

One thing that trying to do eveything in one line can lead to is code assumptions. Which I rate as really annoying when I have to keep fixing them. This is more common with object chaining. For example

$object->getAnotherObject()->getAThirdObject()->doSomething();

A lot of people will tell you that it is easier to read; however it relies on each return being an object every time. I prefer to return each one and check the response.

$secondObject = $object->getAnotherObject();
if ( is_object($secondObject) ) {
    $thirdObject = $secondObject->getAThirdObject();
    if ( is_object($thirdObject) ) {
        $thirdObject->doSomething();
    }
} 

It takes more keystrokes sure, but it will be less likely to blow up and, I think anyway, easier to read.

Although it is worth repeating what Boris Guéry wrote. be consistent.