Using services to glue models¶

Published at: March 30, 2020
Author: Erlend ter Maat
Tags: Laravel, Laravel service container, Maintainable code

I work on a Feedly like application to find the boundaries of the Laravel web framework (and others in the near future). I found that sometimes when models interact with each other there is a ‘risk’ that you put responsibility for some functionality at a model where later you find that it is not really about that specific model. It may ‘depend’ on a specific context of use of the application.

User story¶

For a registered user, in order to see the items that are new since the last visit, any listing of feed items should highlight the items that are new since the last visit.

Requirements¶

Model¶

The model FeedItem and ReadStatus:

class FeedItem extends Model {

    protected $fillable = [
        'id', 'content'
    ];

    protected $casts = [
        'content' => 'array',
    ];

}

class ReadStatus extends Model {

    protected $fillable = ['user_id', 'feed_item_id'];

}

Template¶

Find the if the item has been rendered by the logged in user.

<ul class="feed-item-list">
@foreach($items as $item)
    <li class="feed-item {{ ReadStatus::where('user_id', $user->id)
                                ->where('feed_item_id, $item->id)
                                ->exists() ? 'second-time' : 'first-time' }} ">
        <a href={{ $item->content['link'] }}>{{ $item->content['title'] }}
    </li>
@endforeach
</ul>

Controller¶

Check the status is something for the template, and mark read would be something for the controller.

class HomeController extends Controller {

    public function index(Request $request)
    {
        return view('home', [
            'items' => tap(FeedItem::all(), function ($items) {
                foreach($items as $item) {
                    $item->markRead();
                }
            }),
            'user' => $request->user,
        ]);
    }

}

Note that when I update the status before the template is rendered, the item will always be read by the user by the time the status is checked. Another thing that I dont like much is that if I would repeat this code, for example, in another display of items, I would have to remember that a user object is required.

Fix the bug and get the code clean¶

Two things in the scope of clean coding need to be solved. The first is “how to first test the read status and after that mark the item read” and “how to relate the feed item to the current logged in user in a reusable way”.

Test status and mark read¶

I usually think twice when I smell a method that does more then one thing, but in this case it is not hard to defend. When the feed item is listed for the first time, it should stand out AND, obviously, marked read in order to not stand out in the future.

class FeedItem extends Model {

    public function testMarkRead() {
        $status = $this->isRead();
        if (! $status) {
            $this->markRead();
        }
        return $status;
    }

}

Laravel’s tab method allows you to inject a fixed return value, and do something before returning the given value.

class FeedItem extends Model {

    public function testMarkRead() {
        return tab($this->isRead(),
            function ($status) {
                if (! $status) {
                    $this->markRead();
                }
            }
        );
    }

}

It does not help a lot from a number of lines perspective, but I think it looks cleaner to use the tab method because it shows what it returns on one line, and what it executes after the return value is stored.

The Current user dependency¶

I am tempted to add methods on the FeedItem model that either have a user as argument, or retreive the user from the current request. To add an argument makes the code less clean: I’d have to choose the user each time I test the read status. To get the user from a request would make a model depend on something that is out of scope. The current logged in user is stored at a web request. But when another kind of context is added - like a API, we would need to change the model. In terms of the single responsibility principle this would mean that when the application adds a method to access the list of feed items, the data model should change. Ugly. To solve this I delegate this functionality to a service.

class ReadStatusService {

    protected $user;

    public function __construct(Request $request)
    {
        $this->user = $request->user();
    }

    public function isRead(FeedItem $feedItem)
    {
        if ($this->user) {
            return ReadStatus::->where('user_id', $this->user->id)
                ->where('feed_item_id', $feedItem->id)
                ->exists();
        }
    }

    public function markRead(FeedItem $feedItem)
    {
        if ($this->user) {
            ReadStatus::create([
                'user_id' => $this->user->id,
                'feed_item_id' => $feedItem->id,
            ]);
        }
    }

}

And we use the service in the model, which does not care about how the read status is collected.

class FeedItem extends Model {

    // ...

    protected function isRead() {
        return app(ReadStatusService::class)->isRead($this);
    }

    protected function markRead() {
        return app(ReadStatusService::class)->markRead($this);
    }

}

And we can clean up the controller.

class HomeController extends Controller {

    public function index()
    {
        return view('home', [
            'items' => FeedItem::all(),
        ]);
    }

}

And we use the feed items new method to get the status and mark the item read.

<ul class="feed-item-list">
@foreach($items as $item)
    <li class="feed-item {{ $item->testMarkRead() ? 'second-time' : 'first-time' ">
        <a href={{ $item->content['link'] }}>{{ $item->content['title'] }}
    </li>
@endforeach
</ul>