0
\$\begingroup\$

At my new job, with new young developers, we are seriously discussing improving code structure and performance. (They keep more than 80 lines of code in a method). However I've read more tutorials, and based on my experience, I think I should use some method to wrap that code by its conditional and call it back but those methods are not reusable, so I just separated the piece of code and make conditional for each method. It may be easier for me to validate errors and easy to implement, but they find it difficult to click to find that method when they reach each conditional, so they prefer their monolithic code structure.

I want to know whether I should split the code using private methods or not.

This is their existing code:

public function postDisburse_old($loan_id = NULL)
    {
        $data = Request::except(['_token']);
        $rules = [
            'disburse_on' => 'required|date'
        ];
        $attribs = [
            'disburse_on' => 'Disburse Date',
        ];
        $validator = Validator::make($data, $rules);
        $validator->setAttributeNames($attribs);

        if ($validator->fails()) {
            return redirect()->back()->withErrors($validator);
        } else {

            $loan = Loan::with('schedule')->where('id', '=', $loan_id)->first();
            if (!empty($loan) && $loan->status == 2) {
                $loan->disburse_date = Request::input('disburse_on');
                $loan->contract_date = Request::input('contract_date');
                $loan->disburse_note = Request::input('note');
                $loan->start_date = Request::input('schedule_date');
                $loan->disburse_byuserid = Auth::user()->id;
                $loan->status = 3;

                // Update Schedule
                $holiday = [];
                if ($loan->holiday_flag == 1) {
                    $holiday = $loan->holiday;
                }
                $repayment_array = LoanCalculate::monthly_loan_schedule(
                    $loan->repayment_type, $loan->start_date, $loan->loan_duration, $loan->loan_amount, $loan->interest_rate, $loan->balloon, $loan->balloon_month,
                    $loan->monthly_payment, $loan->balloon_amount_array, $loan->custom_flag, $loan->days_of_month, $loan->holiday_flag, $holiday, $loan->schedule
                )[0];

                if (!empty($repayment_array)) {

                    $total_days = 0;
                    $total_interest = 0;
                    $total_principal = 0;
                    $total_monthly = 0;
                    $total_principal_bal = 0;

                    for ($i = 1; $i < count($repayment_array); $i++) {

                        $total_days = $total_days + $repayment_array[$i][1];
                        $total_interest += $repayment_array[$i][2];
                        $total_principal += $repayment_array[$i][3];
                        $total_monthly += $repayment_array[$i][4];
                        $total_principal_bal += $repayment_array[$i][5];
                    }

                    // Average Balance
                    $average_bal = $total_principal_bal / $loan->loan_duration;

                    if ($average_bal == 0 && $total_principal_bal == 0) {
                        Session::flash('message', 'Loan duration (Tenure) couldbe  not allow for one month');
                    } else {
                        $annual_yield = 100 * ($total_interest / $loan->loan_duration / $average_bal * 12);
                        $loan->annual_yield = $annual_yield;

                        if($loan->save()) {

                            $this->userActivity($loan->disburse_byuserid, $loan_id, 6, 'Disburse Loan');
                            RepaymentSchedule::where('loan_id', '=', $loan_id)->delete();
                            // Insert Repayment Schedule
                            for ($i = 1; $i < count($repayment_array); $i++) {
                                $data = ['loan_id' => $loan_id, 'schedule_date' => $repayment_array[$i][0], 'date_num' => $repayment_array[$i][1], 'interest' => $repayment_array[$i][2], 'principal' => $repayment_array[$i][3], 'intraday_rate' => $repayment_array[$i][6]];
                                RepaymentSchedule::insert($data);
                            }
                            $transaction = new TransactionsRequiry();
                            $transaction->id = TransactionsRequiry::select('id')->orderBy('id', 'desc')->first()->id + 1;
                            $transaction->loan_id = $loan_id;
                            $transaction->trans_date = date('Y-m-d');
                            $transaction->trans_type = "Disbursement";
                            $transaction->amount = $loan->loan_amount;
                            $transaction->description = $loan->disburse_note;
                            $transaction->balance = $loan->loan_amount;
                            $transaction->user_id = $loan->user_id;
                            $transaction->save();
                            // Update Client Loan Account
                            $loan_acc = ClientLoanAccounts::select(['id', 'activated_on', 'status','account_name'])->where('id', '=', $loan->loan_account_id)->first();
                            if (!empty($loan_acc) && count($loan_acc) > 0) {
                                $loan_acc->activated_on = date("Y-m-d");
                                $loan_acc->status = 2; // Activate(Std)
                                $loan_acc->balance = $loan->loan_amount;
                                if ($loan_acc->save()) {

                                } else {
                                    $loan->delete();
                                    $transaction->delete();
                                    Session::flash('message', 'Update client loan account information not successfully');
                                }
                            }
                            //Fee Charge and commission fee
                            $transaction1 = [];
                            if (Request::has('commission_fee') && Request::input('commission_fee') > 0) {
                                $fee_charge = new FeeCharge();
                                $fee_charge->charge_type = 4; // upfront charge
                                $fee_charge->charge_amount = floatval(Request::input('commission_fee')) * $transaction->amount / 100.0;
                                $fee_charge->charge_date = date('Y-m-d');
                                $fee_charge->receipt = Request::input('invoice_number');
                                $fee_charge->note = "Upfront charge for " . $loan->contract_id;
                                $fee_charge->save();

                                // Transaction
                                $transaction1 = new TransactionsRequiry();
                                $transaction1->id = TransactionsRequiry::select('id')->orderBy('id', 'desc')->first()->id + 1;
                                $transaction1->loan_id = $loan_id;
                                $transaction1->trans_date = date('Y-m-d');
                                $transaction1->trans_type = "Fee Charge Repayment";
                                $transaction1->amount = floatval(Request::input('commission_fee')) * $transaction->amount / 100.0;
                                $transaction1->fee = $transaction1->amount;
                                $transaction1->balance = $loan->loan_amount;
                                $transaction1->description = $fee_charge->note;
                                $transaction1->user_id = $loan->user_id;
                                if($transaction1->save()) {

                                }
                            }
                            $transaction_arr = array($transaction, $transaction1);

                            //update repayment schedule
                            $ii = 1;
                            $repayments = RepaymentSchedule::where('loan_id', $loan_id)->get();
                            foreach ($repayments as $rep) {
                                $udate = Request::input('repayment_date');
                                RepaymentSchedule::where('id', $rep->id)->update(array('schedule_date' => $udate[$ii]));
                                $ii++;
                            }

                            //journal\
                            $branch_code = Request::input('branch_code');
                            $entry_date = date('Y-m-d H:i:s');
                            $invoice_number = Request::input('invoice_number');
                            $contract_id = Request::input('contract_id');

                            for ($m = 0; $m < count(Request::input('debit')); $m++) {
                                $description = Request::input('description')[$m];
                                $transaction_id = $transaction_arr[$m]->id;

                                $journal = new JournalRequiry;
                                $journal->tran_id = $transaction_id;
                                $journal->entry_date = $entry_date;
                                $journal->invoice_number = $invoice_number;
                                $journal->description = $description;
                                $journal->user_id = $loan->user_id;

                                if ($journal->save()) {
                                    $tran = TransactionsRequiry::where('id', '=', $transaction_id)->where('flag', '=', 0)->first();
                                    if (!empty($tran)) {
                                        $tran->flag = 1;
                                        $tran->save();
                                    }
                                    for ($k = 0; $k < 2; $k++) { // 0 = debit, 1 = credit
                                        $parent_debit = Request::input('parent_debit')[$m];
                                        $parent_credit = Request::input('parent_credit')[$m];
                                        $debit = Request::input('debit')[$m];
                                        $credit = Request::input('credit')[$m];
                                        $d_description = Request::input('d_description')[$m];
                                        $c_description = Request::input('c_description')[$m];

                                        $jd = new JournalDetail;
                                        $jd->journal_id = $journal->id;
                                        $jd->coa_id = $k == 0 ? $parent_debit : $parent_credit;
                                        $jd->reference = $contract_id;
                                        $jd->branch_code = $branch_code;

                                        $prev_bl = array_fill(0, 2, 0.0);
                                        $prev_row = JournalDetail::select('b_debit', 'b_credit')
                                            ->where('coa_id', $jd->coa_id)
                                            ->orderBy('id', 'desc')
                                            ->first();
                                        if (!empty($prev_row)) {
                                            $prev_bl[0] = $prev_row->b_debit;
                                            $prev_bl[1] = $prev_row->b_credit;
                                        }
                                        $jd->p_debit = $prev_bl[0];
                                        $jd->p_credit = $prev_bl[1];
                                        $jd->debit = $k == 0 ? $debit : 0;
                                        $jd->credit = $k == 1 ? $credit : 0;
                                        $jd->b_debit = floatval($prev_bl[0]) + floatval($jd->debit);
                                        $jd->b_credit = floatval($prev_bl[1]) + floatval($jd->credit);
                                        $jd->description = $k == 0 ? $d_description : $c_description;
                                        $jd->save();
                                    }
                                }
                            }  //END $m
                        }
                    }
                    return redirect()->route('loan_detail', [$loan_id]);
                }
            }
        }
        return redirect('/');
    }

