Classic Code Review for PHP Classes
Classes help organise the code by breaking it into smaller components, that combined together to build powerful applications. Property definitions, abstract classes, local variables and identical methods are all code smells that evolve directly inside large repositories with many classes.
Here some common code smells that arise in OOP PHP code. They all may be spotted with Exakat, the static analysis engine for PHP.
Define All the Properties
PHP is flexible as ever, and there is no need to declare a property before using it.
Yet, it helps PHP to know in advance that a property will be used. It takes advantage of it by pre-allocating memory, and reusing it faster. The property is ready as soon as it is needed. Compare that to the fallback behavior, where PHP has to search for the property, fail to find it, then allocate the memory. This also helps reuse the memory for new objects, as they now have a comparable and predictable footprint.
Quick benchmarks will show that defined properties are 50% faster than undefined properties. It is a micro-optimisation, as accessing a property is very fast, but given the amount of usage of properties, including in loops, this is usually worth the work.
As a common rule, make sure all your properties are defined. As a side note, it also helps your analysis tools.
In fact, as an extra tip, you can add the following __get() and __set() to all your classes, including the one which are totally defined and don’t use magic functions. With them, when an unknown property is called during execution, the magic methods are called, and will act as canaries to warn you about a typo or a mis-target. Use them during development and testing phase, then remove them for production.
<?php
trait NoUnefinedProperties {
function __get($name) {
assert(false, "Attempt to read the $name property, on the class ".__CLASS__;
}
function __set($name, $value) {
assert(false, "Attempt to read the $name property, on the class ".__CLASS__;
}
}
?>
Could Be Abstract Class
An abstract class is a class that can’t be instantiated. It must be a parent of a concrete class, which may be instantiated. Besides holding static methods and class constants, this is the only option for it to be useful.
So how do you call a class which is extended and never instantiated directly? Well, that looks like a template for the inheriting classes, and may actually be marked as such by using the abstract keyword.
<?php
// This class is never instantiated
class Money {
function __construct() { }
}
class Euro extends Money {}
class Dollar extends Money {}
class Yuan extends Money {}
?>
This will actually prevent the class to be instantiated, yet have a constructor. Note that there is no need for any method to be abstract : the whole class may be abstract, implement interfaces, and yet, have no abstract methods.
Abstract classes won’t be tested, at least directly. When you want to keep those tests, extend this abstract class into an empty concrete class. Then, test this class. The behavior will be the same, but now, there is a special class, distinct from any implementation.
<?php
// This class is tested with Rawmoney
abstract class Money {
function __construct() { }
}
class Euro extends Money { /* more code */ }
class Dollar extends Money { /* more code */ }
class Yuan extends Money { /* more code */ }
class Rawmoney extends Money {}
?>
Property Could Be Local
What is the family relationship between a property and a local variable ? Both can hold values, but the scope is different. A local variable is restricted to a method scope, while a property is available within a class, across several methods (let’s omit the public properties for a moment).
<?php
// This class is tested with Rawmoney
class Foo {
private $property = 1;
private $property2 = 1;
function Bar($arg) {
$this->property = $arg + 4 + $this->property2;
$this->property2 = $this->property * 2;
}
function BarBar() {
return $this->property2;
}
}
?>
With those definitions, local variables and properties are similar but different. In fact, the only moment when they are identical is when they cumulate the constraints : a property that is used in only one method.
Such property could be turned into a local variable, without impacting the rest of the class.
The property may be used across several calls to the same method : in that case, it may be interesting to move it to a static variable.
There is also another interesting situation : a local variable that is available in multiple methods. To achieve this, the local scope must transmit the local variable to another scope by calling another method as an argument. This is how variables that are local start appearing in multiple methods signatures, until one realize it could be upgraded to a property, and removed from those signatures.
Identical Methods In A Class Tree
It is usually easy to spot identical methods in one class : the name itself is usually enough to yield a « Fatal error ». On the other hand, identical methods in a class tree is harder to find.
That is, harder to find for us, humans. The same method, available in both the parent class and the child class, is strangely difficult to spot. They look the same, and they also belong to different files, while serving the same purpose. If one overwrites the other, it may be bad luck.
<?php
// This class is tested with Rawmoney
class Foo {
protected function bar() { /**/ }
}
class Foo2 extends Foo {
// Same as above
protected function bar() { /**/ }
protected function barfoo() { /**/ }
}
?>
Identical methods in a class tree is a copy/paste code smell. One of the classes was copied from the other during its creation, or some generalizing refactoring stopped short, and left methods in multiple position.
The good aspect is that it is easy for static analysis to find them. As long as they are identical, and not modified.
The choice of the remaining method may be tricky : is it a parent method, available to other children ? Or is it a child’s specific method ? Or even, should the child method overwrite the parent on purpose, to cancel it?
Could Use Self
Usually, classes are identified with their fully qualified name, such as \X, \A\B\C\MyClass, or even an alias MyClass. And just like anything that is hardcoded, it is hard to change it without refactoring all its instances.
<?php
// This class is tested with Rawmoney
class Foo {
protected const A = 1;
function bar() {
// Static FQN
return Foo::A;
// Adaptable FQN
return self::A;
// For LSB users
return static::A;
}
}
?>
Whenever the class references itself with a FQN, the special name ‘self’ (or its close cousin ‘static’ for Late State Binding), should be used. It keeps the FQN relative to the class, and it is still readable. It makes any refactoring or renaming easier.
Note that self also represents any parent class, so you may use it even when referencing a structure defined in parents.
<?php
// This class is tested with Rawmoney
class Foo {
protected const A = 1;
}
class FooBar extends Foo {
function bar() {
// Return Foo::A
return self::A;
// For LSB users : here, self === static
// as A is defined above only
return static::A;
}
}
?>
Disconnected Classes
Let’s finish with a special structure : the disconnected classes. This happens in class hierarchies, when a class extends another class. The situation is that both classes keep living their life, separated : they never call the other one.
<?php
class Foo {
protected const A = 1;
}
class FooFoo extends Foo {
function bar() {
return 1;
}
}
?>
One extreme case is when the parent class provide support for the child class. This usually saves repeating code, provide formal structure and help plug the child class in a larger framework. In this case, the child class make use of the parent class.
The other extreme case is when the parent classes act as a template for child classes, which provides specialisation, such as methods or properties. The parent has some generic code that is configured thanks to the child configuration. The parent class calls the child.
The general case is a hybrid version of the two above, where parent and children exchange calls to each other. In the end, this build one integrated class, that is a subtle mix of both initial classes. This almost sounds like genetics.
<?php
abstract class Foo {
protected const A = 1;
function foo2() {
return $this->bar();
}
abstract function bar();
}
class FooFoo extends Foo {
function bar() {
return self::A;
}
}
?>
So, what happens when two classes have an extends relationship, but keep from calling each other ? This ends as a code smell, where one of the classes is trying to fulfill a mission in the wrong place of the code. It is a good start for a refactorisation.
Happy Code Review
Most of those patterns happen with the organic evolution of the code. They are the reason why code review is important in the first place : it takes some experience and time to read the code base and spot strange construct in the classes or in the control flow.
All those errors are available in the ‘Class Review‘ rule set, in the Exakat engine. Exakat is a static analysis tool, that review PHP code for code quality and self-improvement. It is open source, easy to install and may run on any PHP code base.