Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
<?php
# Discussion about idiomatic ActiveRecord usage
## Please help me to determine the idiomatic approach for creating a member and attaching an invoice ONLY IF the member doesn't already have 5 invoices.
class Member extends Eloquent {
}
$member = Member::create([
'email' => $email,
'password' => $password,
]);
// Then later, when they are adding invoices
// my gut says that this isn't idiomatic.. suggestions?
if ($member->invoices()->count() >= 5) {
throw new MemberCannotHaveMoreThanFiveInvoices($member->id);
}
$member->invoices()->create([
// ...
]);
@sebdesign

This comment has been minimized.

Copy link

@sebdesign sebdesign commented Sep 15, 2015

Regarding the interest creation I prefer doing $member->interests()->create(['name' => 'snowboarding']); on the first example.

@ShawnMcCool

This comment has been minimized.

Copy link
Owner Author

@ShawnMcCool ShawnMcCool commented Sep 15, 2015

Hi @sebdesign, thanks for the feedback.. Sorry that I just massively changed the example from beneath you. But, I'm using your code in an update.

@danielboendergaard

This comment has been minimized.

Copy link

@danielboendergaard danielboendergaard commented Sep 15, 2015

You don't need to call $member->save() the create method saves automatically :)

@franzliedke

This comment has been minimized.

Copy link

@franzliedke franzliedke commented Sep 15, 2015

Why should a new member have more than five instances already anyway? Or is that not necessarily happen in the same place (member creation and invoice creation)?

@ShawnMcCool

This comment has been minimized.

Copy link
Owner Author

@ShawnMcCool ShawnMcCool commented Sep 15, 2015

@franzliedke, yea the concept is mostly to show how consistency is handled. I've clarified this.

@desicochrane

This comment has been minimized.

Copy link

@desicochrane desicochrane commented Sep 15, 2015

Something like this?

<?php
class Member extends Eloquent {

    public static function register(/* Email */ $email, $password) 
    {     
        // No need for ->save(), because the ->create() method does this internally
        return $this->create([
            'email'    => $email,
            'password' => $password,
        ]);
    } 

    public function attachInvoice(Invoice $invoice) 
    {
        $this->guardInvoiceMaximum();

        $this->invoices()->save($invoice);
    }

    public function numberOfInvoices() 
    {
        return $this->invoices()->count();
    }

    protected function guardInvoiceMaximum()
    {
        if ($this->numberOfInvoices() = 5) {
            throw new MemberCannotHaveMoreThanFiveInvoices($this->id);
        }
    }
}

$member = Member::register($email, $password)->attachInvoice($invoice);
@madbonkey

This comment has been minimized.

Copy link

@madbonkey madbonkey commented Sep 15, 2015

Why not give the Member class a getter for that? $member->hasRechachedInvoiceLimit() : bool

@ShawnMcCool

This comment has been minimized.

Copy link
Owner Author

@ShawnMcCool ShawnMcCool commented Sep 15, 2015

Hi @desicochrane,

First of all, thanks for replying.

A few questions:

  1. Do you feel like this approach is idomatic?
  2. Would you actually have a guard method in your codebase or are you influence by the code sample?
  3. Do you rely on Eloquent's exception for the numberOfInvoices() if a Member isn't yet stored in the database?
@desicochrane

This comment has been minimized.

Copy link

@desicochrane desicochrane commented Sep 15, 2015

@NicolasSchneider I think I would only add that getter if there was a very good reason to expose that method to outside of the model. But exposing this, you will probably end up (or at least allowing) yourself to "ask" instead of "tell" your objects what to do, thus performing logic outside of your model that would probably would have been better encapsulated.

i.e. this is not so good:

if ($member->hasRechachedInvoiceLimit()) { // ask something
  $member->addInvoice($invoice); // then do something
}

I prefer the philosophy of "The model is responsible for protecting its own invariants". Then your app can catch the exceptions and decide at an application (not domain) level how to provide feedback to the user.

@krzystof

This comment has been minimized.

Copy link

@krzystof krzystof commented Sep 15, 2015

