4
\$\begingroup\$

This library is part of an accounting system. Two major models are involved. For every transaction there is an entry in the AccountTransaction model and corresponding 2 entries in the AccountJournal model. For non-cash entries, reference details go to AccountTransactionDetail model. How can the code be rewritten more efficiently? I see lot of repetition in it.

<?php

namespace App\Libraries;

use App\AccountPeriod;
use App\AccountTransaction;
use App\AccountGroup;
use App\AccountLedger;
use App\AccountJournal;
use DB;
use Auth;

class Accounts
{
    protected $entity = [];
    protected $period;
    protected $ledgerWise = false;
    protected $discounts;
    protected $balances;
    protected $ledgers = [];
    protected $reference;
    protected $amount;
    protected $paymentMode;

    /**
     * Entity involved in the transaction
     *
     * @param $entity_type
     * @param $entity_id
     * @return \App\Libraries\Accounts
     */
    public function entity($entity_type, $entity_id)
    {
        $this->entity = [
            'type' => $entity_type,
            'id' => $entity_id,
        ];
        return $this;
    }

    /**
     * Accounting period
     *
     * @param \App\AccountPeriod $period
     * @return \App\Libraries\Accounts
     */
    public function period($period)
    {
        $this->period = $period;
        return $this;
    }

    /**
     * Ledgerwise results
     *
     * @return \App\AccountPeriod
     */
    public function ledgerWise()
    {
        $this->ledgerWise = true;
        return $this;
    }

    /**
     * Calculates account balance of an entity
     *
     * Returns ledger wise balances or aggregate balance
     *
     * @return mixed
     */
    public function balances()
    {
        $balances = AccountJournal::having('total', '>', 0)
            ->whereHas('ledger', function ($q) {
                return $q->where('name', 'Accounts Receivable');
            });
        if (isset($this->period)) {
            $balances->ofPeriod($this->period);
        }
        if (!empty($this->entity)) {
            $balances->ofEntity($this->entity['type'], $this->entity['id']);
        } else {
            $balances->groupBy(['entity_type', 'entity_id']);
        }
        if ($this->ledgerWise) {
            $balances->select(['linked_ledger_id AS ledgerId', 'account_ledgers.name AS ledgerName'])
                ->join('account_ledgers', 'account_journals.linked_ledger_id', '=', 'account_ledgers.id')
                ->groupBy(['ledgerId']);
        }
        return $balances->addSelect(['entity_type', 'entity_id', DB::raw('SUM(amount) AS total')])
            ->get();
    }

    /**
     * Discounts given to account
     *
     * @param array $discounts Associative array of ledger wise discounts
     * @return \App\AccountPeriod
     */
    public function discounts($discounts)
    {
        $this->discounts = $discounts;
        return $this;
    }

    /**
     * Processes payment against an account
     *
     * Calculates balances, adjusts discounts given (if any)
     * and posts to relevant ledgers
     *
     * @return bool|void
     */
    public function pay()
    {
        if (empty($this->getEntityType())) {
            return false;
        }
        $this->processDiscounts();
        switch ($this->getMode()) {
            case 'Cash':
                $debitLedger = $this->getCashLedger();
                $tranType = 'Payment';
                break;
            case 'Bank':
                $debitLedger = $this->getBankLedger();
                $tranType = 'Payment';
                break;
            case 'Adjustment':
                $debitLedger = $this->getDiscountsLedger();
                $tranType = 'Journal';
                break;
            default:
                return false;
        }
        if ($this->getAmount() <= 0) {
            return;
        }
        DB::transaction(function() use(&$t, $debitLedger, $tranType) {
            $t = AccountTransaction::create([
                'period_id' => $this->getPeriod(),
                'entity_type' => $this->getEntityType(),
                'entity_id' => $this->getEntityId(),
                'type' => $tranType,
                'amount' => $this->getAmount(),
            ]);
            if ($debitLedger->name != 'Cash') {
                $t->detail()->create([
                    'ref_id' => $this->getReferenceId(),
                    'ref_date' => $this->getReferenceDate(),
                    'ref_desc' => $this->getReferenceDesc(),
                ]);
            }

            foreach ($this->getBalances() as $balance)
            {
                if ($this->getAmount() >= $balance->total) {
                    $amount = $balance->total;
                    $this->amount($this->getAmount() - $amount);
                } else {
                    $amount = $this->getAmount();
                    $this->amount(0);
                }
                AccountJournal::create([
                    'period_id' => $this->getPeriod(),
                    'entity_type' => $this->getEntityType(),
                    'entity_id' => $this->getEntityId(),
                    'ledger_id' => $this->getReceivableLedger()->id,
                    'linked_ledger_id' => $balance->ledgerId,
                    'transaction_id' => $t->id,
                    'amount' => -($amount),
                    'created_by' => Auth::id(),
                ]);

                AccountJournal::create([
                    'period_id' => $this->getPeriod(),
                    'entity_type' => $this->getEntityType(),
                    'entity_id' => $this->getEntityId(),
                    'ledger_id' => $debitLedger->id,
                    'linked_ledger_id' => $balance->ledgerId,
                    'transaction_id' => $t->id,
                    'amount' => $amount,
                    'created_by' => Auth::id(),
                ]);
                if ($this->getAmount() == 0) {
                    break;
                }
            }
            // If there is no balance, consider advance
            if ($this->getAmount() > 0) {
                AccountJournal::create([
                    'period_id' => $this->getPeriod(),
                    'entity_type' => $this->getEntityType(),
                    'entity_id' => $this->getEntityId(),
                    'ledger_id' => $this->getAdvancesLedger()->id,
                    'transaction_id' => $t->id,
                    'amount' => -($this->getAmount()),
                    'created_by' => Auth::id(),
                ]);

                AccountJournal::create([
                    'period_id' => $this->getPeriod(),
                    'entity_type' => $this->getEntityType(),
                    'entity_id' => $this->getEntityId(),
                    'ledger_id' => $debitLedger->id,
                    'transaction_id' => $t->id,
                    'amount' => $this->getAmount(),
                    'created_by' => Auth::id(),
                ]);
            }
            return $t;
        });
        return $t;
    }

