Code bloopers - part 1 (PHP & Laravel)

Working as a development company in an outsourcing world can be fun, but working on legacy code is often the opposite. That said, those projects have their bright sides. For example, you can find a lot of funky stuff in the code to provide great examples of how not to code. 

Sharing is caring, and we wanted to use this opportunity to provide you with some of those examples, along with a few useful suggestions. 

Hardcoding sensitive data

We’ve found a lot of hardcoded credentials, addresses, and all other kinds of sensitive data in the code we’ve inherited. This is really bad practice, which can make maintaining an application fairly difficult and potentially cause some serious security vulnerabilities.

Example:

1function debugEnable()
2{
3    if ((\Request::getClientIp(true) == '219.206.139.228') ||  App::environment('local')) {
4        return true;
5    }
6    return false;
7}

There are plenty of reasons why not to hardcode your IP address:

• This doesn't guarantee exclusive access.

• Your IP will probably change at some point.

• Hardcoding is generally a bad idea.

• And last but not least, your IP can show up on someone’s blog post.

One more thing, try to avoid writing if statements that return a boolean value. Instead, you can simply return the condition of the statement. This is a small step for a programmer, but a giant leap for code readability. If using an IP address is the only way to do this, for whatever reason, we suggest you put it in an .env file:

1function isDebugEnabled()
2{
3   return Request::getClientIp(true) == env('MAGIC_IP_ADDRESS', '127.0.0.1')
4       || App::environment('local');
5}

Or, if you work with Laravel, then simply use config('app.debug') and avoid involving an IP for this purpose.

Code redundancy

One of the principles behind clean code is to avoid repeating yourself. Here’s a simple example of unnecessary code redundancy, and it’s quite common in the code that we’ve worked on:

1if ($result) {
2    return response()->json(['result' => $result]);
3} else {
4    return response()->json(['result' => []]);
5}

Simply don't do it. Be concise and, wherever possible, try to keep repetition to a minimum:

1return response()->json([
2    'result' => $result ?? []
3]);

Or, if you work with older versions of PHP:

1$result = empty($result) ? [] : $result;
2return response()->json([
3    'result' => $result
4]);

By following this principle, you will have a shorter and cleaner code.

Zero information response

Responses that your API returns should have appropriate status codes and provide descriptive error messages so that anyone who uses your API can realise immediately what has gone wrong and why. 

Take a look at this example:

1if ($tempData == null) {
2    return response()->json(['error' => 'error']);
3}

If something goes wrong we’ll get a response with status code 200 and:

1{
2  "error":"error"
3}

When you see something like this, you can’t help but think “For goodness’ sake, think of a better error message, and please use a more appropriate status code.” 

Also, it's a good idea to use is_null() instead of comparing variables with null:

1if (is_null($tempData)) {
2    return response()->json([
3        'error' => 'Temp data not found'
4    ], 404);
5}

Bad variables and method names

There is a strong probability that the code you’re working on will be read many more times than it will be written or edited. No matter whether it’s destined to be read by you or perhaps by other programmers, it’s important to make it understandable and easy to read. The more time you spend on understanding what the code does, the longer it will take to modify and extend it. One of the most obvious signatures of messy code is the poor naming of variables and methods. 

For example, we have a controller method for matching an authenticated user with his role and permissions as an array:

1public function getMe()
2{
3    $me = auth('api')->user();
4    $role = $me->role()->get();
5    $permissions = $role->permissions();
6    $me_data = $me->toArray();
7    $me_data['role'] = $role->toArray();
8    $me_data['role']['permissions'] = $permissions->toArray();
9    return response()->json($me_data);
10}

The problems here are:

• The method name getMe doesn’t give us any information about what we should get. Would it be a class parameter $me? Or possibly an instance of that class? We can only speculate until we’ve read the entire method. We could use something like getAuthUser.

• Inconsistency in naming style. If you use a camelCase then don’t mix it with snake_case. Choose one and stick with it for the entire project.

• auth('api')->user() can return null if a user is not authenticated, and that can cause an error.

• We don’t have to convert all these objects to the array individually. Laravel can handle this for us.

• We can also use eager loading here.

We suggest writing it like this:

1public function getAuthUser()
2{
3    $user = auth('api')->user();
4    if($user){
5        $user->load('role.permissions');
6    }
7    return response()->json($user);
8}

Overusing elseif statements

If you end up with more than one elseif statement, there is a good chance that the logic behind your code could be simpler. Here’s a perfect example of this:

1public function getNumberBasedTitle($no)
2{
3    if($no < 20)
4        return 10;
5    elseif($no < 30)
6        return 20;
7    elseif($no < 40)
8        return 30;
9    elseif($no < 50)
10        return 40;
11    elseif($no < 100)
12        return 50;
13    elseif($no < 150)
14        return 100;
15    elseif($no < 200)
16        return 150;
17    else
18        return 200;
19}

Okay. This is funny. $no kind of reminds us of

This method is used to round the size of a list depending on how many elements we can provide for it. Yeah, there are a lot of things wrong here:

• Method name - roundListSize would be more appropriate because it's more descriptive and makes it easier to guess the type of result.

• Parameter name - instead of $no, we should use something like $elementsCount.

• A Bunch of statements - Do we really need to check all of these conditions? Can we calculate the result instead? In this particular case, we obviously can.

• Hardcoded values - whenever possible, we should use named constants.

This is our take on rewriting the method.

1private function roundListSize($elementsCount)
2{
3    if ($elementsCount < MIN_LIST_SIZE) {
4        return MIN_LIST_SIZE;
5    }
6    if ($elementsCount > MAX_LIST_SIZE) {
7        return MAX_LIST_SIZE;
8    }
9    $modulus = $elementsCount > 50 ? 50 : 10;
10    return $elementsCount - $elementsCount % $modulus;
11}

How would you refactor it?