nothingworksinc / ticketbeast

Back to the lesson videos:
https://course.testdrivenlaravel.com/lessons
552 stars 11 forks source link

Query count for orders view #65

Closed stidges closed 7 years ago

stidges commented 7 years ago

Hi,

Currently in the backstage.published-concert-orders.index there are several calls to the helper methods on the Concert class. When the view is loaded, this means that about 8 count queries would be executed. While this won't be a huge performance hit, it would be interesting to see how you would go about solving an issue like this :)

Thanks!

adamwathan commented 7 years ago

Hey @stidges! I think the simplest way to handle this would be memoization, but I don't think I'm going to be able to cover it in a video lesson, just due to the size of the course and the amount of content there already is. Want to keep pushing forward and try to avoid too many side quests 😉

Here's a simple way you could implement memoization for query methods like that that have no parameters though:

    protected $memoized = [];

    protected function memoize($key, $callback)
    {
        return array_get($this->memoized, $key, function () use ($key, $callback) {
            return $this->memoized[$key] = $callback();
        });
    }

    public function ticketsRemaining()
    {
        return $this->memoize('ticketsRemaining', function () {
            return $this->tickets()->available()->count();
        });
    }

    public function ticketsSold()
    {
        return $this->memoize('ticketsSold', function () {
            return $this->tickets()->sold()->count();
        });
    }

    public function totalTickets()
    {
        return $this->memoize('totalTickets', function () {
            return $this->tickets()->count();
        });
    }

Could throw that in a trait or something even if you wanted to 👍

stidges commented 7 years ago

No problem, I understand!

I hadn't considered using memoization there, sounds like a great solution :) Thanks!

KognitoInteractive commented 7 years ago

Hi Adam,

Another question on this topic being discussed: why do you consider there's a need to hide the call to $concert->tickets()->count() behind the totalTickets() method? Right before you fleshed it out in the "Making the Progress Bar Work" video I started asking myself why would there be a need for totalTickets(), and couldn't imagine any other way in which it could be implemented; and when you in fact used tickets()->count() for it I got even more confused as to why we should hide such a simple relationship behind what seems like a bit of an unnecessary proxy method.

Thanks!

MichaelDeBoey commented 7 years ago

@KognitoInteractive I think what @adamwathan tries to do is hide every possible relationship from a public kind of view, just to make them more expressive and so the implementation details aren't 'leaking'

KognitoInteractive commented 7 years ago

Hi Michael, thanks for your answer!

Yeah, that I understand, and definitely would totally agree with... if I weren't wondering why there would be motivation for that in this case when relationships such as $foo->bar()->all(), $foo->bar()-count(), etc., etc., are already so pervasive in Laravel. Is there really such an implementation detail leakage by using such a common Laravel idiom in a relationship?

MichaelDeBoey commented 7 years ago

@KognitoInteractive Not really, but if you wanted to change te implementation of the $concert->totalTickets() function, the change wouldn't have impact on the rest of the app, using that piece. Otherwise every method that uses the $concert->tickets()->count(), would have to be changed. I agree it's a small thing to hide it here, but I think if you look at it in his whole, that's really the best way to try to do imo. 🙂

adamwathan commented 7 years ago

Hey @KognitoInteractive! I like to create little methods like that to keep all that logic in one place should it ever change. In this case for example, the concerts table has a ticket_quantity column used to generate the initial amount of tickets, but also a tickets relation.

If we didn't have a method like this, it wouldn't be as obvious what the "right" way to count the tickets was, and you might end up referencing the ticket_quantity property in some places and tickets()->count() in others. Having an explicitly defined method for it makes it easier to be consistent, and gives you one place to change that implementation if needed. It's also extremely cheap to do and doesn't add any complexity, so I think it's worth doing a lot of the time.