Laravel model and controller interaction

Question

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?

Answer 1

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

Answer 2

Model

• I have never seen a paginator inside a model in Laravel (and if it was it wouldn't be named something so vague as a method). It's always in the controller. If you find yourself using the number 4 a lot for the number of user in different controllers, by all means store it here as a constant.
• Stop writing returns needlessly on two lines, so no more $that = x; return $that;, only return x;.
• I found getMonth and getDay intensely confusing. Does it even run like that? Instead, rewrite them to return the month and day of that particular model instance.
• They should probably also be renamed to getMonthCreated/getDayCreated, or something assuming there's some other date on the model like updated_at, which there almost certainly is. (I was too lazy to do this.)
• Use Carbon to your advantage to get the month and day in your format. Take advantage of the automatic date casting.

public function getMonth()
{
    return strtoupper($this->created_at->format('M'));
}

public function getDay()
{
    return $this->created_at->format('d');
}

Controller

• Use the view helper because it's less code and no import.
• Don't use compact so it's on one line, yet it doesn't harm the readability. Plus it's easier to find typos without it as your variable name won't be a string.
• Use the plural posts, as it's an array of more than one post.
• What exactly is up with $this->post? Why does $this have a post instead of rewriting to use Model::getPosts()?
• I removed the part where you get month/day as it's so weird. Go change your view to handle it more naturally.

public function index()
{
    return view('posts.index', ['posts' => Model::paginate(4)]);
}

View

• I have no idea what you're doing here, but a good guess is that you loop through the posts and display the date of each:

@foreach($posts as $post)
  Month: {{ $post->getMonth() }}<br>
  Day: {{ $post->getDay() }}
@endforeach

If you're putting the day and month exactly next to each other, then you should redesign a whole bunch of the code and just use $post->created_at->format('M d') (or similar).

---

Bonus

Give all your methods return types. If you have any accepting parameters, make sure those have types declared too. (This is modern PHP not available when the question was posted.)