3
\$\begingroup\$

I have written this little function to convert UK weight drams (dr) to ounces (oz) and pounds (lbs). As far as my research goes, 1 lbs = 16 oz and 1 oz = 16 dr. I need the function to return an array of lbs, oz and dr.

Am I doing it right? Or is there a better way to do it? Has my method got any flaws?

function fromDrams($drams){

    // Nullify pounds
    $ouncesAndDrams = $drams % 256;

    $lbs = (int) ($drams / 256);
    $oz  = (int) ($ouncesAndDrams / 16);
    $dr  = $ouncesAndDrams % 16;

    return ['lbs' => $lbs, 'oz' => $oz, 'dr' => $dr];
}

It does return good values when tested:

fromDrams(273) = ['lbs' => 1, 'oz' => 1, 'dr' => 1];
fromDrams(272) = ['lbs' => 1, 'oz' => 1, 'dr' => 0];
fromDrams(271) = ['lbs' => 1, 'oz' => 0, 'dr' => 15];
fromDrams(256) = ['lbs' => 1, 'oz' => 0, 'dr' => 0];
fromDrams(32) = ['lbs' => 0, 'oz' => 2, 'dr' => 0];
fromDrams(17) = ['lbs' => 0, 'oz' => 1, 'dr' => 1];
fromDrams(15) = ['lbs' => 0, 'oz' => 0, 'dr' => 15];
\$\endgroup\$
2
\$\begingroup\$

There are two things that stand out to me in your code. The first is that your integer cast seems a bit premature. When creating a general function like this, you probably do not want to round the answer, since 1.5 oz is a perfectly valid weight. A user of your function can then cast the resulting values if this is required.

The second is that your conversion values are magic numbers. These should be in constants with names that express the meaning of the number.

A class would be perfect to store the logic and constants. A Weight class would probably be a well-suited name. An Weight object can then hold the amount of weight in one specific unit and provide public methods to retrieve the weight in other units.

The Weight class that I have created here provides three static "Builder/Factory" methods to create a Weight object from. The constructor has been made private to support the use of the more expressive static function. The reasoning behind this, is that $weight = Weight::fromDrams(16); is a lot easier to understand than $weight = new Weight(16);.

Furthermore, the class provides the three public methods to retrieve the given weight for a specific unit.

class Weight {
    const POUNDS_TO_OUNZES = 16;
    const OUNZES_TO_DRAMS = 16;

    /**
     * @var float
     */
    private $drams;

    public static function fromPounds ($pounds) {
        return self::fromOunzes($pounds * self::POUNDS_TO_OUNZES);
    }

    public static function fromOunzes ($ounzes) {
        return self::fromDrams($ounzes * self::OUNZES_TO_DRAMS);
    }

    public static function fromDrams ($drams) {
        return new static($drams);
    }

    private function __construct ($drams) {
        $this->drams = $drams;
    }

    public function inPounds () {
        return $this->inOunzes() / self::POUNDS_TO_OUNZES;
    }

    public function inOunzes () {
        return $this->inDrams() / self::OUNZES_TO_DRAMS;
    }

    public function inDrams () {
        return $this->drams;
    }
}

If you still require a very specific function that returns an array of 'lbs', 'oz' and 'dr' like your question says, than this Weight class can be used in conjunction with it as such:

function fromDrams ($drams) {
    $weight = Weight::fromDrams($drams));

    return [
        'lbs' => (int) $weight->inPounds(),
        'oz' => (int) $weight->inOunzes(),
        'dr' => (int) $weight->inDrams(),
    ];
}

Here you see the Weight class not performing any rounding itself and letting the user of the class perform these when required.

This Weight class is now much more versatile than your original piece of code and can be used in many other situations. For example a website that provides conversion of an amount of weight for a specific unit to the respective weights for the other three units.

if (!empty($_POST["amount_in_drams"]))
    $weight = Weight::fromDrams($_POST["amount_in_drams"]);

if (!empty($_POST["amount_in_ounzes"]))
    $weight = Weight::fromOunzes($_POST["amount_in_ounzes"]);

if (!empty($_POST["amount_in_pounds"]))
    $weight = Weight::fromPounds($_POST["amount_in_pounds"]);

echo $weight->getPounds() . " pounds, ";
echo $weight->getOunzes() . " ounzes or ";
echo $weight->getDrams() . " drams.";
\$\endgroup\$
-1
\$\begingroup\$

I'm not convinced your method of type casting to (int) is the most reliable way to receive output. That's probably just a personal preference, but other than that I can't see any dramatic ways to improve your code.

Fundamentally, I spent a few minutes figuring out what the output represented. It's seems strange to convert one unit into two units (a new unit plus the original one). Here in the US, I've seen ounces expressed with a decimal, so you might consider just keeping your code metric -> imperial (not mixed).


A possible alternative to what you have:

Use a while loop and count up for each unit. This method is often seen is change dispenser scripts (Out of $1.74, how many quarters, nickels, pennies, etc.).

Simply declare a a variable pounds, and then in a while loop, count up $pounds++; until you cannot make a whole pound any longer.

E.g.:

$pounds = 0;

while ($drams/256 > 1) {
  $pounds++;
  $drams -= 256;
}

This results in your $pounds and then $drams/16 is ounces, optionally roundable. If keeps drams in your output is necessary, simply repeat the above code but in respect to ounces. What's let over is drams!

I hope you're alright with receiving this alternative, instead of heavy review of what you have. But in my opinion, type cast can cause you to lose some of the precision that is lost when moving to integers.

\$\endgroup\$
  • \$\begingroup\$ (ounces and drams is not mixed imperial and metric: ounces and grams would be.) I don't get the point of getting the quotient of drams to 256 by counting at all especially with the condition used in stead of `(256 <= $drams). \$\endgroup\$ – greybeard Jan 12 '16 at 23:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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