Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've noticed I've got quite a few repeated lines in my code.

$start = mktime(0, 0, 0, 3, 1, 2014);
$rate = 1 / 30;
$amount = number_format(floor((time() - $start) * $rate));

$id = array(
    array("42706686", "rebel2america"),
    array("100004055720670", "marquez.candace"),
    array("100000756492404", "vince.goode.3"),
    array("100007763585373", "laura.allen.75491"),
);

// 70% chance of red an 30% of green
$status = array(
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("green", "yes"),
    array("green", "yes"),
    array("green", "yes")
);

$online = range(7, 21);

// create and assign a 'seed' value that changes 
// every $duration and assign to mt_srand
$duration = 5;
$mins = date('i', strtotime('now'));
$seed = $mins - ($mins % $duration);
mt_srand($seed);

// use mt_rand to build an 'order by'
// array that will change every $duration
$orderBy = array_map(function() {
    return mt_rand();
}, range(1, count($id)));

// sort $ids against $orderBy
array_multisort($orderBy, $id);

$orderBy = array_map(function() {
    return mt_rand();
}, range(1, count($status)));
array_multisort($orderBy, $status);

$orderBy = array_map(function() {
    return mt_rand();
}, range(1, count($online)));
array_multisort($orderBy, $online);

for($i = 0; $i < 10; $i++) {
    $results1 = array_pop($id);
    $results2 = array_pop($status);
    echo "                            <td>{$results1[0]}</td>\n";
    echo "                            <td>{$results1[1]}</td>\n";
    echo "                            <td><span class=\"{$results2[0]}\">{$results2[1]}</span></td>\n";
}
share|improve this question
    
you are effectively writting a lot of loops complex - - can you please put it the input ouput of your algorithm so that we can see if we can shorten the code by known what to expect. –  azngunit81 Mar 28 at 6:35
3  
Could you please also state what this code does? That should especially be in the title. –  Jamal Mar 28 at 7:09

3 Answers 3

up vote 11 down vote accepted

This part is duplicated:

$orderBy = array_map(function() {
    return mt_rand();
}, range(1, count($id)));
array_multisort($orderBy, $id);

You apply the same logic to $id, $status, $online.

To eliminate duplication, you can move this common logic to a function with one parameter:

function reorder(&$arr) {
    $orderBy = array_map(function() {
        return mt_rand();
    }, range(1, count($arr)));
    array_multisort($orderBy, $arr);
}
reorder($id);
reorder($status);
reorder($online);

Note that we pass the array by reference (&$arr), because array_multisort does the same, and modifies the content of the array anyway.

UPDATE

As other answers and comments have pointed out, you could also eliminate the magic numbers and duplication when creating the $status array, for example like this:

$color1 = array("red", "no");
$color2 = array("green", "yes");
$num_color1 = 7;
$num_color2 = 3;
$status = array_fill(0, $num_color1, $color1)
          + array_fill($num_color1, $num_color2, $color2);
share|improve this answer

Extract some constants:

replace

// 70% chance of red an 30% of green
$status = array(
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("red", "no"),
    array("green", "yes"),
    array("green", "yes"),
    array("green", "yes")
);

by:

// 70% chance of red an 30% of green
$red = array("red", "no");
$green = array("green", "yes");
$status = array($red, $red, $red, $red, $red, $red, $red, $green, $green, $green);
share|improve this answer
3  
The above code can be further re factored by avoiding magic numbers i.e. you can use a constant like: define("COLOR_RED", "red") and then use this constant identifier in you code. –  vivekpoddar Mar 28 at 9:28
3  
This could be even more elegant with array_fill: php.net/manual/en/function.array-fill.php –  Ultra Mar 28 at 9:50
    
@vivekpoddar: I am not a php programmer. You are right defining $red = array("red", "no"); (and also $green) as a constant would be an improvement. –  MrSmith42 Mar 28 at 9:54
1  
@MrSmith42: I am not php programmer too ;) –  vivekpoddar Mar 28 at 10:29
1  
A better approach would be to use a enum and use color names as a constants there. That way you can manage that as a single source of information about colors and hence keep your code dry. –  vivekpoddar Mar 31 at 5:36

I'm assuming you want your script to output a list of user ids, usernames, and online status. Each user has a 30% chance of being online and the list changes randomly every 5 minutes. If I'm wrong about any of that please correct me.

These lines of code don't affect the output.

$start = mktime(0, 0, 0, 3, 1, 2014);
$rate = 1 / 30;
$amount = number_format(floor((time() - $start) * $rate));

$online = range(7, 21);

$orderBy = array_map(function() {
    return mt_rand();
}, range(1, count($online)));
array_multisort($orderBy, $online);
  • Your seed changes every five minutes, but loops every hour. This gives you just 12 possible seeds.
  • For the online status, inside your loop generate a true/false value that has a certain probability of being true. This requires fewer lines of code and the maximum number of ids is not limited by the size of your status array.
  • Use $i as an index to the array rather than poping elements.
  • Don't print extra white space with your HTML, unless you have a specific reason for doing so.

The modified code:

// 30% chance of being online
$ONLINE_PROB = 0.3;

// create and assign a 'seed' value that changes 
// every $DURATION seconds and assign to mt_srand
$DURATION = 5*60;
mt_srand(floor(time()/$DURATION));

$id = array(
  array("42706686", "rebel2america"),
  array("100004055720670", "marquez.candace"),
  array("100000756492404", "vince.goode.3"),
  array("100007763585373", "laura.allen.75491"),
);
// randomly shuffle $id
$orderBy = array_map(function(){return mt_rand();},range(1,count($id)));
array_multisort($orderBy,$id);

for($i=0; $i<count($id); ++$i)
{
  echo "<td>{$id[$i][0]}</td><td>{$id[$i][1]}</td>";
  if(mt_rand()/mt_getrandmax() <= $ONLINE_PROB)
    echo "<td><span class=\"green\">yes</span></td>";
  else
    echo "<td><span class=\"red\">no</span></td>";
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.