Nsryjdtyk

I want to improve the logic for bill settlement and calculation of amount due. I managed to write code with T.D.D., but I think those loops could be improved somehow. Do you have any ideas on that front?

<?php



namespace TestsUnit;



use PHPUnitFrameworkTestCase;



class BillTest extends TestCase

{

    protected $dataSet = [

        [

            'day' => 1,

            'amount' => 50,

            'paid_by' => 'tanu',

            'friends' => ['kasun', 'tanu']

        ],

        [

            'day' => 2,

            'amount' => 100,

            'paid_by' => 'kasun',

            'friends' => ['kasun', 'tanu', 'liam']

        ],

        [

            'day' => 3,

            'amount' => 100,

            'paid_by' => 'liam',

            'friends' => ['liam', 'tanu', 'liam']

        ]

    ];



    /**

     * @test

     */

    public function it_throws_exception_if_data_is_not_a_multidimensional_array()

    {

        $this->expectException(InvalidArgumentException::class);



        new Bill();

    }



    /**

     * @test

     */

    public function it_throws_exception_if_data_is_invalid()

    {

        $this->expectException(InvalidArgumentException::class);



        $invalidData = [

            [

                'baz' => 1,

                'foo' => 50,

                'bar' => 'foo',

                'zoo' => ['bar', 'foo']

            ]

        ];



        new Bill($invalidData);

    }



    /**

     * @test

     */

    public function it_should_have_a_valid_data_set()

    {

        $expense = new Bill($this->dataSet);



        $this->assertTrue($expense->hasValidDataSet());

    }



    /**

     * @test

     */

    public function it_calculates_total_number_of_days()

    {

        $this->assertEquals(3, (new Bill($this->dataSet))->days());

    }



    /**

     * @test

     */

    public function it_calculates_total_bill_amount()

    {

        $this->assertEquals(250, (new Bill($this->dataSet))->total());

    }



    /**

     * @test

     */

    public function it_calculates_each_users_total_expense()

    {

        $expected = [

            'tanu' => 50,

            'kasun' => 100,

            'liam' => 100

        ];



        $this->assertEquals($expected, (new Bill($this->dataSet))->expenseByUsers());

    }



    /**

     * @test

     */

    public function it_calculates_due_amount_of_each_users()

    {

        $expected = [

            'tanu' => 66.66,

            'kasun' => 25,

            'liam' => 33.33

        ];



        $this->assertEquals($expected, (new Bill($this->dataSet))->dueByUsers());

    }



    /**

     * @test

     */

    public function it_calsulates_settlement_of_each_friends()

    {

        $expected = [

            'tanu' => ,

            'kasun' => [

                [

                    'from' => 'tanu',

                    'amount' => 41.66

                ],

                [

                    'from' => 'liam',

                    'amount' => 8.33

                ]

            ],

            'liam' => [

                [

                    'from' => 'tanu',

                    'amount' => 33.33,

                ]

            ]

        ];



        $this->assertEquals($expected, (new Bill($this->dataSet))->settlement());

    }

}



class Bill

{

    /**

     * Keys of the data item

     *

     * @var array

     */

    protected $keys = ['day', 'amount', 'paid_by', 'friends'];



    /**

     * Settlement data set

     *

     * @var array

     */

    protected $data;



    /**

     * Bill constructor.

     *

     * @param array $data

     */

    public function __construct($data)

    {

        if (! $this->isMultidimensionalArray($data) || ! $this->hasValidDataSet($data)) {

            $keys = implode(', ', $this->keys);



            throw new InvalidArgumentException(

                "Data should contains only {$keys} keys."

            );

        }



        $this->data = $data;

    }



    /**

     * Check data set is valid

     *

     * @param $data

     * @return bool

     */

    public function hasValidDataSet($data = null)

    {

        $data = is_null($data) ? $this->data : $data;



        foreach ($data as $item) {

            if (count(array_diff_key(array_flip($this->keys), $item))) {

                return false;

            }

        }



        return true;

    }



    /**

     * Number of days of the bill

     *

     * @return int

     */

    public function days()

    {

        return count($this->fetchValuesByKey('day'));

    }



    /**

     * Total bill amount

     *

     * @return int

     */

    public function total()

