7
\$\begingroup\$

I want to know if I'm going about creating and calling two functions from my model to my controller in the simplest and cleanest way.

Model:

public function getPosts()
{
    $post = $this->paginate(4);
    return $post;
}

public function getMonth($post)
{
    $post->month = date('M', strtotime($this->created_at));
    $post->month = strtoupper($post->month);
    return $post->month;
}

public function getDay($post)
{
    $post->day = date('d', strtotime($this->created_at));
    return $post->day;
}

Controller:

public function index()
{
    $post = $this->post->getPosts();
    $post->month = $this->post->getMonth($post);
    $post->day = $this->post->getDay($post);

    return View::make('posts.index', compact('post'));
}

I am unsure about if my controller is acting in a strict MVC way, being that I thought it's only job is to direct traffic, but it's doing more by calling functions from my model. Is this the best way to go about this?

\$\endgroup\$
  • \$\begingroup\$ please include your full class so that we can see if you have done other declaration and setting and if you are extending certain classes or not - because if you are not using eloquent you have to let us know and maybe we can critic a bit more. \$\endgroup\$ – azngunit81 Apr 3 '14 at 18:43
7
\$\begingroup\$

PHP is not my area of expertise, so just some generic notes:

  1. 4 is a magic number here:

    $post = $this->paginate(4);
    

    Why is it 4? What the purpose if this number? A named constant or local variable would be readable with a descriptive name.

  2. public function getMonth($post)
    {
        $post->month = date('M', strtotime($this->created_at));
        $post->month = strtoupper($post->month);
        return $post->month;
    }
    

    Using a local variable with a proper name, like lowercase_month, in the first line would be readable and more descriptive.

  3. These methods violates Command Query Separation since they return some data and modify the $post too.

    Functions should either do something or answer something, but not both.

    Source: Clean Code by Robert C. Martin, Chapter 3: Functions, Command Query Separation p45

  4. $post->month = date('M', strtotime($this->created_at));
    $post->month = strtoupper($post->month);
    

    I'd consider moving these calls to the Post class since it seems data envy. (Pseudocode.)

    class Post {
        ...
        public void setMonth($created_at) {
            $lowercase_month = date('M', strtotime($created_at));
            $post->month = strtoupper($lowercase_month);
        }
        ...
    }
    
\$\endgroup\$
  • 1
    \$\begingroup\$ for a non expert - you are pretty complete in your answers. good job. \$\endgroup\$ – azngunit81 Apr 3 '14 at 18:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.