And these are my private methods, which I want to use to replace their code.

   private function Initial_LoanData($loanId)
    {
        if (!empty($loanId)) {

            $loan = Loan::with('schedule')->where('id', '=', $loanId)->first();
            if(!empty($loan) && (int)$loan->holiday_flag == 1 && (int)$loan->status == 2) {

                    $loan->disburse_date = Request::input('disburse_on');
                    $loan->contract_date = Request::input('contract_date');
                    $loan->disburse_note = Request::input('note');
                    $loan->start_date = Request::input('schedule_date');
                    $loan->disburse_byuserid = Auth::user()->id;
                    return $loan;
            } else {
                return false; //If LoadId not match
            }
        } else {
            return false;
        }
    }
    private function Initial_Repayment($loan){

        if(is_object($loan)) {

            $repayment_array = LoanCalculate::monthly_loan_schedule(
                $loan->repayment_type, $loan->start_date, $loan->loan_duration,
                $loan->loan_amount,$loan->interest_rate, $loan->balloon,
                $loan->balloon_month,$loan->monthly_payment, $loan->balloon_amount_array,
                $loan->custom_flag, $loan->days_of_month, $loan->holiday_flag, $loan->holiday, $loan->schedule
            )[0];
            if(!empty($repayment_array)) {
                $total_days = 0;
                $total_interest = 0;
                $total_principal = 0;
                $total_monthly = 0;
                $total_principal_bal = 0;

                for ($i = 1; $i < count($repayment_array); $i++) {

                    $total_days = $total_days + $repayment_array[$i][1];
                    $total_interest += $repayment_array[$i][2];
                    $total_principal += $repayment_array[$i][3];
                    $total_monthly += $repayment_array[$i][4];
                    $total_principal_bal += $repayment_array[$i][5];
                }
                $average_bal = $total_principal_bal / $loan->loan_duration;
                return [
                    'repay'=>$repayment_array,
                    'total'=>[
                        'total_days'=>$total_days,
                        'total_interest'=>$total_interest,
                        'total_principal'=>$total_principal,
                        'total_monthly'=>$total_monthly,
                        'total_principal_bal'=>$total_principal_bal,
                        'average_bal'=>$average_bal
                    ]
                ];
            }
        }
        return false;
    }
    private function Save_RepaymentSchedule($repayment_array, $loanid){

        if(!empty($repayment_array)){

            for ($i = 1; $i < count($repayment_array); $i++) {
                $data = [
                    'loan_id' => $loanid,
                    'schedule_date' => $repayment_array[$i][0],
                    'date_num' => $repayment_array[$i][1],
                    'interest' => $repayment_array[$i][2],
                    'principal' => $repayment_array[$i][3],
                    'intraday_rate' => $repayment_array[$i][6]];
                return RepaymentSchedule::insert($data);
            }
        } else {
            return false;
        }
    }
    private function Save_Transaction($loan, $loanId) {

        $transaction = new TransactionsRequiry();
        $transaction->id = TransactionsRequiry::select('id')->orderBy('id', 'desc')->first()->id + 1;
        $transaction->loan_id = $loanId;
        $transaction->trans_date = date('Y-m-d');
        $transaction->trans_type = "Disbursement";
        $transaction->amount = $loan->loan_amount;
        $transaction->description = $loan->disburse_note;
        $transaction->balance = $loan->loan_amount;
        $transaction->user_id = $loan->user_id;
        return $transaction->save();
    }
    private function Save_LoanAccount($loan){

        $loan_acc = ClientLoanAccounts::select(['id', 'activated_on', 'status', 'account_name', 'account_no'])->where('id', '=', $loan->loan_account_id)->first();
        if (!empty($loan_acc) && count($loan_acc) > 0) {

            $loan_acc->activated_on = date("Y-m-d");
            $loan_acc->status = 2; // Activate(Std)
            $loan_acc->balance = $loan->loan_amount;
            $loan_acc->save();
        }
    }
    private function Save_TransactionWithFee_Charge ($transaction, $loan, $loanId){

        $return_Val = [];
        $fee_charge = new FeeCharge();
        $fee_charge->charge_type = 4; // upfront charge
        $fee_charge->charge_amount = floatval(Request::input('commission_fee')) * $transaction->amount / 100.0;
        $fee_charge->charge_date = date('Y-m-d');
        $fee_charge->receipt = Request::input('invoice_number');
        $fee_charge->note = "Upfront charge for " . $loan->contract_id;
        $return_Val['feeCharge'] = $fee_charge->save();
        if($return_Val['feeCharge'] != false) {

            // Transaction
            $transaction1 = new TransactionsRequiry();
            $transaction1->id = TransactionsRequiry::select('id')->orderBy('id', 'desc')->first()->id + 1;
            $transaction1->loan_id = $loanId;
            $transaction1->trans_date = date('Y-m-d');
            $transaction1->trans_type = "Fee Charge Repayment";
            $transaction1->amount = floatval(Request::input('commission_fee')) * $transaction->amount / 100.0;
            $transaction1->fee = $transaction1->amount;
            $transaction1->balance = $loan->loan_amount;
            $transaction1->description = $fee_charge->note;
            $transaction1->user_id = $loan->user_id;
            $return_Val['trans'] = $transaction1->save();

            if(count($return_Val) > 0 && $return_Val['trans'] != false){

                return $return_Val;
            }else{

                return $return_Val;
            }
        }else{
            return false;
        }
    }
    private function Save_Journal($transaction_id, $description, $loan, $entry_date, $invoice_number){

        $journal = new JournalRequiry;
        $journal->tran_id = $transaction_id;
        $journal->entry_date = $entry_date;
        $journal->invoice_number = $invoice_number;
        $journal->description = $description;
        $journal->user_id = $loan->user_id;
        return $journal->save();
    }
    private function Save_journal_Detail($m, $journal, $contract_id, $branch_code){

        for ($k = 0; $k < 2; $k++) { // 0 = debit, 1 = credit
            $parent_debit = Request::input('parent_debit')[$m];
            $parent_credit = Request::input('parent_credit')[$m];
            $debit = Request::input('debit')[$m];
            $credit = Request::input('credit')[$m];
            $d_description = Request::input('d_description')[$m];
            $c_description = Request::input('c_description')[$m];

            $jd = new JournalDetail;
            $jd->journal_id = $journal->id;
            $jd->coa_id = $k == 0 ? $parent_debit : $parent_credit;
            $jd->reference = $contract_id;
            $jd->branch_code = $branch_code;

            $prev_bl = array_fill(0, 2, 0.0);
            $prev_row = JournalDetail::select('b_debit', 'b_credit')->where('coa_id', $jd->coa_id)->orderBy('id', 'desc')->first();
            if (!empty($prev_row)) {
                $prev_bl[0] = $prev_row->b_debit;
                $prev_bl[1] = $prev_row->b_credit;
            }
            $jd->p_debit = $prev_bl[0];
            $jd->p_credit = $prev_bl[1];
            $jd->debit = $k == 0 ? $debit : 0;
            $jd->credit = $k == 1 ? $credit : 0;
            $jd->b_debit = floatval($prev_bl[0]) + floatval($jd->debit);
            $jd->b_credit = floatval($prev_bl[1]) + floatval($jd->credit);
            $jd->description = $k == 0 ? $d_description : $c_description;
            return $jd->save();
        }
    }
