current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

I want to show users what package they are, based on how many items they have in their cart. The code works, but I'm wondering if anything can be improved.

function new_thing (){


//decleare variables
global $x_subs;
$lim01 = get_option(woomps_limit01);
$lim02 = get_option(woomps_limit02);
$lim03 = get_option(woomps_limit03);
$x = $x_subs;
$plugin_url =(plugins_url("/", __FILE__ ));

//set CSS class based on value of $x 
if  ($x >= 1 && $x < $lim02) {
    $active1 = "fb-sub-active";
    $active2 = "fb-sub-deactive";
    $active3 = "fb-sub-deactive";
} else if ($x >= $lim02 && $x < $lim03) {
    $active1 = "fb-sub-deactive";
    $active2 = "fb-sub-active";
    $active3 = "fb-sub-deactive";
} else if ($x >= $lim03) {
    $active1 = "fb-sub-deactive";
    $active2 = "fb-sub-deactive";
    $active3 = "fb-sub-active";
} else {
    $active1 = "fb-sub-deactive";
    $active2 = "fb-sub-deactive";
    $active3 = "fb-sub-deactive";
}


//generate the content that will be displayed
$content = <<<EOD

<div class="fb-sub-img-holder">
<img src="{$plugin_url}/images/subscription_pack_1.png" class="fb-img fb-sub-table1 {$active1}"></img>
<img src="{$plugin_url}/images/subscription_pack_2.png" class="fb-img fb-sub-table2 {$active2}"></img>
<img src="{$plugin_url}/images/subscription_pack_3.png" class="fb-img fb-sub-table3 {$active3}"></img>
<p>{$x_subs}</p>
</div>

EOD;

return $content;
}

.fb-sub-img-holder {
    width: 95%;
    margin: 0 auto;
}

.fb-img {
    width: 30%;

.fb-sub-active {
    width: 50%;
}
.fb-sub-deactive {
    width: 20%;
}
share|improve this question
up vote 3 down vote accepted

Why is $lim01 never used? Is it always equal to 1?

Only one of the subscription packs applies, if at all. You should first make it clear which subscription pack applies.

$subscription_pack = (($x >= $lim03) ? 3 :
                     (($x >= $lim02) ? 2 :
                     (($x >= $lim01) ? 1 : NULL)));
# Note: inactive rather than deactive
$active1 = $active2 = $active3 = 'fb-sub-inactive';
if ($subscription_pack == 1) { $active1 = 'fb-sub-active'; }
if ($subscription_pack == 2) { $active2 = 'fb-sub-active'; }
if ($subscription_pack == 3) { $active3 = 'fb-sub-active'; }

I also suggest reworking the CSS so that you can drop the fb-sub-inactive class altogether.

share|improve this answer
    
This was so much simpler. And you are right, $Lim01 is not used for this functionality. Thank you very much, I learned a lot from implementing it. I only changed the operators so it gave out the MY expected result: The reason I have the inactive CSS there is to put a transparency on deactive subscription package. But maybe this can be the default state or use the :not(.fb-sub-active). Hemmm… will try it out. – Robin Pedersen Oct 21 '15 at 9:54
    
You also gave me a lesson on " and ' :) – Robin Pedersen Oct 21 '15 at 9:56

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.