Bill settlement for multiple users

Question

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 Tests\Unit;

use PHPUnit\Framework\TestCase;

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);
    }
}

Answer 1

The code looks pretty decent. Good job making some methods that are atomic, as well as comprehensive tests for the code.

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;
    }, []);