Refactoring Laravel 4 App

Question

I'm in the process of refactoring my Laravel 4 app and I'd like some advice on my approach and best practice going forward. I'm trying to be as DRY as possible while also following SOLID principles.

I have an update method that updates delegates in my app. There is a many to many relationship between delegates and events, and I need to check several conditions are met when updating in order to perform the correct action/function.

For example, what I have so far checks whether the delegate is a floating delegate which isn't assigned to an event, and if it is it then needs to check event capacity, resit capacity and exam capacity, while also checking whether an event has been selected to move the delegate to.

public function update($id)
{
    $delegate = $this->delegate->findById($id);
    $input = Input::all();

    if ($delegate->isFloater())
    {
        if (empty($input['new_event_id']))
        {
            $message = (object) array(
                'title'         => 'Oops!',
                'content'       => 'You did not select an event to move this floating delegate to.',
                'alert_type'    => 'error'
            );

            return Redirect::back()->withInput()->with('message', $message);
        }

        $newEvent = $this->event->findById($input['new_event_id']);

        if ( ! $newEvent->hasCapacity())
        {
            $message = (object) array(
                'title'         => 'Oops!',
                'content'       => 'The event you are trying to move this floating delegate to has no availability.',
                'alert_type'    => 'error'
            );

            return Redirect::back()->withInput()->with('message', $message);
        }

        if ( ! $newEvent->hasResitCapacity($input))
        {
            $message = (object) array(
                'title'         => 'Oops!',
                'content'       => 'The event you are trying to move this floating delegate to has no resit spaces left.',
                'alert_type'    => 'error'
            );

            return Redirect::back()->withInput()->with('message', $message);
        }

        if ( ! $newEvent->hasExamCapacity($input))
        {
            $message = (object) array(
                'title'         => 'Oops!',
                'content'       => 'The event you are trying to move this floating delegate to has no exam spaces left.',
                'alert_type'    => 'error'
            );

            return Redirect::back()->withInput()->with('message', $message);
        }

        $this->delegate->moveFloater($id, $input, $newEvent);

        $message = (object) array(
            'title'         => 'Excellent!',
            'content'       => 'This floating delegate was successfully moved.',
            'alert_type'    => 'success'
        );

        return Redirect::to('admin/events')->with('message', $message);
    }
}

It's certainly much better than what I had previously, and I am abstracting code to various model methods such as hasCapacity() and isFloater() etc but already the method is big, and I'm repeating code with regards to the messages and redirects.

I'm unsure at the moment how best to approach this going forward and I'm looking for some advice on how to proceed and clean it up further.

Any help would be much appreciated. Thanks in advance.

Answer 1

First of all, whether or not a delegate can be moved is business/domain logic and does not belong in the controller. Move all the logic related to that into the repository/service class which returns false if something goes wrong, then use a getError method to get an error message.

Second, the construction of the 'messages' flash variable can be abstracted into a protected method achieve DRY.

Third, "update" to describe a controller action which is clearly just aimed at moving a delegate from one event to another is misleading, so I'd rename it to just "move".

public function move($id)
{
    $delegate = $this->delegate->findById($id);

    if (!$delegate) {
        return $this->notFound();
        // or something like this...
        App::abort(404);
    }

    $input = Input::all();

    if (!$this->delegate->move($delegate, $input)) {
        $error = $this->delegate->getError();
        $message = $this->wrapMessage('error', 'Oops!', $error)
        return Redirect::back()->withInput()->with('message', $message);
    }

    $message = $this->wrapMessage('success', 'Excellent!', 'This floating delegate was successfully moved.');
    return Redirect::to('admin/events')->with('message', $message);
}

protected function wrapMessage($type, $title, $content)
{
    return (object) array(
        'title'      => $title,
        'content'    => $content,
        'alert_type' => $type
    );
}