Should I refactor this code?

The code is for viewing the talk page. The code should determine whether or not to show the response form to the viewer.

If the user is signed in and the user is not the thread creator, check if the user has replied to the thread.

If the user has not yet responded to the debate then show the form ... Otherwise, check if the user wants to edit an already existing response by looking at the response id url

If any of these tests fail, then I store the reason as an int and pass that to the switch statement in the view.

The logic seems simple enough, but my code looks a bit messy.

Here's the code .. (using Kohana V2.3.4)

public function view($id = 0)
{
    $debate = ORM::factory('debate')->with('user')->with('category')->find($id);

    if ($debate->loaded == FALSE)
    {
        url::redirect();
    }

    // series of tests to show an add reply form
    if ($this->logged_in)
    {
        // is the viewer the creator?
        if ($this->user->id != $debate->user->id)
        {
            // has the user already replied?
            if (ORM::factory('reply')
                ->where(array('debate_id' => $id, 'user_id' => $this->user->id))
                ->count_all() == 0)
            {
                $form = $errors = array
                (
                    'body'      => '',
                    'choice_id' => '',
                    'add'       => ''
                );

                if ($post = $this->input->post())
                {
                    $reply = ORM::factory('reply');

                    // validate and insert the reply
                    if ($reply->add($post, TRUE))
                    {
                        url::redirect(url::current());
                    }

                    $form   = arr::overwrite($form,   $post->as_array());
                    $errors = arr::overwrite($errors, $post->errors('reply_errors'));
                }
            }
            // editing a reply?
            else if (($rid = (int) $this->input->get('edit'))
                    AND ($reply = ORM::factory('reply')
                                ->where(array('debate_id' => $id, 'user_id' => $this->user->id))
                                ->find($rid)))
            {
                $form = $errors = array
                (
                    'body'      => '',
                    'choice_id' => '',
                    'add'       => ''
                );

                // autocomplete the form
                $form = arr::overwrite($form, $reply->as_array());

                if ($post = $this->input->post())
                {
                    // validate and insert the reply
                    if ($reply->edit($post, TRUE))
                    {
                        url::redirect(url::current());
                    }

                    $form   = arr::overwrite($form,   $post->as_array());
                    $errors = arr::overwrite($errors, $post->errors('reply_errors'));
                }
            }
            else
            {
                // user already replied
                $reason = 3;
            }
        }
        else
        {
            // user started the debate
            $reason = 2;
        }
    }
    else
    {
        // user is not logged in.
        $reason = 1;
    }

    $limits = Kohana::config('app/debate.limits');
    $page   = (int) $this->input->get('page', 1);
    $offset = ($page > 0) ? ($page - 1) * $limits['replies'] : 0;

    $replies = ORM::factory('reply')->with('user')->with('choice')->where('replies.debate_id', $id);

    $this->template->title  = $debate->topic;
    $this->template->debate = $debate;
    $this->template->body   = View::factory('debate/view')
        ->set('debate',     $debate)
        ->set('replies',    $replies->find_all($limits['replies'], $offset))
        ->set('pagination', Pagination::factory(array
            (
                'style'          => 'digg',
                'items_per_page' => $limits['replies'],
                'query_string'   => 'page',
                'auto_hide'      => TRUE,
                'total_items'    => $total = $replies->count_last_query()
            ))
        )
        ->set('total', $total);

    // are we showing the add reply form?
    if (isset($form, $errors))
    {
        $this->template->body->add_reply_form = View::factory('reply/add_reply_form')
            ->set('debate', $debate)
            ->set('form',   $form)
            ->set('errors', $errors);
    }
    else
    {
        $this->template->body->reason = $reason;
    }
}

      

This view, this is the logic that determines which message to show to the user.

<!-- Add Reply Form -->
<?php if (isset($add_reply_form)): ?>

    <?php echo $add_reply_form; ?>

<?php else: ?>

    <?php
        switch ($reason)
        {
            case 1 :
                // not logged in, show a message
                $message = 'Add your ' . html::anchor('login?url=' . url::current(TRUE), '<b>vote</b>') . ' to this discussion';
                break;

            case 2 :
                // started the debate. dont show a message for that.
                $message = NULL;
                break;

            case 3:
                // already replied, show a message
                $message = 'You have already replied to this debate';
                break;

            default:
                // unknown reason. dont show a message
                $message = NULL;
                break;
        }
    ?>

    <?php echo app::show_message($message, 'h2'); ?>

<?php endif; ?>
<!-- End Add Reply Form -->

      

Should I refactor the logic for adding the response to another function or whatever ... Everything works, it just seems sloppy.

thanks

Edit: I've taken all the answers into account. Since I hadn't added anything new at the moment and had time to kill, I decided to refactor the code. After a little thought, the best solution came out to me. The whole process took me about 30 minutes and I would say it was worth it. Thanks everyone for your answers

+2


a source to share


5 answers


I would say yes. Refactor it, measure the time it takes you, then when everyone has evaluated the improvement. How much time did it take? Was it worth it? So reorganize it as an experiment. And please let us know your results. Bottom line: was refactoring worth it?



+2


a source


Yes, refactoring. Remove PHP and use the real language.;)



Seriously, however, refactoring is to avoid nesting if the assertions are so deep (which makes spoofing logic and makes testing harder), and to split monolithic sections into separate functions / methods.

+8


a source


No. If you have another line of code to write elsewhere in this project, take the time to do this.

As is often the case, there will be many different ways to solve the same problem your code solves. But if you have already solved the problem, pay attention to what you learned here and keep driving. If this code becomes a weak link in development in the future, it will be good; you have proof and specific verification that it should be re-accounted for. Until then, you are wasting time that could be wasted pushing the project forward by reinventing your reinvention of the wheel.

+5


a source


avoid nested if statement:

+1


a source


Yes, refactoring. If you loop through complexity with this code, it will probably return a fairly large number (bad). Design case / switch statements nested if they all contribute to a higher score.

A prospective developer who might need to work with this codebase could potentially do a cyclometric complexity analysis before diving in and perhaps appreciate that there is a relatively high risk / complexity in working with this codebase.

+1


a source







All Articles