Calculating the price of digital media

Question

I have taken over maintenance of some code, and have restructured it after reading some documents about PSR-2:

<?php

function calc_p() 
{
    list($arg1, $arg2, $arg3) = func_get_args();
    switch($arg1) {
      case 'ebook':
            $bp = 5;
            $tax = .08;
              break;
      case 'audio_book':
           $bp = 5;
           $tax = .08;
              break;
      case 'text_book':
           $bp = 7.5;
           $tax = .1;
           break;
    }   
    $bp = ($bp + ($bp*$tax)); 
    //Calclate shipping
    if($arg3 == 'California') {
    }
    else {
        $bp = $bp + 1;
    }
    if ($arg3 == 'Texas') {
        $norm = $bp;
        $bp = $norm + 5;
    }
    //final price format: $XX:XX
    $final_price = '$'.$bp; 
    if (strpos($final_price, '.')) {
        $num = (2 - (strlen(substr($final_price, strpos($final_price, '.') + 1))));

            for ($num>0; $num--;) {
                $final_price .= '0';
            }
    }
    elseif (strpos($final_price, '.') == FALSE) {
        $final_price = $final_price .'.00';
    }
    else {
    } 
    return $final_price; 
}
$totalprice = calc_p('text_book','oldversion','Texas');
echo '<p style="font-weight: bold;">Total Price: </p>' . $totalprice;
?>

I wanted to ask if what I've done is already enough to follow the coding standards, or if I am missing something.

Answer 1

I've been waiting for this question to be reopened because the formatting issues here are quite extraordinary! I think I'll keep this post to a clean code/code styling post exclusively, that way others can touch on purpose and implementations.

I think what's required here is a bullet point list, which I know is boring, but it's the easiest way to go line by line! Let's begin :)

• Eeek! What in the world is calc_p()!? I immediately know nothing about your code. That's the opposite of what we want. Names should be meaningful and thorough. It looks like you're calculating a price, so a better name might be calculateBookPrice(). Functions are usually expected to be camelCase.
• $arg1: bad name. $arg2: bad name. $arg3: bad name. What are these variables? We need description in these names!
• I had no idea this function needed variables for func_get_args(). You limit the arguments to three, so I'd say add three parameters to the function. That or have something such as ... $someArguments for a parameter. (Obviously with a name that fits your code)
• What is $bp? I immediately have converted this name to "blood pressure". However, that doesn't make sense. Now your reader is confused. See how important useful variable names are?
• 'ebook', 5, .08: what do all these have in common? I don't know why you chose them. If you need to, create (constants, if you have a class) variables and apply these values to a named variable.
• I don't see a default to your switch. I need a default to function! It's best practice to use one anyways.
• What in the world is $bp = ($bp + ($bp*$tax));? Hows about $bp *= ++$tax;? It's shorter and takes advantage PHP's combined operators and pre-increment operator. However, even this isn't too pretty. Perhaps refactor the code before it...
• //Calclate shipping - this is a useless comment. Comment's should describe why.
• It'd be nice to get some place in between statements: if(, '$'.$bp.
• The condition for $arg3 == 'California' is empty, why not negate the comparison? ($arg3 != 'California')
• $bp = $bp + 1 --> $bp++
• Shouldn't you have a switch for the states you check against?
• What is $norm? Are you referring to a person named Norman? Spell out a useful variable name!
• You may want to look into money_format or formatCurrency... That whole block is hideous!
• Some variables are like $final_price and some are like $totalprice! It's vital that you stay consistent!

Answer 2

Most of the improvements Alex had already mentioned in his answer. So I just wrote something quickly down to visualise it a bit better ...

class Product {
    const TYPE_AUDIO_BOOK = 'audio_ebook';
    const TYPE_EBOOK      = 'ebook';
    const TYPE_TEXT_BOOK  = 'text_book';
}

class Price {

  private static $costMap = [
      Product::TYPE_AUDIO_BOOK => [
          'cost'     => 5,
          'tax_rate' => self::TAX_RATE_FULL
      ],
      Product::TYPE_EBOOK => [
          'cost'     => 5,
          'tax_rate' => self::TAX_RATE_FULL
      ],
      Product::TYPE_TEXT_BOOK => [
          'cost'     => 7.5,
          'tax_rate' => self::TAX_RATE_REDUCED
      ]
  ];

  private static $shippingCostMap = [
      'California' => [ 'free_shipping' => true,  'extra_supplement' => 0 ],
      'Texas'      => [ 'free_shipping' => false, 'extra_supplement' => 5 ]
  ];

  const TAX_RATE_FULL    = 0.8;
  const TAX_RATE_REDUCED = 0.1;

  public static function calculatePrice($productType, $destinationState = null)
  {
      if (! isset(self::$costMap[$productType])) {
          throw new \RuntimeException(sprintf('Unsupported product type "%s" given.', $productType));
      }

      $basePrice = self::$costMap[$productType]['cost'] +
          (self::$costMap[$productType]['cost'] * self::$costMap[$productType]['tax_rate']);

      return $basePrice + self::calculateShippingCost($destinationState);
  }

  private static function calculateShippingCost($destinationState = null)
  {
      $cost = isset(self::$shippingCostMap[$destinationState]) && self::$shippingCostMap[$destinationState]['free_shipping']
          ? 0
          : 1;

      return isset(self::$shippingCostMap[$destinationState]['extra_supplement'])
          ? $cost += self::$shippingCostMap[$destinationState]['extra_supplement']
          : $cost;
  }
}

setlocale(LC_MONETARY, 'en_US.UTF-8');

echo money_format('%.2n', Price::calculatePrice(Product::TYPE_TEXT_BOOK, 'Texas')) . "\n";      // $14.25
echo money_format('%.2n', Price::calculatePrice(Product::TYPE_TEXT_BOOK, 'California')) . "\n"; // $8.25