Ben bu kodu refactor?

5 Cevap php

Kod bir görünüm tartışma sayfası içindir. Kod izleme kullanıcıya bir eklenti cevap formu göstermek için olup olmadığını belirlemek gerekiyordu.

Kullanıcı oturum ve kullanıcı tartışmanın yaratıcısı değildir eğer kullanıcı zaten tartışmaya cevap eğer, o zaman kontrol edin.

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

Bu testlerden herhangi geçmek dont, Sonra bir int olarak nedenini kaydetmek ve görünümünde bir switch deyimi bu geçmektedir.

Mantık yeterince kolay görünüyor, ama benim kod biraz özensiz görünüyor.

İşte kod .. (Kohana V2.3.4 kullanarak)

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

İşte görünüm, kullanıcı göstermek için ne mesajı belirler burada biraz mantık gidecekseniz.

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

Ben başka bir işlev ya da bir şey eklemek cevap mantığı refactor .... Bu tüm eserlerini, sadece gerçek özensiz görünüyor.

Teşekkürler

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. Teşekkürler to all for your answers

5 Cevap

Ben evet derdim. Tüm iyileştirme değerlendirmek yapıldığında o zaman, sizi zamanı ölçmek, refactor. Ne kadar zaman alır mı? Buna değer miydi? Yani bir deney olarak refactor. Ve bize sonuçlarınızı bildirin lütfen. Bottom line: Bu üstlenmeden değdi?

Evet, Refactor. PHP çıkarın ve gerçek bir dil kullanın. ;)

Cidden, refactor yapmak - if ifadeleri çok derinden yuvalama kaçınmak (bunu yaparken mantık obfuscates ve zor test yapar) ve ayrı işlevleri / yöntemleri içine yığın monolitik bölümleri.

Başka bir yerde bu proje üzerinde yazmak için kod bir kez daha hat var ise Hayır, bunun yerine üzerinde zaman harcamak.

Sık sık olduğu gibi, kod çözme aynı sorunu çözmek için farklı yollar bir ton olacaktır. Ama if you've already solved the problem sonra buraya öğrendiklerinizi not almak ve hareket. Bu kod iyi o gelişiminde daha sonra zayıf bir bağlantı olduğu ortaya çevirmek değilse; Eğer kanıt ve yeniden çarpanlarına gerektiğini somut bir doğrulama var. O zamana kadar tekerleğin yeniden icat sizin yeniden buluşun ileri doğru iterek projeyi geçirmiş olabilir zaman kaybediyoruz.

önlemek deyimi iç içe if :)

Evet, Refactor. Bu kodun karşı cyclomatic karmaşıklık analizi çalıştırırsanız, muhtemelen (kötü) oldukça yüksek bir sayı dönecekti. Tüm daha yüksek bir skora katkıda eğer iç içe ayrıntılı vaka / switch ifadeleri,.

Bu kod temeli ile çalışmak gerekebilir bir gelecek geliştirici potansiyel dalış önce bir cyclomatic karmaşıklık analizi çalıştırın, ve muhtemelen nispeten yüksek risk / karmaşıklığı Bu kod temeli ile ilgili olduğunu tahmin ediyorum.