\$\endgroup\$
6
  • 3
    \$\begingroup\$ You may be interested in How to get the best value out of Code Review - Asking Questions. Your question could be improved by editing your title, and adding a short paragraph about what the code is supposed to do. \$\endgroup\$ – tim May 12 '16 at 12:27
  • 2
    \$\begingroup\$ If there's one very important thing you should take from tim's answer, it's that you should throw exceptions instead of returning false on a failure. A function should have only one return type, not two or more. If you cannot return the type you are expecting as a result from calling a function, throw an exception. Don't abuse it just because PHP allows you to do so. I have worked on several PHP projects in the last decade and fixing functions to only return one type is usually the first refactor that is done when I come to a new project, because so often people ignore it. \$\endgroup\$ – Andy May 12 '16 at 15:57
  • \$\begingroup\$ @David Packer, Exactly, Should you show me as in this answer? \$\endgroup\$ – Heng Sopheak May 12 '16 at 16:04
  • \$\begingroup\$ @200_success, Thank for edit, I'm not good for English but I try hard to used it although I never have a chance to study at private school or any course I've used to study at public school which the poorest quality in the world I think so Cambodia government never care about Cambodian education and they are really corruption.Everything is possible for peoples who is the rich family only.Sorry to say that but it is happen in Cambodia special with poor peoples \$\endgroup\$ – Heng Sopheak May 12 '16 at 16:56
  • \$\begingroup\$ @Malachi, I come to ask which is better. Voted me down is you? and This Pages is named Code Review right? and I also don't you maintaining this code I just want to know those code is better or which one is better. Did you read all of my description and another developer commend? \$\endgroup\$ – Heng Sopheak May 12 '16 at 18:19
