Clean and Lean Code

How to Write Clean, Readable, Maintainable Code


Anyone who involved on coding has their own way to make their code understable, maintainable and clean. In this article I will talk about my experience to keep code clean. I will be using PHP and Laravel functions on my example however it does not mean pretty much same rules apply for other programming languages too.

So Lets get started with basics.


Naming

Meaningful names

All your class names, view names should be meaningful and it should give an idea about your class. Also avoid using plural on classes. Unless you require one. E.g if you are using composite design pattern.

Class Users cannot have a method getUsername(). An object instantiated by Users can have sendMassEmailToAll(), getAllActiveUsers() but getUsername() function belongs to User Class.

Function names, parameters, variables all must be meaningful names too

function createUser($username, $password, $email, $firstname, $lastname ...)
{
}

Less function parameters

Example this function its quite good name is ok parameters are meaningful but imagine that you are creating a user in multiple part of your code and each time you have to remember order of this parameters.

Its also hard to maintain also there is literrally no difference between firstname and lastname variable they both are string and stored in a database having a code like this would accur some bugs. So a function parameters shouldnt be more than 3. maximum 4 in same cases.

If you need more function parameters either they needs to be grouped for example pass $data array or you need to divide your function to smaller functions

function createUser($data)

ok this code looks better I guess. So now simply we can do something like this in our controllers

$userData = $request->all();
//Validation
User::createUser($userData); 

Consistency along variable names

Even though its quite ok there is something bothers me. $userData. if createUser function requires $data variable its better to name it as it is.

Also in some projects I noticed frontend developers use different fieldnames on their forms which also makes confusion and extra work. if your form input named as

