Simple guide to improve our code
Code is never permanent, it changes.
Easier improvements.
Welcome collaboration.
Programming is the art of telling another human what one wants the computer to do.
— Donald Knuth
According to [Schach 1999] the maintenance part of the life-cycle accounts for 67% of the project cost.
simple
it works
secure
commented
flexible
atomic
readable
no duplicated
no spaghetti code
follow standards
simple
it works
secure
commented
flexible
atomic
readable
no duplicated
no spaghetti code
follow standards
No code is perfectly clean.
Code rots.
Code requires maintenance.
Requirements change and code gets obsolete.
Leave the code always cleaner than you found it.
Who owns the code?
Developer
Developer
Team
Team
Company / Client
Brevity
Full feature
Performance
Robustness
Flexibility
Development time
Brevity of code.
Keep It Simple, Stupid
— K.I.S.S principle
Do not be afraid of code reviews.
A developer should always be able to justify the code:
Ask for help or points of view if you feel your code smells.
Agreed before the project.
Follow PSR-1/2 as guide
http://www.php-fig.org/psr/psr-1/
http://www.php-fig.org/psr/psr-2/
Follow drupal standards
https://www.drupal.org/coding-standards
Make your life easier
array(
'id' => 'identifier',
'description' => 'short description',
'text' => 'full text',
);
array(
'id' => 'identifier',
'description' => 'short description',
'text' => 'full text',
);
You can read your code like a book.
The name explicitly says what does/contains/purpose.
Length is not a problem.
Avoid type information in the names.
function get_book_title(array $books) {
foreach ($books as $book) {
if ($book->hasTitle()) {
return $book->getTitle();
}
}
return '';
);
function get_bt(array $bookList) {
foreach ($bookList as $b) {
if ($b->title()) {
return $b->title;
}
}
return '';
);
Say what you mean. Mean what you say.
Write code as if you weren't going to use comments.
Code changes fast.
Comments barely do.
If it doesn't provide useful info, better don't put it.
Ex. of good comments: API, business weird logic,
function get_books_with_title(array $books) {
return array_filter($books, function($book) {
return $book->hasTitle();
});
);
function get_bt(array $bookList) {
// Initialise aux list.
$list = array();
// Loop the list of books.
foreach ($bookList as $b) {
// Check if it has title.
if ($b->title()) {
$list[] = $b->title;
}
}
return $list;
);
function get_book_list($data) {
if ($data['list']) {
return (array) $data['list'];
}
return array();
);
function get_book($id) {
return ($id) ? Book::getInstance($id)
: Book::getEmptyInstance();
}
function get_book_list($data) {
if ($data['list']) {
return (array) $data['list'];
}
return FALSE;
);
function get_book($id) {
return ($id) ? Book::getInstance($id)
: NULL;
}
Always return the same data type.
foreach (get_book_list($data) as $book) {
print get_book($book['id'])->getTitle();
}
function redirect($path, $http_code) {
if ($path) {
header('Location: ' . $path, TRUE,
$http_code);
}
}
function redirect_on_login() {
redirect('/login', 302);
);
function redirect($path = '/login',
$http_code = 302) {
header('Location: ' . $path, TRUE,
$http_code);
}
function redirect_on_login() {
redirect();
);
Avoid default values. They hide functionality.
function get_book_info($data) {
if ($data['id']) {
return load_book_info($data);
}
if ($data['info']) {
return $data['info'];
}
return array();
}
function get_book_info($data) {
if (!$data['id']) {
return array();
}
else if ($data['info']) {
return $data['info'];
}
else {
return load_book_info($data);
}
}
Place default value at the end for visibility.
function setcookie(Cookie $cookie) {
...
);
function get_distance(Coordinate $origin,
Coordinate $destination) {
...
}
function setcookie($name, $value,
$expire, $path, $domain, $secure,
$httponly) {
...
);
function get_distance($originX, $originY,
$destinationX, $destinationY) {
...
}
One argument (monad)
Two arguments (dyad)
Three arguments (triad)
- Think carefully. Can we use objects or lists?
Four arguments
- Probably you can split your function or join arguments.
Stateless
Stateful
class Number {
public function __construct(int $number) {
$this->number = $number;
}
public function double() {
return 2 * $this->number;
}
}
$number = new Number(2)
echo $number->double(); // prints 4
$number = new Number(3);
echo $number->double(); // prints 6
function double(int $number) {
return 2 * $number;
);
echo double(2); //prints 4
echo double(3); //prints 6
One input = One output
One input >= One output
class Number {
private $i;
public function increment(int $increment) {
$this->i += $increment;
return $this->i;
}
}
$number = new Number();
echo $number->increment(2); // prints 2
echo $number->increment(2); // prints 4
function incremental(int $increment) {
static $i = 0;
$i += $increment;
return $increment;
);
echo increment(2); //prints 2
echo increment(2); //prints 4
Please don't use globals
- breaks encapsulation.
- naming collisions.
- anyone can change them.
Singleton
class Number implements Singleton {
private $i;
public function increment(int $increment) {
$this->i += $increment;
return $this->i;
}
}
$number = Number::getInstance();
echo $number->increment(2); // prints 2
$number = Number::getInstance();
echo $number->increment(2); // prints 4
function incremental(int $increment) {
global $i;
$i += $increment;
return $increment;
);
echo increment(2); //prints 2
echo increment(2); //prints 4
static methods are procedural functions.
too many static methods is a smell indicator.
class Number {
static function abs(int $number) {
return ($number * $number) / $number;
}
static function decimals(float $number) {
return $number % self::int($number);
}
static function int(float $number) {
return (int) $number;
}
}
class Number {
protected $value;
public function abs() {
return abs($this->value);
}
static function getTypes() {
return ['float', 'int', 'real'];
}
}
do not extend constants
class Car {
protected $wheels = 4;
public function getWheels() {
return $this->wheels;
}
}
class Motorbike extends Car {
protected $wheels = 2;
}
$car = new Car();
echo $car->getWheels(); // prints 4
$motorbike = new Motorbike();
echo $motorbike->getWheels(); // prints 2
class Car {
const WHEELS = 4;
public function getWheels(int $increment) {
return static::WHEELS;
}
}
class Motorbike extends Car {
const WHEELS = 2;
}
$car = new Car();
echo $car->getWheels(); // prints 4
$motorbike = new Motorbike();
echo $motorbike->getWheels(); // prints 2
Single responsibility
Open-closed
Liskov substitution
Interface segregation
Dependency inversion
Every class is responsible for one part of the functionality.
class Dashboard {
public function getItems() { ... }
}
class Version {
public function getMinorVersion() { ... }
public function getMajorVersion() { ... }
public function getBuildVersion() { ... }
}
class Dashboard {
public function getItems() { ... }
public function getBuildVersion() { ... }
}
entities should be open for extension, but closed for modification
interface Product {
public function getPrice();
}
class Cart {
function getTax(Product[] $products) {
$tax = 0;
foreach ($products as $product) {
if (is_a($product, 'Book')) {
$tax += $product->getPrice() * 0.21;
}
else if (is_a($product, 'Food')) {
$tax += $product->getPrice() * 0.07;
}
}
return $tax;
}
}
interface Product {
public function getPrice();
public function getTax();
}
class Cart {
function getTax(Product[] $products) {
$tax = 0;
foreach ($products as $product) {
$tax += $product->getTax();
}
return $tax;
}
}
no entity should be forced to depend on methods it does not use
interface Task {
public function preprocess();
public function process();
public function print();
public function fax();
public function pdf();
}
interface Task {
public function preprocess();
public function process();
}
interface Print extends Task {
public function print();
}
interface Fax extends Task {
public function fax();
}
interface PDF extends Task {
public function pdf();
}
Depend upon Abstractions. Do not depend upon concretions
interface Car { ... }
class Volkswagen implements Car { ... }
class Garage {
public function fix(Volkswagen $car) {
...
}
public function fixVolkswagen($car) {
...
}
}
interface Car { ... }
class Volkswagen implements Car { ... }
class Garage {
public function fix(Car $car) {
...
}
}
objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program
abstract Car {
public function getModel() : string {
return $this->model;
}
}
class SEAT extends Car {
public function getModel($type) {
if (!$this->model) {
throw new Exception('No model set');
}
return array($this->model);
}
}
abstract Car {
public function getModel() : string {
return $this->model;
}
}
class SEAT extends Car {
public function getModel() : string {
return random($this->spainCities);
}
}
http://www.slideshare.net/hebel/clean-code-vortrag032009pdf
http://blog.codinghorror.com/the-best-code-is-no-code-at-all/
https://en.wikipedia.org/wiki/SOLID_(object-oriented_design)
https://cleancoders.com/category/clean-code
https://scotch.io/bar-talk/s-o-l-i-d-the-first-five-principles-of-object-oriented-design
http://www.irahul.com/2005/10/software-development-vs-software.html
Gorka Guridi
Drupal Architect, Cameron & Wilding Ltd.
www.cameronandwilding.com
@cameronwilding