9
\$\begingroup\$

Yes, the original code is awful. It is 200 lines of spaghetti code; I have no idea what it does, it is impossible to reuse and extremely difficult to test, debug, or change.

That doesn't necessarily mean that the code should be changed, especially not without discussion, but that's a bit off-topic here.

Regarding your code:

Return early

By adding guard clauses to your functions and returning early, you can reduce the level of nesting, and make it more obvious when what values are returned.

So for example, instead of this:

 private function Initial_LoanData($loanId)
    {
        if (!empty($loanId)) {

            $loan = Loan::with('schedule')->where('id', '=', $loanId)->first();
            if(!empty($loan) && (int)$loan->holiday_flag == 1 && (int)$loan->status == 2) {

                    // set loan values and return the loan
            } else {
                return false; //If LoadId not match
            }
        } else {
            return false;
        }
    }

You would do this:

private function Initial_LoanData($loanId) {
    if (empty($loanId)) {
        return false;
    }

    $loan = Loan::with('schedule')->where('id', '=', $loanId)->first();

    if(empty($loan) || (int)$loan->holiday_flag !== 1 || (int)$loan->status !== 2) {
        return false;
    }

    // set loan values and return the loan
}

Error Handling

Returning false isn't a great way to handle errors. The calling method has no idea why the code failed, only that it failed. Throw exceptions instead.