<\input name="userName" type="text" />
while you store data as username in your database. You need to have an extra line
$data['username'] = $request->get('userName);

Prevent using negative functions

public function userExist($username)
{
    return "if user exist" ? True : False;
}
if (userExist($username) {
    return sendWelcomeEmail();
}
return 'user not found';
if (userDoesNotExist($username) {
    return 'user not found';
} else {
    sendWelcomeEmail();
}

Commenting

Commenting a code is probably one of the hardest part for programmers. Either too much or too less comment have same consequiences. Unclean code

More comments doesn't make your code better

Recently I saw someone selling out a wordpress package and he claims his code is very clean by advertesing that he commented every single line of his code.

$user = new User(); //define new user
$user->firstName = 'Anar'; //assign firstname
$user->lastName = 'Bayramov'; //assign lastname

If code explains itself no point of commenting it

Commenting everything creates a lot of distraction for a reader and decrease readability tremendously. Having clean variable, function, class names is more than enough. Like the example above

if($user->isLoggedInToday() && $user->isActive()) {
    if(!is_null($user->email)) { //if user loggedin today and active and if email is defined send email
        sendHappyNewYearMessage($user);
    }
}

If you think your code is hard to understand simplfy instead of commenting

If you look at the code above it does explain itself. However programmer had concerns that in future he might not understand code thats why he put that inline comment.

Unless there is an exceptional case inline commands are not necessary a clean code should explain itself. If you feel like you need to put an inline comment to that part most likely you better refactor your code to more readable parts.

Use generic functions instead of specific

Another problem with that code is isLoggedInToday() function. For different cases this function is not really useful. isLoggedInThisWeek(), isLoggedInThisMonth(), isLoggedInThisYear() etc... defining a function like isLoggedIn($date1, $date2) would avoid you to duplicate your code...

But wait ? what is $date1 and $date2 is it beginning time or end time ? if I am using this function often each time I have to check what date1 stands for. So function inputs and outputs should explain what exactly they are such as $beginDate, $endDate

Get rid of commented out code

Last thing about commenthing is commented out code. If you are done with a code do not commented it out. Simply remove it. Commented out codes decrease readability of your code

public function uploadImages($request)
{   //loop stuff
    //foreach ($request->files as $file) {
    //    User->savePhotos($file);
    //}
    $user->savePhoto($request->file);
}

Based on the code Users could upload multiple images in past but now they can upload only one so programmer commented out that foreach loop but left it there.

I added first comment on purpose to mention about that. However I have seen a lot of sloppy comments in programs too. Comments are as important as function,variable or class names they should be clean not sloppy as "loopstuff".


Indention

Align arrows on your arrays

Align arrows below each other on your arrays

return view('employee.vacancy.index')->with([
           'departments' =>  $departments,
           'productCategories' =>  $productCategories,
           'locations' =>  $locations,
           'leadershipLevels' =>  $leadershipLevels,
           'userDepartments' =>  $userDepartments,
           'userProductCategories' =>  $userProductCategories,
           'userLocations'         =>  $userLocations,
       ]);
       
return view('employee.vacancy.index')->with([
           'departments'           =>  $departments,
           'productCategories'     =>  $productCategories,
           'locations'             =>  $locations,
           'leadershipLevels'      =>  $leadershipLevels,
           'userDepartments'       =>  $userDepartments,
           'userProductCategories' =>  $userProductCategories,
           'userLocations'         =>  $userLocations,
       ]);

Indent your code vertically too

Pretty much every programmer try to stick with some coding style rules. Sometimes rules provided by programming language or framework itself or company rules.

For PHP and Laravel generally PSR-2 coding style rules are followed. You can read more about them here

In this part I would prefer talking about vertical indention more. Only small tip: official PHP coding standarts suggest snake case for variable names however many big frameworks such as Zend, Symphony, Laravel prefer using camelCase so it depends on your framework choise but come on who wants to use snake_case variables ?. Also a general rule for PHP. Non-word variables such as $i, $a etc should only be used as iterators not for variable names.

Lets now talk about vertical indention

class User extends Model
{
    public static function getUsersEmail()
    {
        return this->email;
    }
    const ACTIVE = 1;
    const PASSIVE = 0;

    protected $fillable = ['username', 'email','activity_status'];
    use hasRoles;
    public function roles()
    {
        return this->hasMany('App\Roles');
    }
    public function __construct()
    {
        //construction
    }
}

This is a class that would work however it is not vertically indented.

  • Class constants and traits must be on top of your class
  • First method of your class must be always __construct following with other magic methods
  • First a few methods of your class must be methods that explains about your class and what it does.
  • Think about an Article online, first you see title (class name) then you see short description (first methods) then if you are interested you read the rest.

    class User extends Model
    {
        use hasRoles;
    
        const ACTIVE = 1;
        const PASSIVE = 0;
        protected $fillable = ['username', 'email','activity_status'];
    
        public function __construct()
        {
            //construction
        }
    
        public function roles()
        {
            return this->hasMany('App\Roles');
        }
    
        public static function getUsersEmail()
        {
            return this->email;
        }
    }
    

    I am saying first a few methods doesnt mean that your class should contain more than 8-10 methods.

    First principle of SOLID says a class should have only 1 responsibility so if you feel your class gets bigger and bigger you should divide them. For example sendEmail(), sendWelcomeEmail(), sendNotificationEmail(), getUserEmail() etc can be a class such as EmailUser, UserNotification.


    Methods

    Return Exceptions not error or response codes

    When writing a new method especially if it is related with API we prefer using HTTP response codes or error instead of exceptions
    class Url extends Model 
    {
        public static function getFullUrl($shortUrl) 
        {
            $url = self::where(['shortened_url' =>$shortUrl])->first();
    
            if (is_null($url)) {
                return 404;
            }
    
            if (!$url->isActive) {
                return 403;
            }
    
            return $url;
        }
    }
    
    class UrlController extends Controller
    {
        public function index(Request $request)
        {
            $url = Url::getFullUrl($request->shortUrl);
            if ($url == 404) {
                return response()->json('url not found', 404)
            }
    
            if ($url == 403) {
                return response()->json('you are not authorized', 403)
            }
            return response()->json($url);
        }
    }
    

    However same example would be way simpler if we did it with exceptions

    class Url extends Model {
        public static function getFullUrl($shortUrl)
        {
            $url = self::where(['shortened_url' =>$shortUrl])->first();
    
            if (is_null($url)) {
                Throw new Exception 'url not found';
            }
    
            if (!$url->isActive) {
                Throw new Exception 'you are not authorized'
            }
    
            return $url;
        }
    }
    
    class UrlController extends Controller
    {
        public function index(Request $request)
        {
            try {
                return Url::getFullUrl($request->shortUrl);
            } catch (Exception $e) {
                return response()->json($url);
            }
        }
    }
    

    Avoid returning null

    When working with objects especially with database objects sometimes you return no result (null).
    Even though it depends on the situation most of the times not returning null increase your code quality

    public static function getFullUrl($shortUrl)
    {
        $url = self::where(['shortened_url' =>$shortUrl])->first();
        if (!$url->isActive) {
            Throw new Exception 'you are not authorized'
        }
        return $url;
    }
    

    This is completely legit code however if you have to run this method in multiple places and in each of them if you have to check if( is_null ($url)) which will create a lot of unnecessary duplication in your code so its better to do it in first place in your model

    Keep Your Methods Short

    You dont need to write a boolean logic inside an if else block

    private function hasUsers()
    {
       $userCount = User::all()->count();
       if ($userCount > 0) {
           return true;
       }
       else {
           return false;
    }
    

    simply you can do

    public function hasUser()
    {
        return User::all()->count() > 0;
    }

    Get rid of deads

    Sometimes working in a single project for very time leaves some unnecessary traces behind such as methods, variables that are not used anymore even Classes.
    Removing dead variables, functions, classes will make your code way cleaner and they are easly detectable, these days most of IDE's have a searching functionality to find usage of a function also XDebug provides a code coverage tool for that.


    Constants are your friend.

    If a variable might be changed in different situations you should keep them in constants instead of defining under function.
    CONST firstDayofWeek = 'Monday';
    public function foo()
    {
        $firstDayofWeek = 'Monday'
        //do something
    }
    

    Constant variable would make it easier to modify for situations e.g if firstdayofweek changed to Sunday. Otherwise you need to go through all code and find where it is defined.

    Parameterize your functions If order is important

    public function foo()
    {
        $this->createEmailList($users);
        $this->createEmailBody('Welcome');
        $this->send(); //send email
    }
    

    First you create email list of users then you create an email body then you send email. What if user changes order to

  • createEmailBody('Welcome')
  • send()
  • createEmailList($users)

  • A good code must prevent it.
    $emails = $this->createEmailList($users);
    $body = $this->createEmailBody('Welcome', $emails);
    $this->send($body, $emails);
    

    Avoid using array indices directly

    $nav = [‘About Us’, ‘Contact’, ‘Meet The Team’] , $nav = [‘Uber uns’, ‘Kontakt’]
    There is no meet the team in germany so return array[3] in navigation will give you index error so do foreach instead

    Keep PHP HTML CSS seperately

    Use exceptions or return null but never mix HTML CSS and PHP with each other. Think how much trouble it will make if you wanna change your styling in a specific page
    public function getUsers()
    {
        $users = self::all;
        return $users ? $users : '&h4& style="text-color:red"> no users found &/h4&';
    }
    

    Single responsibility on functions

    Same as Classes, functions should have only one responsibilty too. With new IDEs and tools its pretty easy to refactor your function into small ones.

    Prefer non static to static methods

    class Human()
    {
        public static function speed()
        {
            return '10km/h';
        }
    }
    
    class SuperHuman extends Human()
    {
        public function superHumanSpeed()
        {
            return '50km/h';
        }
    }
    
    However without static you could simply override method instead of creating new method with similar
    class Human()
    {
        public function speed()
        {
            return '10km/h';
        }
    }
    
    class SuperHuman extends Human()
    {
        public function speed()
        {
            return '50km/h';
        }
    }
    

    Classes

    Prefer Polymorphism instead of if else

    class Human()
    {
        public function speed($human)
        {
            if($human instance of SuperHuman) {
                return '50km/h';
            } else {
                if ($human->age < 50){
                    return '10km/h';
                } else {
                    return '5km/h';
                }
            }
        }
    }
    
    class SuperHuman extends Human()
    {
        public function getSpeed()
        {
            return parent::speed($this);
        }
    }
    

    Avoid cyclomatic complexity

    Another important thing is when you write a code you should avoid using nested if else statements it reduces code complexity

    class Human()
    {
        public function speed($human)
        {
            if($human instance of SuperHuman) {
                return $this->getSuperHumanSpeed($human);
            } else {
                return $this->getHumanSpeed($human);
        }
        public function getSuperHumanSpeed($human)
        {
            return '50km/h';
        }
        public function getHumanSpeed($human)
        {
            if ($human->age < 50){
                return '10km/h';
            } else {
                return '5km/h';
            }
        }
    }
    
    class SuperHuman extends Human
    {
        public function getSpeed()
        {
            return parent::speed($this);
        }
    }
    

    Interfaces are not waste of time

    This code is not clean enough you can use an interface for HumanSpeed

    interface HumanSpeed
    {
        public function getSpeed();
    }
    
    public class Human()
    {
        public function speed()
        {
            return '10km/h';
        }
    }
    
    public class SuperHuman extends Human implements HumanSpeed
    {
        public function getSpeed()
        {
            return '50km/h'; //or in real case  return parent::speed() * 5;
        }
    }
    
    public class AgedHuman() extends Human implements HumanSpeed
    {
        public function getSpeed()
        {
            return '5km/h' //or in real case  return parent::speed() /2;
        }
    }
    

    Conclusion

    I tried cover all my knowledge hope it will be helpful. I would like to thank and give credits to Robert Cecil Martin and his precious book Clean Code. As it helped me a lot to improve and write this article.

    You can buy it from Amazon.

    I appreciate all your feedbacks and questions. Feel free to drop me an email at anr.bayramov@gmail.com