Jantlap's avatar

Best practice (pattern) for a method on model

Hi,

i have a question on what is the best practice pattern with this situation.

I have a Pages repository and Page model. And I need to get pageviews for all pages.

I have a simple Controller like this:

public function index(Pages $pages)
{
    $pages = $pages->all();

    return view('pages.index', [
      'pages' => $pages
    ]);
}

And in the model this method:

public function getPageviewsAttribute(){

        $cachekey = Auth::user()->id.'_pageviews_count_'.$this->id;
        $count = Cache::driver('user')->remember($cachekey, 15, function () {
            return DB::select('...')[0]->count;
        });

        return $count;
    }

This is accessor to appended pageviews property. I can then use $page->pageviews right in the view.

But this is not a good practice, is it? Too much logic in the model, with Cache, Auth and so on.

Where should this be? Not in Controller, right? In repository? How? Somewhere else? What is a good pattern?

Thank you very much.

0 likes
4 replies
wilburpowery's avatar

I see it very good. This is exactly what an accessor is for.

Snapey's avatar

The issue I see with it is that it only gets incremented if you actually get the count

Jantlap's avatar

I feel somehow that the model class itself should not contain such things like Auth::user() and any other logic outside of the model, and that it maybe would be better to move this to some method in repository like getAllWithPageviews and work with a collection there.

But i dont know.

The query is something like select count(*) AS count from pagelog where page_id = ? and there is a cache just for performance reasons. But the question is more about the pattern.

Jantlap's avatar

And what about going for something like:

$pages = $pagesRepo->all()->each(function ($item) use($pagesRepo) {
    $item->pageviews = $pagesRepo->getPageviews();
});

Please or to participate in this conversation.