    /**
     * Payment mode for the transaction
     *
     * @param $mode Payment mode
     * @return \App\Libraries\Accounts
     */
    public function mode($mode)
    {
        $this->paymentMode = $mode;
        return $this;
    }

    /**
     * Amount for the transaction
     *
     * @param $amount Amount
     * @return \App\Libraries\Accounts
     */
    public function amount($amount)
    {
        $this->amount = $amount;
        return $this;
    }

    /**
     * Reference details for the transaction
     *
     * @param $ref_id
     * @param null $ref_date
     * @param null $ref_desc
     * @return \App\Libraries\Accounts
     */
    public function reference($ref_id, $ref_date = null, $ref_desc = null)
    {
        if (is_array($ref_id)) {
            $this->reference = $ref_id;
        } else {
            $this->reference = [
                'id' => $ref_id,
                'date' => $ref_date,
                'desc' => $ref_desc,
            ];
        }
        return $this;
    }

    /**
     * Adjusts discounts against balances, ledger wise
     *
     * @return bool
     */
    public function processDiscounts()
    {
        if (!$this->getDiscounts()) {
            return false;
        }

        foreach ($this->getBalances() as $balance)
        {
            if (in_array($balance->ledgerId, array_keys($this->getDiscounts())) &&
                $this->getDiscounts()[$balance->ledgerId] > 0) {
                DB::transaction(function () use($balance) {
                    $t = AccountTransaction::create([
                        'period_id' => $this->getPeriod(),
                        'entity_type' => $this->getEntityType(),
                        'entity_id' => $this->getEntityId(),
                        'type' => 'Journal',
                        'amount' => $this->getDiscounts()[$balance->ledgerId],
                    ]);

                    $t->detail()->create([
                        'ref_id' => $this->getReferenceId(),
                        'ref_date' => $this->getReferenceDate(),
                        'ref_desc' => $this->getReferenceDesc(),
                    ]);


                    AccountJournal::create([
                        'period_id' => $this->getPeriod(),
                        'entity_type' => $this->getEntityType(),
                        'entity_id' => $this->getEntityId(),
                        'ledger_id' => $this->getReceivableLedger()->id,
                        'linked_ledger_id' => $balance->ledgerId,
                        'transaction_id' => $t->id,
                        'amount' => -($this->getDiscounts()[$balance->ledgerId]),
                        'created_by' => Auth::id(),
                    ]);

                    AccountJournal::create([
                        'period_id' => $this->getPeriod(),
                        'entity_type' => $this->getEntityType(),
                        'entity_id' => $this->getEntityId(),
                        'ledger_id' => $this->getDiscountsLedger()->id,
                        'linked_ledger_id' => $balance->ledgerId,
                        'transaction_id' => $t->id,
                        'amount' => $this->getDiscounts()[$balance->ledgerId],
                        'created_by' => Auth::id(),
                    ]);
                });
            }
        }
        unset($this->balances);
    }

