We all want to be better coders. When i code, i try to stick to the PSR-1 and PSR-2 coding standards. For those of you who don’t know what these are, i recommend reading: PSR-1 Coding Standard and PSR-2 Coding Standard. However i centralize this article on PHP, it applies to any programming language. It’s just a collection of do’s and dont’s that i have picked up over the years. Rule of thumb should be: SIMPLICITY.
Don’t use abbreviations
Never write abbreviations for variables / class names / methods / namespaces. Longer names in the code won’t slow your code down. Also, avoid using syntaxes like “for ($i=1; $i < 10; $i++)” or “foreach ($p as $persons)” – it’s bad in the long run, making your code hard to read in the future. Exception to the rule would be ID as an abbreviation for identifier.
Be explicit
When writing methods especially, do your best to describe what the method does (and potentially the variables used) – i.e. if you have a User::get($email), it is best to write it as User::getByEmail($email) – making it explicitly verbose and easy to understand in time – not requiring you to find the declaration, or read the typehints for the variables used. There are people who state that methods/variables/classnames should not exceed two words – however the example provided is ok, because it is still very verbose. Whenever you do write methods with more than 3 words in the name, it might indicate you are breaking other rules – like the method doing too much, an indication that should lead to refactoring the method into multiple methods. A good example would be, if you have a method for creating a user and sending the activation email – i.e. User::createAndSendActivationEmail() – this is a clear example of a code smell. You should break it down to User::create() and User::sendActivationEmail(). Try to avoid however, being too explicit. For instance, using User::createUser(). Imagine how that sounds… $user->createUser() – it’s like you are repeating yourself, right?
Try avoiding else as much as possible
If you have a business rule that, you have to check before performing an action… let’s take a simple example, say signing up for a newsletter [i will be using Laravel syntax because it’s so nice]
public function signup()
{
$email = Input::get('email');
if (is_valid($email)) {
if (!$this->newsletter->has($email)) {
return $this->newsletter->add($email); //let's say it returns true if the insert was made
} else {
throw new Exception('Email already exists in our newsletter!');
}
} else {
throw new Exception('Invalid email specified!');
}
How does that look? Every branching of the code makes it very confusing and hard to read in the future. How should it be done? I would do it like this: (i know the code is not a real life scenario, but it will prove the point)
public function signup()
{
$email = Input::get('email');
if (!is_valid($email) {
throw new Exception('Invalid email specified!');
}
if ($this->newsletter->has($email)) {
throw new Exception('Email already exists in our newsletter!');
}
return $this->newsletter->add($email);
}
Doesn’t it look much better and simpler to read, now? Notice how we have removed a chunk of the code and made it simpler to read? Also decreased the indentation – which is a proof that we’re doing it right!
Consider the case where you have multiple if – elseif – elseif – else branches or the switch counterpart – for instance, if we have a user login function that logs in and redirects users to a different landing page based on the user type – for instance, we have a fashion website, where users can be regular users, editors, and administrators and we want to redirect them to /user, /editor and /admin. We would have this following code (again, using Laravel syntax):
function loginAndRedirect() {
$credentials = Request::all();
if (!Auth::attempt($credentials)) {
return redirect('/');
} else {
$user = Auth::user();
if ($user->type == 'administrator') {
return redirect('admin');
} elseif ($user->type == 'editor') {
return redirect('editor');
} else {
return redirect('user');
}
}
}
Sounds simple, right? Well… not really. The above example is bad in more than one way. If you were paying attention, this could be improved in a very elegant way. Consider the alternative:
function login()
{
if (!Auth::attempt(Request::all())) {
return redirect('/');
}
$this->redirect(Auth::user());
}
function redirect(User $user)
{
if ($user->type == 'administrator') {
return redirect('administrator');
}
if ($user->type == 'editor') {
return redirect('editor');
}
return ('user');
}
Notice, yet again how we made the code much simpler, easier to understand, especially a few years from now, when you or others will go into the code to change functionality? Note the above code should live inside a class. I have not wrapped it properly to make it easier to read and understand. You can go further, and create detailed methods to check the user type – i.e. $user->isAdministrator() and $user->isEditor() to make the code easier to read and to be used in different parts of the code.
One level of indentation
You should try and limit yourself to one line of indentation. This one is hard to force yourself into, especially if you love doing multiple for loops or combining if with for or foreach.
Let’s do another example. Let’s say we have a CRM that has companies with multiple users, and we want to get all users by type – say we want to get salespersons, or admins, or account managers. How do we do that? Let’s consider the following code:
class Company
{
private $users; //we should have a constructor for this, or this could be your model and it is lazy loaded somehow - i will exclude it from the current example.
public function getUsersBy($type)
{
$usersCollection = [];
foreach ($this->users as $user) {
if ($user->getType() == $type) {
//next, we check if the user account is active.
if ($user->isActive()) {
$usersCollection[] = $user;
}
}
}
return $usersCollection;
}
}
Admit it, you wrote code like this. I think we all did. Look at the number of indentation (disregard the first level, the method’s body). We have 3 levels of code. We are 2 methods over what we’ve decided is ok. Let’s consider the proper alternative:
class Company
{
private $usersCollection;
public function getUsersBy($type)
{
return array_filter($this->usersCollection, function ($user) use ($type) {
return $user->is($type);
});
}
public function userIsType($user, $type)
{
return (($user->getType() == $type) && $user->isActive());
}
}
class User
{
private $type;
private $is_active;
public function is($type)
{
return (($this->getType() == $type) && $this->isActive());
}
private function getType()
{
return $this->type;
}
private function isActive()
{
return $this->is_active;
}
}
I have included the User class to dig deeper into best practices.
Single responsibility principle
We often get to a point where our classes start depending on too many other objects to function. That is a good practice in general, but when certain objects become dependant of too many other objects, it is not a good idea, due to the fact that we are creating very “heavy” objects and we are exposing a lot of business logic to objects that don’t need to be exposed to so much logic. They are often called god objects. So, what to do? Break up the functionality between multiple objects that can work together, or create a middle man that does some part of the heavy lifting, making your objects lighter and simpler.
When you realise that an object is doing things that it should not do, it is a red flag that you should externalize that functionality, to abide to the single responsibility principle. This is called extracting responsibilities. This also makes your code more useful, by granting you the possibility to re-use certain methods elsewhere in the code. Also, for some of the functionality you can use event listeners, that trigger functionality when certain actions fire (example, would be logging certain actions or sending notifications).
Make your code semantic
It helps, when people read your code, for it to sound like a sentence. Like is doing something. A few years back, i had a user model in Codeigniter (it was not dabase driven, not in the regular way), where i had methods called like this:
if ($this->user->isLogged()) //this, obviously checks if the user is logged
$this->user->logout() //this logs the user out
$this->user->login($credentials) //this logs the user in
Pretty straightforward, right?
Wrapping primitves
When we write code that, for instance draws a line on top of a google maps layer to say, delimit a property, we would have something like this:
function draw($distance) {
//draws a line of $distance Meters
}
This will become confusing over time, when you will find draw(1.5); in the code and you won’t know what that 1.5 actually is. How we fix this? We create a Meter class. Or even better – a Milimeter class, to give us better precision, like this:
class Milimeter
{
protected $milimeters;
function __construct($milimeters) {
$this->milimeters = $milimeters;
}
}
draw(new Milimeter(1500));
You would be inclined to say this is pointless, and complicates the code. For some scenarios, i agree with you. So you have to be very careful, when chosing to go on this path, to make sure it provides more clarity to your code, and extends functionality. The above example is very simple to read and understand. Think about the fact that you may need to display the information to the user… maybe some sizes are too big for milimeters and you need to express the size in Meters or Kilometers. Just create toMeters() and toKilometers() and you are done. What if your user lives in England, or America, or another part of the world where they use yards? No problem! Create methods for them, too.
Also, it will help you when doing operations, where you can restrict which units of measure you can use and how the behavior of your objects should be. Typehinting the unit in the draw function – function draw(Milimeter $distance) also makes the code easier to read. Be warned, again… there are cases when this is useful and when this is overkill.
Posted July 11, 2016 11:08 am