CODEWITHSUNDEEP
phprefactoring
Derp
Derp

Reputation: 109

Refactoring simple function

I need to show current record status in human readable format. In my database I have an int for this field, so record may have status 1, 2, 3, 4 e.t.c

I wrote some code to show current status to user:

<?php 
// code to retrieve current status from DB
// ...


if ($status == '1') {
    echo "Current status: Active";
}
if ($status == '2') {
    echo "Current status: Pending";
}
    if ($status == '3') {
    echo "Current status: Inactive";
}

// etc..

?>

This code looks very ugly, but I can't figure out how to refactor it to make it more effective and without infinite series of conditions.

Upvotes: 0

Views: 112

Answers (5)

Bojangles
Bojangles

Reputation: 101473

This is best solved with an array:

function foo($status) {
    $strings = array(
        '1' => "Current status: Active",
        '2' => "Current status: Pending";
        '3' => "Current status: Inactive";
    );

    return $strings[$status];
}

The syntax is much shorter, and you can expand it at a later date by adding keys to the array.

Upvotes: 4

Neville Kuyt
Neville Kuyt

Reputation: 29619

There are a number of things you might do to improve this.

Firstly, many developers prefer to write comparisons with constant values as follows:

if ('1' == $status) {

That makes it harder to accidentally use the assignment operator ("=") instead of the equality operator ("=="), which tends to create entertaining bugs.

Secondly, as both MortenSickel and Thomas Ruiz write, it's a good idea to replace this kind of statement with a lookup. MortenSickel's answer is better because it means you can have gaps in the array, and don't have to coordinate your array population with arbitrary integer positions - it's always better to be explicit.

My recommendation, however, would be to put the status descriptions in the database - it's likely that this information is required in several places (the code which populates the database, the code which reads it out, reports etc.), and coordinating this across multiple code locations sounds like a bad idea.

Upvotes: 1

NullPoiиteя
NullPoiиteя

Reputation: 57312

try Switch Statement

switch($status){
  case '1':
   echo "Current status: Active";
   break;

  case '2':
    echo "Current status: Pending";
    break;

 case '3':
    echo "Current status: Inactive";
   break;
 default:
   echo "i is not equal to  1,2 or 3";
}

deleted because . better approach would be JamWaffles answer

Upvotes: 2

Thomas Ruiz
Thomas Ruiz

Reputation: 3661

You can create an array referencing the status :

<?php
$status_cases = ['active', 'pending', 'inactive'];

echo "Current status: ". $status_cases[$status - 1];

Upvotes: 2

MortenSickel
MortenSickel

Reputation: 2200

Make an array: 

$statvalue=array(1=>'Active',2=>'Pending',3=>'Inactive');
echo("Current status: {$statvalue[$status]}");

Upvotes: 5

Related Questions