Должен ли я рефакторинг этого кода?
Код для просмотра страницы обсуждения. Предполагается, что код определяет, будет ли отображаться форма добавления ответа для просматривающего пользователя.
Если пользователь вошел в систему, и пользователь не является создателем обсуждения, проверьте, ответил ли пользователь уже на обсуждение.
Если пользователь еще не ответил на обсуждение, тогда покажите форму... В противном случае, проверьте, хочет ли пользователь изменить свой уже существующий ответ, посмотрев в 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 ответов
Я бы сказал, да. Проведите рефакторинг, измерьте время, которое у вас уходит, затем, когда все сделано, оцените улучшение. Сколько времени это заняло? Стоило ли? Так что рефакторинг это как эксперимент. И, пожалуйста, дайте нам знать ваши результаты. Итог: стоил ли рефакторинг?
Да, рефакторинг. Удалите PHP и используйте настоящий язык.;)
Если серьезно, то сделайте рефакторинг - избегайте вложения, если операторы так глубоко (это запутывает логику и усложняет тестирование) и разбивают монолитные секции на отдельные функции / методы.
Нет. Если у вас есть еще одна строка кода для написания в другом месте этого проекта, потратьте время на это.
Как это часто бывает, будет множество разных способов решить ту же проблему, которую решает ваш код. Но если вы уже решили проблему, обратите внимание на то, что вы узнали здесь, и двигайтесь дальше. Если этот код оказывается слабым звеном на более поздних этапах разработки, тогда хорошо; у вас есть доказательства и конкретное подтверждение того, что оно должно быть пересмотрено. До этого вы тратите время, которое можно потратить, продвигая проект вперед, заново изобретая свое изобретение колеса.
Да, рефакторинг. Если вы запустите цикломатический анализ сложности этого кода, он, вероятно, вернет довольно большое число (плохо). Разработанные операторы case/switch, вложенные if все способствуют более высокому баллу.
Будущий разработчик, которому, возможно, понадобится работать с этой кодовой базой, потенциально мог бы выполнить анализ цикломатической сложности перед погружением и, вероятно, предположить, что при работе с этой кодовой базой существует относительно высокий риск / сложность.