I was looking at some malfunctioning code the other day where the price was pulled out of one API service and sent to another API. The problem stemmed from the fact that the value coming out of the first API was as a string and the second API required the price in pence as an integer.
The difference in formats here meant that the number had to be converted from one format to another. During this process it was found that the value was sometimes out by a single pence.
For example, whilst the first API sent over a value of £20.40, the second API received a value of 2039, which is one penny out. This class did have some unit tests, but the tests but had failed to account for this rounding error.
As it turned out, this wasn't the only problem with the class in question, so I thought I would write up a quick PHP:CSI showing the problems and how I solved them.
Here is the class, I've changed the name of the class from the original and removed some of the other details, but the essential functions are in place.
<?php
class Amount {
protected $price;
/**
* Get the account price.
*
* @param string $price
* The price.
*/
public function setPrice($price) {
return $this->price = $price;
}
/**
* Get the account price.
*
* @return string
* Price as a string with decimal point but without £ prefix.
*/
public function getPrice() {
return $this->price;
}
/**
* Get the account price in pence.
*
* @return int
* Price as an integer without £ prefix.
*/
public function getPriceInPence() {
// Strip any thousand separators if present.
if (is_string($this->price)) {
$this->price = (float) str_replace(',', '', $this->price);
}
// Multiply pound value by 100 pence.
$this->price *= 100;
return (int) $this->price;
}
}
The first thing this class says to me is "I've had a long and colourful history". Sure enough, checking the git annotations shows that this class (especially the getPriceInPence() method) has been changed by at least four different developers since it was first written. Each solving one problem, making sure the unit tests still work and carrying on.
Solving The Rounding Error
The rounding error I talked about at the start is caused by a floating point precision issue that is exposed by casting the number into an integer. When the string is multiplied by 100, PHP stores it as a float value. After we cast the float into an integer we lose some of the information and the number comes out one digit less.
I found that it's possible to increase the precision of float printing by setting the precision setting in PHP to a high value, like 17. With his in place, when we print out the float using var_dump() and see the full precision of the value. From this we can therefore clearly see where the problem is introduced.
Here is a demonstration of the number above where casting a number that we thought was 2040 results in 2039.
ini_set('precision', 17);
$val = '20.40' * 100;
var_dump($val); // 2039.9999999999998
var_dump((int) $val); // 2039
Since the class had some unit tests I decided to see what they were testing. The unit test had a couple of examples like this where a value was being set and then retrieved.
public function testAccountPriceInPenceFromPriceWithCommas() {
$account = new Amount();
$account->setPrice('1,000,000');
$this->assertEquals('1,000,000', $account->getPrice());
$this->assertEquals(100000000, $account->getPriceInPence());
}
Unfortunately, the values being tested did not expose the rounding error and so everything appeared to work. As there were a couple of tests that looked like the above example I decided to remove them and replace them with a data provider.
To set up a data provider in PHPUnit we use the @dataProvider annotation in the doc block area. The value of this is the function that will be the data provider, in this case getAmounts(). The parameters sent to the test method are set up in the data provider function.
Here is the new test method, rewritten as a data provider.
/**
* Test different prices against Amounts.
*
* @dataProvider getAmounts
*/
public function testPriceAmounts($price, $priceInPence) {
$amount = new Amount();
$amount->setPrice($price);
$this->assertEquals($price, $amount->getPrice());
$this->assertEquals($priceInPence, $amount->getPriceInPence());
}
The data provider function needs to return an array, each item in the array is then mapped to the parameters of the method being tested. In the testPriceAmount() method above the first set of parameters will be '0.01' for the $price and 1 for the $priceInPence. I filled the array with the test values I found in the test class and added the a few examples I knew would break due to the rounding error.
public function getAmounts() {
return [
['0.01', 1],
['0.10', 10],
['1.00', 100],
['1,000.00', 100000],
['20.40', 2040],
['132.14', 13214],
['516.80', 51680],
['143.14', 14314],
['155.92', 15592],
['80.60', 8060],
['280.40', 28040],
['80.35', 8035],
['40.48', 4048],
['140.67', 14067],
['39.91', 3991],
];
}
Indeed, running the tests at this point meant that the first 4 items passed, but everything else failed. This was good because it game me a platform to be able to fix the problem and ensure I hadn't taken a step back.
To correct the rounding error in the Amount class we just need to round the value before casting it into an integer. This might seen unintuitive, but this removes the precision from the float so that when it is casted into an integer no data is lost. Here is the code above with the solution that shows we aren't changing the number.
ini_set('precision', 17);
$val = '20.40' * 100;
var_dump($val); // 2039.9999999999998
var_dump((int) $val); // 2039
var_dump((int) round($val)); // 2040
With this in mind we can now change the original method to correct the rounding issue.
public function getPriceInPence() {
// Strip any thousand separators if present.
if (is_string($this->price)) {
$this->price = (float) str_replace(',', '', $this->price);
}
// Multiply pound value by 100 pence.
$this->price *= 100;
return (int) round($this->price);
}
Re-running the tests now shows that everything passes! Good.
Fixing Dangerous Code
Now we can move onto fixing some of the more dangerous code in the Amount class.
Some of you may have spotted this, but when the getPriceInPence() method is called it actually changes the value of the price $price variable. This means that if we call the getPrice() method after calling getPriceInPence() we would get a different value than expected. Also, more we call getPriceInPence() the larger the $price value gets.
Here is an example of the problem in action.
$amount = new Amount();
$amount->setPrice('2040');
echo "Price :" . $amount->getPrice(); // 2040
echo "Price In Pence :" . $amount->getPriceInPence(); // 204000
echo "Price :" . $amount->getPrice(); // 204000
echo "Price In Pence :" . $amount->getPriceInPence(); // 20400000
echo "Price :" . $amount->getPrice(); // 20400000
Looking through the codebase I couldn't find anywhere where these functions were used in this way, but I'm certain that it's only a matter of time until the problems appears. The issue here is that the name of the function is getPriceInPence() and so it should be solely returning the value of the price in pence and not altering anything in the class. I therefore decided to increase the test coverage and attempt to solve this problem before it appeared.
The unit tests we added above can be updated here in order to ensure that we test the value of the price before and after calling getPriceInPence(). Due to the fact that the original price value is a string that might be formatted with commas we also needed to add an additional parameter to the method.
/**
* Test different prices against Amounts.
*
* @dataProvider getAmounts
*/
public function testPriceAmounts($price, $storedPrice, $priceInPence) {
$amount = new Amount();
$amount->setPrice($price);
$this->assertEquals($storedPrice, $amount->getPrice());
$this->assertEquals($priceInPence, $amount->getPriceInPence());
$this->assertEquals($storedPrice, $amount->getPrice());
}
We can now update the getAmounts() data provider to show the translated price value.
public function getAmounts() {
return [
['0.01', 0.01, 1],
['0.10', 0.10, 10],
['1.00', 1.00, 100],
['1,000.00', 1000.00, 100000],
['20.40', 20.40, 2040],
// snip
];
}
This causes all tests to fail at least once, but the test for the price value '1,000' failed due to the fact that we only format the price within the getPriceInPence() method.
A better way to approach the incoming price formatting is to change the value from a string into a float. This means that every time the class accepts a price value we can ensure that it meets the correct data type. We can do this by moving the str_replace() function from the getPriceInPence() method into the setPrice() method and then cast the value we receive into a float.
public function setPrice($price) {
$price = str_replace(',', '', $price);
return $this->price = (float) $price;
}
With that sorted out we can now revisit the getPriceInPence() method and reduce it down to a single line.
public function getPriceInPence() {
// Multiply pound value by 100 pence.
return (int)round($this->price * 100);
}
With that in place, all of the nests now pass. This means that the price value isn't unexpectedly mutated as the class is used.
Formatted Price
The final piece here is to ensure that we can get back the original value that we passed to the class as this was used elsewhere in the codebase. I did a quick scan of the codebase and found that in a couple of instances this class was being used to present the price to the user, but every time code was duplicated to print the number out correctly.
The formatting of the number easily done using the number_format() function in PHP. A new method added to the class allows us to do this.
public function getFormattedPrice() {
return number_format($this->price, 2);
}
The unit test can be modified to test this method along with the rest of the changes made.
/**
* Test different prices against Amounts.
*
* @dataProvider getAmounts
*/
public function testPriceAmounts($price, $storedPrice, $priceInPence) {
$amount = new Amount();
$amount->setPrice($price);
$this->assertEquals($price, $amount->getFormattedPrice());
$this->assertEquals($storedPrice, $amount->getPrice());
$this->assertEquals($priceInPence, $amount->getPriceInPence());
$this->assertEquals($storedPrice, $amount->getPrice());
}
After running this we can see that all of the tests pass. This means that we can now show the original price to the user in a standardised way. I was able to reduce the duplicated code level in the rest of the project a little by rewriting the way that the price was printed out.
Lessons
Whilst there is a lot to unpack here, I hope that the steps I have shown in this post show that I didn't just jump in and start changing things. I first ensured that I was writing tests for the things I was about to change before changing them. The resulting code is now a little bit more robust and predictable than the original.
There are, however, some key points that we can take from this exercise.
- Don't assume that because a class has tests then that class works completely.
- Don't change class properties in functions that are called 'get'. Or perhaps, be careful what you name methods as they will communicate what the method does to a developer.
- Don't just look at classes in isolation. They should be considered part of the ecosystem they are contained in and as such any changes made to the class might have knock on effects. Thankfully in my case the upstream code that uses this class had unit tests as well.
- Look out for code that has had lots of changes by many developers. It tends to point to either poor unit tests or something that is complex to understand.
- Use data providers as much as possible in your unit test classes. This allows you to plug in more edge cases without having to copy and paste test methods as they are just extra lines on your data providers.
- Finally, it's good practice in development to leave things better than you found them. My following the steps above I created less confusion for other developers and less maintenance overhead by simplifying a single class.
Add new comment