2010-04-13 95 views
2

该代码用于查看辩论页面。该代码应该确定是否向查看用户显示添加回复表单。我应该重构这段代码吗?

如果用户登录,并且用户不是辩论的创建者,则检查用户是否已经回复了辩论。

如果用户尚未回复辩论,然后展示形式... 否则,检查,如果用户想通过在URL中寻找回复ID

如果有任何修改他们已有的回复这些测试不通过,然后我将原因保存为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分钟,我会说这是值得的。谢谢大家的回答

回答

2

我会说是的。重构它,衡量你需要花费的时间,然后再评估改进。需要多少时间?它值得吗?所以重构它作为一个实验。请让我们知道你的结果。底线:它值得重构吗?

8

是的,重构。删除PHP并使用真实的语言。 ;)

尽管如此,重构 - 避免嵌套如果语句如此之深(这样做混淆逻辑&使测试更难)和块单片部分成单独的函数/方法。

+1

+1对于制造关于大邪恶的笑话 – whiskeysierra 2010-04-13 23:26:49

5

不可以。如果您还有一行代码可以在此项目的其他地方编写代码,请花些时间在其上。

通常情况下,将会有很多不同的方法来解决您的代码正在解决的同一问题。但是如果你已经解决了问题然后记下你在这里学到了什么并继续前进。如果这段代码在开发过程中确实是一个薄弱环节,那么很好;你有证据和具体的验证,应该重新考虑它。在那之前,你正在浪费时间,通过重新发明轮子的重新发明来推动项目前进。

+0

-1他并没有询问重构背后的哲学。他在问上面的代码质量是否好。我不知道为什么会有这么多点赞。 – ryeguy 2010-04-19 16:35:09

+0

@ryeguy请重读这个问题。他问他是否应该重构编辑上方的两行。 – Mathew 2010-04-19 17:29:05

+0

通过你的逻辑,我希望你在这里打消所有其他问题,而不仅仅是挑剔我! :)另外,为什么不添加积极的东西...就像一个答案。 – Mathew 2010-04-19 17:32:20

1

避免嵌套if语句:)

1

是的,重构。如果你对这个代码运行一个圈复杂度分析,它可能会返回一个非常高的数字(坏)。精心制作的case/switch语句,嵌套的if都有助于提高分数。

未来可能需要使用此代码库的开发人员可能会在潜入之前运行复杂度分析,并可能估计在处理此代码库时存在相对较高的风险/复杂性。