Not 100% sure about the idiomatic approach but from what I understand
if ($member->invoices()->has(5)) would be only using Eloquent grammar.
But the has method would have to be tweaked as the keys start by 0.

protected function has($key)
{
     return parent::has($key--);
}
@desicochrane

This comment has been minimized.

Copy link

@desicochrane desicochrane commented Sep 15, 2015

@ShaunMcCool

No problem :)

  1. I feel like as with every culture, there are many subcultures with their own idioms - and this is certainly a fitting metaphor for the developer communities. For the subculture I run with at least, I feel this is pretty idiomatic.

  2. I would have a guard method in this case because it a place to isolate the specifics of a domain rule in a generic manner. I know there is a rule concerning maximum invoices, this rule might be appropriate in other places and the specifics might change later (6 instead of 5 perhaps) now we have one place to do this.

    It's not idiomatic to have models simply be an aggregate of getters and settings (basically a glorified array), instead it is better to attach relevant behaviour and enable the model to responsible for handling its own invariants etc.

  3. This is a really good point, and depending on the team I am working with I would decide differently. Right now, if we go through the method above, then the attachInvoice method will protect against the invariant, but there is nothing to stop someone in the team from accessing the $members->invoices()->save logic directly and thus throwing our domain into an invalid state. A robust approach might be to have the guard method invoked in a pre-save eloquent model event hook (further reason to have the guard method in the model).

@torkiljohnsen

This comment has been minimized.

Copy link

@torkiljohnsen torkiljohnsen commented Sep 15, 2015

As @desicochrane pointed out: Someone "creative" could pull up an invoice model from anywhere and just add a new invoice. If you embrace Eloquent, then the pre-save invoice eloquent event (saving) could call a "too many invoices" guard.

@adamwathan

This comment has been minimized.

Copy link

@adamwathan adamwathan commented Sep 15, 2015

I agree with @desicochrane's example, though I probably wouldn't use the email value object that's sort of hinted at.

I think accessing the persistence methods of an active record outside of the active record is a common mistake that I've talked about here (towards the bottom), and in the most recent Full Stack Radio.

I would probably setup the guard like this:

public function attachInvoice(Invoice $invoice) 
{
    if ($this->hasReachedInvoiceLimit()) {
        throw new MemberCannotHaveMoreThanFiveInvoices($this->id);
    }

    $this->invoices()->save($invoice);
}

I don't know if it's "idiomatic" to do things this way; it's probably more common to see people using AR persistence methods all over the place. But I think that's more to do with frameworks like Laravel being very accessible and using AR out of the box. If there was an equally accessible entry point that used Doctrine, I'm sure you'd find a lot of Doctrine misuse that you wouldn't want to call idiomatic ;)

I think of DB access as an implementation detail when using AR. The fact that attaching an invoice involves saving a record to the database is something the outside world doesn't need to know about.

Whatever consumer needed to create the member and invoice (say a controller action) would end up looking like this:

public function store($request)
{
    try {
        $member = Member::register($request->email, $request->password);
        $member->attachInvoice(new Invoice($request->invoice_description, $request->amount);
        return SomeHttpResponse;
    } catch (MemberCannotHaveMoreThanFiveInvoices $e) {
        return SomeOtherHttpResponse;
    }
}

@JeffreyWay went over the same ideas of encapsulating DB access inside of well name AR method in a recent Laracast as well, so I think it's a common approach for less inexperienced developers.

@zackkitzmiller

This comment has been minimized.

Copy link

@zackkitzmiller zackkitzmiller commented Sep 15, 2015

@adamwathan, You're approach is close to what I would suggest, but I prefer the positive case first. What are your thoughts on something like:

public function attachInvoice(Invoice $invoice) 
{
    if ($this->canAttachInvoice()) {
        $this->invoices()->save($invoice);
    }

    throw new MemberCannotHaveMoreThanFiveInvoices($this->id);
}
@adamwathan

This comment has been minimized.

Copy link

@adamwathan adamwathan commented Sep 15, 2015

@zackkitzmiller I usually keep the happy path outdented and guard the less common cases up front, but not religious about it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.