    /**
     * Account balances
     *
     * @return mixed
     */
    public function getBalances()
    {
        if (!isset($this->balances)) {
            $this->balances = $this->ledgerWise()->balances();
        }
        return $this->balances;
    }

    /**
     * Receivable ledger
     *
     * @return \App\AccountLedger
     */
    public function getReceivableLedger()
    {
        if (!isset($this->ledgers['receivable'])) {
            $this->ledgers['receivable'] = AccountLedger::whereName('Accounts Receivable')
                ->first();
        }
        return $this->ledgers['receivable'];
    }

    /**
     * Discounts ledger
     *
     * @return \App\AccountLedger
     */
    public function getDiscountsLedger()
    {
        if (!isset($this->ledgers['discounts'])) {
            $this->ledgers['discounts'] = AccountLedger::whereName('Discounts')
                ->first();
        }
        return $this->ledgers['discounts'];
    }

    /**
     * Advances ledger
     *
     * @return \App\AccountLedger
     */
    public function getAdvancesLedger()
    {
        if (!isset($this->ledgers['advances'])) {
            $this->ledgers['advances'] = AccountLedger::whereName('Advances')
                ->first();
        }
        return $this->ledgers['advances'];
    }

    /**
     * Cash ledger
     *
     * @return \App\AccountLedger
     */
    public function getCashLedger()
    {
        if (!isset($this->ledgers['cash'])) {
            $this->ledgers['cash'] = AccountLedger::whereName('Cash')
                ->first();
        }
        return $this->ledgers['cash'];
    }

    /**
     * Bank ledger
     *
     * @return \App\AccountLedger
     */
    public function getBankLedger()
    {
        if (!isset($this->ledgers['bank'])) {
            $this->ledgers['bank'] = AccountLedger::whereName('Bank')
                ->first();
        }
        return $this->ledgers['bank'];
    }

    /**
     * Accounting period
     *
     * @return $id Accounting period id
     */
    public function getPeriod()
    {
        if (!isset($this->period)) {
            $this->period = AccountPeriod::active()->first()->id;
        }
        return $this->period;
    }

    /**
     * Entity type of transaction
     *
     * @return mixed
     */
    public function getEntityType()
    {
        return $this->entity['type'];
    }

    /**
     * Entity id of transaction
     * 
     * @return mixed
     */
    public function getEntityId()
    {
        return $this->entity['id'];
    }

    public function getDiscounts()
    {
        return isset($this->discounts) ? $this->discounts : false;
    }

    public function getReferenceId()
    {
        if(isset($this->reference)) {
            return $this->reference['id'];
        }
        return null;
    }

    public function getReferenceDate()
    {
        if(isset($this->reference)) {
            return $this->reference['date'];
        }
        return null;
    }

    public function getReferenceDesc()
    {
        if(isset($this->reference)) {
            return $this->reference['desc'];
        }
        return null;
    }

    public function getMode()
    {
        return $this->paymentMode;
    }

    public function getAmount()
    {
        return $this->amount;
    }
}
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

How can the code be rewritten more efficiently? I see lot of repetition in it.

Yes there is a lot of repetition in the code. The most common thing I spot is multiple places where AccountJournal::create() is called with an array with common elements.

In the pay method, inside the foreach loop in transaction- an array of common props to pass to AccountJournal::create() could be abstracted to a variable, or better yet, perhaps a method on class Accounts could be made to create the journal entry with parameters for the values that change - e.g. 'ledger_id', 'linked_ledger_id', 'transaction_id', 'amount'. Then that method can be called whenever a journal entry needs to be created.

The created_by value could be set automatically - e.g. by an observer or closure event handler method.

Other suggestions

Break up the elephant

The class Account is quite lengthy- partly because of methods like pay and processDiscounts. Those methods could likely be broken up into smaller methods - e.g. after pay calls processDiscounts it could call a method to set $debitLedger and $tranType based on the return value of this->getMode() instead of the somewhat long switch statement.

Consider whether it makes sense to break that monstrous class into multiple sub-classes - perhaps one for each mode (i.e. adjustment, bank, cash, etc.

Use the null coalescing operator

public function getDiscounts()
{
     return isset($this->discounts) ? $this->discounts : false;
}

Bearing in mind this code was posted at the end of 2017, hopefully the server is running PHP 7.3 or newer (at least from security standpoint given LTS) and with version 7 the null coalescing operator can be used to simplify lines like that return statement.

return $this->discounts ?? false;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.