Also, sometimes you don't even return false, but just ignore errors, which makes it really difficult to find bugs.

Functions

Your functions look good to me. Their scope is narrow (most of them either load or save stuff), which is how it should be.

You could further improve your code structure by putting these functions in appropriate classes. For example, Save_X and Loan_X could either go in an X class, or in an XDao.

Naming

Don't shorten variable names, it makes code hard to read. $d_description, $c_description, $jd, $prev_bl, p_debit, b_debit, and so on are very unclear.

\$\endgroup\$
1
  • \$\begingroup\$ Naming: I understand this this is the original code and I copay and separated them not modify yet how. Error Handling: I'm not much understand about error handle we should you try catch all those private method or not? and if we used try catch how should we return the data and if we found any errors how can we return those errors to the main method or I have return in catch.Should you show me in your answer? \$\endgroup\$ – Heng Sopheak May 12 '16 at 13:56
1
\$\begingroup\$

Note that visibility should be used to control how methods are accessible to clients, whether external classes or subclasses. Smaller private methods are usually by-products of refactoring your methods to be more granular, but nothing prevents you from also exposing your small methods to clients by making them public or to subclasses by making them protected.

In short, the only factor that should influence whether to use public or private for a method is whether you want to make it available to clients or not, NOT how small or big you want the method to be. (Of course, there are other benefits to writing small methods, but that is another question.)

\$\endgroup\$
5
  • \$\begingroup\$ So as your description should I keep to change or following to put all code in a method as my question? \$\endgroup\$ – Heng Sopheak May 12 '16 at 13:43
  • \$\begingroup\$ I know to used Visibility depend on big or small function but this function contain more than 100 line so I think i should used private method to separate the piece of code is better for me and another developer to maintain. \$\endgroup\$ – Heng Sopheak May 12 '16 at 13:44
  • 1
    \$\begingroup\$ It is definitely better to break the method into small ones, each fulfilling a very specific responsibility. To facilitate the decision how to organise your methods, use DbC. Decide what you want a method to achieve (not how it is done), identify what conditions need to exist for the method to succeed without throwing an exception, identify what conditions determine success, outline the steps to meet all these conditions, then implement the algorithm. If at any point in your method, any of the conditions are broken, throw an exception. \$\endgroup\$ – RWRkeSBZ May 12 '16 at 14:01
  • \$\begingroup\$ I have read your commend so helpfully for me but should you tell me about use DbC? what is that? \$\endgroup\$ – Heng Sopheak May 12 '16 at 14:11
  • 1
    \$\begingroup\$ DbC means "Design by Contract". It is an approach to designing code based on setting the conditions that need to exist before, during and after a component or function executes. The component or function does not run if the "pre" conditions are not met and throws an exception if the conditions are not met during or after execution. Please see Design by Contract. \$\endgroup\$ – RWRkeSBZ May 12 '16 at 14:21

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.