How can I reduce lines of code in PHP?

Question

I am a web developer and I write code in PHP/Laravel framework. I've been tying to follow best practice for writing code and I know it's a good practice to write 15-20 lines of code in functions and maximum 200 lines of code in Classes. But every time I end up writing minimum 40-50 lines in a function. for example here the code snippet which I wrote to get details of client and assigned users.

public function preMessageSend($client, $assigned)
{
    $ticket_number = $client->ticket_number;
    $title = $client->title;
    $department = $client->department;
    $priority = $client->priority;
    if ($client->first_name !== null || $client->first_name !== '') {
        $client_name = $client->first_name." ".$client->last_name;
    } else {
        $client_name = $client->username;
    }
    if ($client->email !== '' || $client->email !== null) {
        $client_email = $client->email;
    } else {
        $client->email = 'Not available';
    }
    if($client->mobile !== null || $client->mobile !== '') {
        $client_mobile = $client->code."".$client->mobile; 
    } else {
        $client_mobile = 'Not available';
    }
    if($assigned != null) {
        if ($assigned->first_name !== null || $assigned->first_name !== '') {
            $assigned_name = $assigned->first_name." ".$assigned->last_name;
        } else {
            $assigned_name = $assigned->username;
        }
        if ($assigned->email !== '' || $assigned->email !== null) {
            $assigned_email = $assigned->email;
        } else {
            $assigned->email = 'Not available';
        }
        if($assigned->mobile !== null || $assigned->mobile !== '') {
            $assigned_mobile = $assigned->code."".$assigned->mobile; 
        } else {
            $assigned_mobile = 'Not available';
        }
        if ($assigned->address !== null || $assigned->address !== '') {
            $assigned_address = $assigned->address;
        } else {
            $assigned_address = 'Not available';
        }
        $this->sendMessageWithAssigned($ticket_number, $title, $department, $priority, $client_name, $client_email, $client_mobile, $assigned_name, $assigned_email, $assigned_mobile, $assigned_address);
    } else {
        $this->sendMessageWithoutAssigned($ticket_number, $title, $department, $priority, $client_name, $client_email, $client_mobile);
    }

Please tell me how can I reduce the loc in my class and functions and what are the best practices to avoid writing such long functions. TIA

First question: why do you need to assign all those client object properties to locally scoped variables? — Mark Baker, Sep 23 '16 at 08:13
i think you can do an empty check instead of all those `not null, not empty string` — danopz, Sep 23 '16 at 08:19
you can rewrite most of your conditions into ternary operator one liner `$client_mobile = ($client->mobile !== null || $client->mobile !== '') ? $client->code."".$client->mobile : 'Not available';` — Jan Holas, Sep 23 '16 at 08:19

Xorifelse · Answer 1 · 2016-09-23T09:18:20.353

First of all, null and '' is true for empty() so you could do:

if (!empty($client->first_name)) { // if not empty
    $client_name = $client->first_name." ".$client->last_name;
} else {
    $client_name = $client->username;
}

Then you could also use the ternary operator:

$client_name = !empty($client->first_name) ? $client->first_name." ".$client->last_name : $client->username;

Then for some statements there is also the or statement available:

$client_email     = $client->email or 'Not available';
$client_mobile    = $client->code . $client->mobile or 'Not available';
$assigned_address = $assigned->address or 'Not available';

These or statements are only equal to:

if(!empty($assigned->address)){
   $assigned_address = $assigned->address;
} else {
   $assigned_address = 'Not available';
}

// Or the equivalent ternary
$assigned_address = !empty($assigned->address) ? $assigned->address : 'Not available';

And what I mean for "some" is that:

$client->first_name = null;
$client->last_name  = null;
echo empty($client->first_name." ".$client->last_name); // false 
echo isset($client->first_name." ".$client->last_name); // true

is not empty even if both variables are null, due to the " " space which would make it isset()

Now be careful with those or statements because !empty() does not always give the opposite results as isset() where isset([]) is true and where empty([]) is also true.

score 1 · Accepted Answer · answered Sep 23 '16 at 08:19

Instead of

if ($client->first_name !== null || $client->first_name !== '') {
    $client_name = $client->first_name." ".$client->last_name;
} else {
    $client_name = $client->username;
}

You could do:

$client_name = ($client->first_name !== null || $client->first_name !== '') ? $client->first_name." ".$client->last_name : $client->username;

simon · Answer 3 · 2016-09-23T08:49:37.297

As others already suggested, you can use empty() instead of the != null and != '' checks. Furthermore, you could omit the else part in most statements, e.g.:

$assigned_name = $assigned->username;
if (!empty($assigned->first_name)) {
    $assigned_name = $assigned->first_name." ".$assigned->last_name;
}

This sets $assigned_name to your former else value by default and if the condition is met $assigned_name gets overwritten. I don't recommend using ternary operator because it's not that readable IMO.

I wouldn't worry too much about lines of code anyway as long as the code is readable and efficient.

How can I reduce lines of code in PHP?

3 Answers3