Coding Best Practices

Book References

Bad Code Sample

Class AuditObserver {
    public function created($model)
    {
        $ipAddress      = request()->ip();
        $current_date   = Carbon::now();
        $user           = Auth::user()->email;
        $after_action   = $model->getAttributes(); // Get the updated values after the Create
        $delimiter      = "App\Models\\"; // Note that we need to escape the backslash character
        $split_delimiter = explode($delimiter, get_class($model));
        $model_name     = end($split_delimiter);

        Log::create([
            'model_name'    => $model_name,
            'model_id'      => $model->id,
            'user_mail'     => $user,
            'action'        => 'Create',
            'date_time'     => $current_date,
            'before_values' => Null,
            'after_values'  => $after_action,
            'ip'            => $ipAddress
        ]);
    }

    public function updating($model)
    {
        $before_action  = $model->getOriginal(); // Get the original values before the update
        $ipAddress      = request()->ip();
        $after_action   = $model->getAttributes(); // Get the updated values after the update
        $current_date   = Carbon::now();
        $user           = Auth::user()->email;
        $delimiter      = "App\Models\\"; // Note that we need to escape the backslash character
        $split_delimiter = explode($delimiter, get_class($model));
        $model_name     = end($split_delimiter);


        Log::create([
            'model_name'    => $model_name,
            'model_id'      => $model->id,
            'user_mail'     => $user,
            'action'        => 'Update',
            'date_time'     => $current_date,
            'before_values' => $before_action,
            'after_values'  => $after_action,
            'ip'            => $ipAddress
        ]);
    }

    public function deleted($model)
    {
        $current_date   = Carbon::now();
        $user           = Auth::user()->email;
        $before_action  = $model->getOriginal(); // Get the original values before the Delete
        $ipAddress      = request()->ip();
        $delimiter      = "App\Models\\"; // Note that we need to escape the backslash character
        $split_delimiter = explode($delimiter, get_class($model));
        $model_name     = end($split_delimiter);


        Log::create([
            'model_name'    => $model_name,
            'model_id'      => $model->id,
            'user_mail'     => $user,
            'action'        => 'Delete',
            'date_time'     => $current_date,
            'before_values' => $before_action,
            'after_values'  => Null,
            'ip'            => $ipAddress
        ]);
    }
}

Corrected Code Sample

Class AuditObserver {

	public function created($model)  { $this->AuditLog($model, Log::CREATED); }
    public function updating($model) { $this->AuditLog($model, Log::UPDATED); }
	public function deleted($model)  { $this->AuditLog($model, Log::DELETED); }

    private function AuditLog($model, $action) {
        $l = new Log([
	        'model_id'   => $model->id,
            'model_name' => get_class($model),
            'user_email' => Auth::user()->email,
            'ip'         => request()->ip(),
            'action'     => $action,
            'value_old'  => $model->getOriginal(),
            'value_new'  => $model->getAttributes(),
        ]);
            
        // Ignore Log creation failures as Laravel will automatically handle any errors
    }
}

Bugs

Use of Magic Variables

Best Practice

Do not assign meaningless constants to variables, instead create meaningful constants or enums and assign them.
Compilers and interpreters cannot spot errors in magic numbers.
Developers cannot understand the significance of these numbers.

Sample Code

'action' => 'Create',

Problem

The action string 'Create' is "magical", instead define a constant in the Log class and use it in the AuditObserver class.


// In the Log class define the required constants and use them in other classes 
Class Log {
	public const CREATE = 'Create';
	public const UPDATE = 'Update';
	public const DELETE = 'Delete';
	...
}

// In the AuditObserver class
'action' => Log::CREATE
'action' => Log::UPDATE
'action' => Log::DELETE

// In the DB, define the "action" field as an Enum type with values ['Create', 'Update', 'Delete']

Action at a Distance

#TODO: Define Observer "action" in the Log class and use it in other places.

Poor Quality Comments

Best Practice

Write clear code that is easily understandable without any comments.
Do not write bad code and comment it for clarity, it often makes it worse.
Ensure code and comment are in sync, if you update the code ensure you also immediately update any comments.
Do not comment in English what your code is doing. Every programmer can understand the code, so there is no need to reexplain in English what is clearly mentioned in the code.
Instead comment only why the code is doing something, not what.

