views:

272

answers:

5

The code is for a view debate page. The code is supposed to determine whether or not to show an add reply form to the viewing user.

If the user is logged in, and the user is not the creator of the debate, then check if the user already replied to the debate.

If the user did not already reply to the debate then show the form... Otherwise, Check If the user wants to edit their already existing reply by looking in the url for the reply id

If any of these tests dont pass, Then I save the reason as an int and pass that to a switch statement in the view.

The logic seems easy enough, but my code seems a little sloppy.

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;
    }
}

Heres the view, theres some logic in here that determines what message to show 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 add reply logic into another function or something.... It all works, it just seems real sloppy.

Thanks

Edit: I took all answers into consideration. Since I wasn't adding anything new at the moment and had time to kill, I chose to refactor the code. After a little thought, a better solution popped out to me. The whole process took me about 30 minutes, and I would say it was worth it. Thanks to all for your answers

+8  A: 

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

Seriously though, do refactor - avoid nesting if statements so deeply (doing so obfuscates logic & makes testing harder) and chunk monolithic sections into separate functions/methods.

fig
+1 for making jokes about the big evil
Willi
+5  A: 

No. If you've got one more line of code to write elsewhere on this project, spend your time on that instead.

As is often the case, there will be a ton of different ways to solve the same problem your code is solving. But if you've already solved the problem then take note of what you've learnt here and move on. If this code does turn out to be a weak link later on in development, then fine; you've got proof and a concrete validation that it should be re-factored. Until then you're wasting time that could be spent pushing the project forward by re-inventing your re-invention of the wheel.

MatW
-1 He isn't asking about the philosophy behind refactoring. He's asking if the code above is of good quality. I don't know why this has so many upvotes.
ryeguy
@ryeguy Pls reread the question. He asks whether he should refactor two lines above the edit.
MatW
By your logic, I hope you're marking down every other question here and not just picking on me! :) Also, why not add something positive... Like an answer.
MatW
@MatW: Yes, he asked if he should refactor the code sample below. That's it. If someone posts code and asks if it should be refactored, they're asking whether the code can be changed in a way that will make it more easily understood. It's a coding question, not a philosophical one. And actually, I upvoted every other answer in this thread, because they all gave code-related answers (to a code-related question). Just because I don't agree with your answer doesn't mean I for some reason have to provide an answer of my own.
ryeguy
The answer you gave would be to a question such as "is it worth my time to refactor something if it still works?" But the question he's asking is more like "Is there room for improvement in the below code?" Notice of the other 4 answers, all of them commented on the actual code itself - you did not.
ryeguy
When someone asks *if* they should refactor, then a philosophical answer / opinion is being requested. If they ask *how* they should refactor, then obviously a code specific answer is more pertinent. The OP asked if, not how. I don't think those that answered how are wrong at all, but I think you are being overly negative. FYI, the OP is asking **exactly** the first question you posit: If it worth his time refactoring his code, given that it still works...
MatW
+1  A: 

avoid nested if statement :)

stdnoit
+2  A: 

I'd say Yes. Refactor it, measure the time it takes you, then when all done assess the improvement. How much time did it take? Was it worth it? So refactor it as an experiment. And please let us know your results. Bottom line: was it worth refactoring?

Carl Manaster
+1  A: 

Yes, refactor. If you run a cyclomatic complexity analysis against this code, it would probably return a pretty high number (bad). Elaborate case/switch statements, nested if's all contribute to a higher score.

A future developer who may need to work with this codebase would potentially run a cyclomatic complexity analysis before diving in, and probably estimate that there is relatively high risk/complexity in dealing with this codebase.

DShultz