Laravel controller for creating payments with Mollie

Question

I have a controller with functions to create and finish a Mollie payment. But I've got the idea that those functions are too big and do too many things. What I understand is that a function should do only 1 thing.

Any tips on how I can tidy this further up? Is it maybe a good moment to use events? Or are there any better solutions?

Note: creditBundles() returns an array with possible packages/bundles which are currently for sale.

public function __construct()
{
    $this->payment = Mollie::api()->payments();
}

public function createPayment(Request $request, $bundle)
{
    $bundles = $this->creditBundles();

    if (!array_key_exists($bundle, $bundles)) {
        return redirect()->route('order.credits');
    }

    $order_id = time();
    $bundle = $bundles[$bundle];

    $payment = $this->payment->create([
        "amount"            => $bundle['price'],
        "description"       => $bundle['credits'] . " credits",
        "redirectUrl"       => route('order.credits.finish', ['order_id' => $order_id]),
        "webhookUrl"      => route('order.credits.webhook'),
        "metadata"          => ['order' => 'credits', 'order_id' => $order_id],
    ]);

    CreditOrder::create([
        'user_id' => auth()->user()->id,
        'order_id' => $order_id,
        'payment_id' => $payment->id,
        'payment_status' => $payment->status,
        'credits' => $bundle['credits'],
    ]);

    return redirect($payment->getPaymentUrl());
}

public function finish(CreditOrder $order)
{
    $user = auth()->user();

    if ($order->user_id !== $user->id) {
        return redirect()->route('order.credits');
    }

    $payment = $this->payment->get($order->payment_id);

    if (!$payment->isPaid() || $order->payout) {
        return view('order_status', [
            'payment' => $payment,
            'order' => $order,
        ]);
    }

    $user->increment('credits', $order->credits);
    $order->payout = true;
    $order->save();

    notify()->flash('Thanks ' . escape($user->first_name) . '!', 'success', [
        'text' => 'We received your payment and added ' . $order->credits . ' credits to your account!'
    ]);

    return redirect()->route('home');
}

Answer 1

Yes, the Single Reponsibility Principle can apply to methods (I know that article is geared towards Swift but should hopefully still be practical in other languages).

While there are guidelines like this answer:

What's the Right Size, Really?

Depends on the language you use, but in general (and for my personal taste):

• Ideally, less than 25 lines.
• Acceptably, less than 35 lines.

there are varying opinions. I try to keep methods under 10 lines if possible.

With the method createPayment(), the lines that handle creating the payment and CreditOrder could be moved out to a separate method:

/**
* Create a payment record and a CreditOrder
* @param $bundle
* @return Payment
*/
private function _createPaymentAndOrder($bundle) 
{
    $order_id = time();
    $payment = $this->payment->create([
        "amount"            => $bundle['price'],
        "description"       => $bundle['credits'] . " credits",
        "redirectUrl"       => route('order.credits.finish', ['order_id' => $order_id]),
        "webhookUrl"      => route('order.credits.webhook'),
        "metadata"          => ['order' => 'credits', 'order_id' => $order_id],
    ]);

    CreditOrder::create([
        'user_id' => auth()->user()->id,
        'order_id' => $order_id,
        'payment_id' => $payment->id,
        'payment_status' => $payment->status,
        'credits' => $bundle['credits'],
    ]);
    return $payment
}

Then those lines can be moved out and replaced with a call to that new method:

/**
* Attempt to create a payment record
* @param Request $request
* @param $bundle
*/
public function createPayment(Request $request, $bundle)
{
    $bundles = $this->creditBundles();

    if (!array_key_exists($bundle, $bundles)) {
        return redirect()->route('order.credits');
    }

    $bundle = $bundles[$bundle];
    $payment = $this->_createPaymentAndOrder($bundle);

    return redirect($payment->getPaymentUrl());
}

The names could be modified slightly so they aren't so close.

As far as the finish() method goes, the three lines below that actually update data could be moved into a separate method, unless that feels too small.

$user->increment('credits', $order->credits);
$order->payout = true;
$order->save();

And one could move the first couple blocks that check that the user is the same as the user on the order and the payment is paid into a separate method to handle error checking. If that method returned a certain value (e.g. true to signify an error occurred, false that no error occurred) then the rest of the method could be run accordingly.