Должен ли я рефакторинг этого кода? - PullRequest
2 голосов
/ 14 апреля 2010

Код для просмотра страницы обсуждения. Предполагается, что код определяет, будет ли отображаться добавленная форма ответа для просматривающего пользователя.

Если пользователь вошел в систему, а пользователь не является создателем обсуждения, проверьте, не ответил ли пользователь уже на обсуждение.

Если пользователь еще не ответил на обсуждение, тогда покажите форму ... В противном случае, проверьте, хочет ли пользователь редактировать свой уже существующий ответ, посмотрев в URL идентификатор ответа

Если какой-либо из этих тестов не пройден, тогда я сохраняю причину как int и передаю ее в оператор switch в представлении.

Логика кажется достаточно простой, но мой код кажется немного неаккуратным.

Вот код .. (используя 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;
    }
}

Вот вид, здесь есть некоторая логика, которая определяет, какое сообщение показывать пользователю.

<!-- 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 -->

Должен ли я реорганизовать логику добавления ответа в другую функцию или что-то в этом роде ... Все это работает, это выглядит просто небрежно.

Спасибо

Редактировать: Я учел все ответы. Поскольку в тот момент я не добавлял ничего нового и успел убить, я решил провести рефакторинг кода. Подумав немного, мне пришло лучшее решение. Весь процесс занял у меня около 30 минут, и я бы сказал, что оно того стоило. Спасибо всем за ваши ответы

Ответы [ 5 ]

8 голосов
/ 14 апреля 2010

Да, рефакторинг. Удалите PHP и используйте настоящий язык. ;)

Если серьезно, то сделайте рефакторинг - избегайте вложения, если операторы так глубоко (это запутывает логику и усложняет тестирование) и разбивают монолитные секции на отдельные функции / методы.

5 голосов
/ 14 апреля 2010

Нет. Если у вас есть еще одна строка кода для написания в другом месте этого проекта, потратьте на это время.

Как это часто бывает, будет множество разных способов решить ту же проблему, которую решает ваш код. Но , если вы уже решили проблему , обратите внимание на то, что вы узнали здесь, и двигайтесь дальше. Если этот код оказывается слабым звеном в дальнейшем в разработке, тогда все в порядке; у вас есть доказательства и конкретное подтверждение того, что оно должно быть пересмотрено. До этого вы тратите время, которое можно потратить на продвижение проекта, заново изобретая свое изобретение колеса.

2 голосов
/ 14 апреля 2010

Я бы сказал, да. Проведите рефакторинг, измерьте время, которое у вас уходит, затем, когда все сделано, оцените улучшение. Сколько времени это заняло? Стоило ли? Так что рефакторинг это как эксперимент. И, пожалуйста, дайте нам знать ваши результаты. Итог: стоил ли рефакторинг?

1 голос
/ 16 апреля 2010

Да, рефакторинг. Если вы выполните циклический анализ сложности для этого кода, он, вероятно, вернет довольно большое число (плохо). Разработанные операторы case / switch, вложенные if все способствуют более высокому баллу.

Будущий разработчик, которому, возможно, понадобится работать с этой кодовой базой, потенциально может выполнить анализ цикломатической сложности перед погружением и, вероятно, оценит относительно высокий риск / сложность работы с этой кодовой базой.

1 голос
/ 14 апреля 2010

избегать вложенного оператора if :)

...