Inaccurate Comment, is this a Bug?

Sample Code

$delimiter      = "App\Models\\"; // Note that we need to escape the backslash character

Problems

This comment just repeats what the PHP code is doing in English. A good comment should focus on the why and not the what.
Comment is inconsistent with code. There are two \ characters in the string, but only the second on is escaped. What about the first? Is this a bug? Note that this is not a bug, without escaping the second \, the closing quote will get escaped and string will not be terminated correctly. VS Code or any IDE will pickup this issue and there's really no need to document it.

Useless Comments

Sample Code

$before_action  = $model->getOriginal(); // Get the original values before the update
$after_action   = $model->getAttributes(); // Get the updated values after the update

Problems

Code can be improved by properly naming the variables and removing the comment entirely.
The variable name after_action conveys no meaning, what it really means is value_new.

$value_old = $model->getOriginal();
$value_new = $model->getAttributes();

Poor Variable Names

Best Practices

Variable names accurately must reflect what value a variable is containing.
Use variable names consistently

Inconsistent Variable Names

Sample Code

$user       = Auth::user()->email;
...
'user_mail' => $user,
...

Problems

User's email is called $user, 'user_mail'. Make this consistent with the function call and call it user_email

$user_email = Auth::user()->email;
...
'user_email' => $user_email,

Do Not Repeat Yourself

Best Practice

Do not copy and paste the same code either within a function or across functions.
Instead of repeating code, extract it into a function and call it whenever required.

Useless Variable Assignments

Sample Code

public function updating($model)
{
	$before_action  = $model->getOriginal(); // Get the original values before the update
	$ipAddress      = request()->ip();
	$after_action   = $model->getAttributes(); // Get the updated values after the update
	$current_date   = Carbon::now();
	$user           = Auth::user()->email;
	$delimiter      = "App\Models\\"; // Note that we need to escape the backslash character
	$split_delimiter = explode($delimiter, get_class($model));
	$model_name     = end($split_delimiter);

	Log::create([
		'model_name'    => $model_name,
		'model_id'      => $model->id,
		'user_mail'     => $user,
		'action'        => 'Update',
		'date_time'     => $current_date,
		'before_values' => $before_action,
		'after_values'  => $after_action,
		'ip'            => $ipAddress
	]);
}

Problems

There's no need for all these temporarily variables, just assign them to the Log::create hash directly.

public function updating($model)
{
	Log::create([
		'model_name' => get_class($model),
		'model_id'   => $model->id,
		'user_email' => Auth::user()->email,
		'action'     => Log::UPDATE,
		'value_old'  => $model->getOriginal(),
		'value_new'  => $model->getAttributes(),
		'ip'         => request()->ip()
	]);
}

Repeating Code Across Functions

Sample Code

public function created($model) {
	...
}

public function updated($model) {
	...
}

public function deleted($model) {
	...
}

Problems

Do not repeat the same code in each function, extract the repeated lines into a function and call it when required.

public function created($model) {
	$this->AuditLog($model, Log::CREATED);
}

public function updating($model) {
	$this->AuditLog($model, Log::UPDATED);
}

public function deleted($model) {
	$this->AuditLog($model, Log::DELETED);
}

private function AuditLog($model, $action) {
	$l = new Log([
		'model_id'   => $model->id,
		'model_name' => get_class($model),
		'user_email' => Auth::user()->email,
		'ip'         => request()->ip(),
		'action'     => $action,
		'value_old'  => $model->getOriginal(),
		'value_new'  => $model->getAttributes(),
	]);

	// Ignore Log creation failures as Laravel will automatically handle any errors
}

Pending Items from BIP

Lack of OOPS understanding - attaching Laravel Observer to the base Model Class instead +250 of child classes.
Improper use of public static function foo in Laravel Classes. The keyword static makes the function callable without instantiating the object. Static functions therefore cannot access the $this variable.
Improperly overloading Laravel Eloquent boot method to create closures instead of using simple class methods.
Improperly ordering library imports, framework/standard imports first, self written module imports next section.