    {

        return array_sum($this->fetchValuesByKey('amount'));

    }



    /**

     * All bill paid users

     *

     * @return array

     */

    public function users()

    {

        return array_unique($this->fetchValuesByKey('paid_by'));

    }



    /**

     * Total expense amount by each user

     *

     * @return array

     */

    public function expenseByUsers()

    {

        $expense = ;



        foreach ($this->data as $item) {

            $user = $item['paid_by'];



            $expense[$user] = isset($expense[$user])

                ? $expense[$user] + $item['amount']

                : $item['amount'];

        }



        return $expense;

    }



    /**

     * Due amount of the each user

     * 

     * @return array

     */

    public function dueByUsers()

    {

        $due = ;



        foreach ($this->data as $item) {

            $share = round($item['amount'] / count($item['friends']), 2);



            $users = array_filter($item['friends'], function ($friend) use ($item) {

                return $friend != $item['paid_by'];

            });



            foreach ($users as $user) {

                $due[$user] = isset($due[$user])

                    ? $due[$user] + $share

                    : $share;

            }

        }



        return $due;

    }



    /**

     * Settlement of each user

     * 

     * @return array

     */

    public function settlement()

    {

        $settlements = ;



        $users = $this->users();

        $dueUsers = $this->dueByUsers();



        foreach ($users as $creditor) {

            $settlements[$creditor] = ;



            foreach ($dueUsers as $debtor => $due) {

                if ($creditor == $debtor) {

                    continue;

                }



                if ($dueUsers[$creditor] > $due) {

                    continue;

                }



                $owe = [

                    'from' => $debtor,

                    'amount' => $due - $dueUsers[$creditor]

                ];



                array_push($settlements[$creditor], $owe);

            }

        }



        return $settlements;

    }



    /**

     * Fetch data values by key

     *

     * @param $key

     * @return array

     */

    protected function fetchValuesByKey($key)

    {

        $values = ;



        foreach ($this->data as $item) {

            $values = $item[$key];

        }



        return $values;

    }



    /**

     * Check multidimensional array

     *

     * @param $data

     * @return bool

     */

    protected function isMultidimensionalArray($data)

    {

        return count($data) !== count($data, COUNT_RECURSIVE);

    }

}

I don't see very many improvements I would make, though I do see a couple places where native PHP functions could be used instead of manually iterating over arrays.

These changes (in the first couple blocks) shaved a few milliseconds off the total time - e.g. from ~194ms to 169ms.

filtering friends in dueByUsers()

The filtering of array elements at key friends in dueByUsers() could be simplified using array_diff().

I.E. this block:

$users = array_filter($item['friends'], function ($friend) use ($item) {

    return $friend != $item['paid_by'];

});

Could be simplified like the following:

$users = array_diff($item['friends'], [$item['paid_by']]);

fetchValuesByKey()

The method fetchValuesByKey() can be simplified to basically a call to array_column():

protected function fetchValuesByKey($key)

{

    return array_column($this->data, $key);

}

Or perhaps it would be simpler to eliminate that method and replace its usage with calls to array_column().

Array_push()

On the PHP documentation for array_push(), there is a paragraph in the description:

Note: If you use array_push() to add one element to the array it's better to use $array = because in that way there is no overhead of calling a function.

So you might want to consider doing that in the settlement() method.

Functional approaches

Functional approaches, like using array_map(), array_reduce(), etc. could be employed, although in some places it would require extra work (e.g. employing the use keyword to reference variables outside closures)) and may be slower because a function is being called.

For example, method settlement() could be re-written as:

public function settlement()

{



    $users = $this->users();

    $dueUsers = $this->dueByUsers();

    return array_reduce($users, function($settlements, $creditor) use($dueUsers) {

        $settlements[$creditor] = array_reduce(array_keys($dueUsers), function($creditorSettlements, $debtor) use ($creditor, $dueUsers) {

            $due = $dueUsers[$debtor];

            if ($creditor !== $debtor && $dueUsers[$creditor] <= $due) {

                $creditorSettlements = [

                    'from' => $debtor,

                    'amount' => $due - $dueUsers[$creditor]

                ];

            }

            return $creditorSettlements;

        }, );

        return $settlements;

    }, );