Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am trying to simplify the amount of code required here using arrays, I feel like my foreach loop is still excessively long. Any advice would be appreciated.

$date_list = Array(
Array( 'age', "$birthday", 'now' ),
Array( 'membership', "$datejoined", 'now' ),
Array( 'promoted', "$lastpromo", 'now' ),
);

foreach ( $date_list as $date_set ) {
$var = $date_set[0];
$start = $date_set[1];
$end  = $date_set[2];
$datetime1 = date_create($start);
$datetime2 = date_create($end);
$interval = date_diff($datetime1, $datetime2);
if ( substr_count( $var, 'age' ) > 0 ){
    $age = $interval->format('%y');
}
elseif ( substr_count( $var, 'membership' ) > 0 ){
    $years = $interval->format('%y');
    $months = $interval->format('%m');
    $membership = floor(($years*12)+$months);
    if($membership > 1){
        $suffix = 'Months';
    }
    elseif($membership == 1){
        $suffix = 'Month';
    }
    else{
        $membership = "< 1";
        $suffix = 'Month';
    }
}
elseif ( substr_count( $var, 'promoted' ) > 0 ){
    $years = $interval->format('%y');
    $months = $interval->format('%m');
    $test = $interval->format('%a');
    $promoted = floor(($years*12)+$months);
    if($promoted > 1){
        $suffix = 'Months ago';
    }
    elseif($promoted == 1){
        $suffix = 'Month ago';
    }
    else{
        $promoted = "< 1";
        $suffix = 'ago';
    }
}

}

share|improve this question
Ugh. Firstly, use switch statements. Why are you using substr_count when you could use switch? You put the data there, you know it is clean. If this is just test code and in reality it would have input from elsewhere, just clean it up either before you put it into the array or after you extract it. – itsbruce Aug 16 at 23:30
Secondly, your question is ambiguous. Do you mean that you tried to use arrays to improve the code, but need advice on how to complete that, or that you want to improve this code, which just happens to use arrays? If you can be clearer about this, and about why you are using arrays, that will help – itsbruce Aug 16 at 23:33
Finally, a little more context for this code snippet would help. – itsbruce Aug 16 at 23:37
up vote 1 down vote

It's hard to suggest simplifications without a greater understanding of the problem being solved. However, this is perhaps a bit more readable:

$dates = array(
    array('type' => 'age',        'start' => $birthday,   'stop' => 'now' ),
    array('type' => 'membership', 'start' => $datejoined, 'stop' => 'now' ),
    array('type' => 'promoted',   'start' => $lastpromo,  'stop' => 'now' ),
);

foreach($dates as $date)
{
    $start = date_create($date['start']);
    $stop  = date_create($date['stop']);
    $interval = date_diff($start, $stop);

    switch($date['type'])
    {
        case 'age':
            $age = $interval->format('%y');
            break;

        case 'membership':
            $years      = $interval->format('%y');
            $months     = $interval->format('%m');
            $membership = floor(($years*12)+$months);

            if($membership >  1) { $suffix = 'Months'; }
            if($membership == 1) { $suffix = 'Month'; }
            if($membership <  1) { $suffix = 'Month'; $membership = '< 1'; }
            break;

        case 'prompted':
            $years    = $interval->format('%y');
            $months   = $interval->format('%m');
            $test     = $interval->format('%a');
            $promoted = floor(($years*12)+$months);

            if ($promoted  > 1) { $suffix = 'Months ago'; }
            if ($promoted == 0) { $suffix = 'Month ago';  }
            if ($promoted  < 1) { $suffix = 'ago'; $promoted = '< 1'; }
            break;
    }
}

?

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.