Laravel Code Review: Tests, Readability and Eloquent

I received a code snippet for review on what can be improved. First, let's write automated tests to ensure the code works, and then make some changes for readability and performance.

- - - - -
Support the channel by checking out my products:

- - - - -
Other places to follow:

[intipontt]

When you do, the 'overdueInvoices' relationship is not being eager loaded.
Same thing with the invoice.items relationship. Also, checking if an item is a product could be done in the query itself.

It's ugly, but I think there would be a performance gain from querying the users in the following way:

$users = User::query()
->has('overdueInvoices')
->with([
'overdueInvoices.items' => function ($query) {
$query->where('is_product', true);
}
])
->get();

I am not totally convinced by the last query. The mass update. Something about using a LIMIT clause in the update... Even with the scenarios in your test I am not confident.

[tetleyk]

Personally, I'd look at doing something to remove the four nested loops. That is a code smell as far as I'm concerned. Also, I'd investigate whether using collections would be more readable.

[nikulasoskarsson]

These videos are so valuable for learning as well as a yearly subscription to all courses!

[renwar]

1: you didn't use ->with('overdueInvoices') which makes n+1 problem!
2: also accessing items is also n+1.

[nipuntharuksha]

Instead of $disabledUsers we can directly use $users->toArray() since it has only ids which could remove line number 28.

[MarkJaquith]

I would move the user disabling logic to the model or to an InvoiceService class, to slim the command down and allow for other code paths to disable users without waiting for a cron job to run this command.

[phil_]

Great great video, precious resource for good design and refactoring practices.

[mandoll]

6:19 - if you add this 'query()' and the function is typed with the model you will have to change the function typing

[amirulidzham]

Thanks Povilas for the great tip. Just got something in your video

[monocatz]

I would put the possible invoice statuses in either an enum or a public const on the invoice model, so you could use something like Invoice::STATUS_UNPAID. It helps preventing typos

[GC_WK]

I perefer dont mass update in such cases because no model events triggered. If it fails then it fail whole batch. My goal is processing what we can and if errors - report it and continue loop. Speed of background job not so critical as response.

[hemantbhardwaj]

That's quit a good suggestion but it's way way faster before like 0.26s now .90s aproxx 😅

[MrFluteboy]

Shift F6 at least on the windows version of PHP storm is your friend. It will rename all instances of a variable for you.

[CJ-ewdf]

I always try to follow the return first pattern to avoid nesting where possible.

My only real nitpicking is around that final nesting of the if statement, to avoid this nesting I'd replace the following:

if($item->is_product) { ... } with if(!$item->is_product) continue;

[cobratst]

Why there is a for loop in quantities while he is using the product_id >> why he need duplicated ids on the array?

[bulent]

I'm big fun of extracting everything into small function in order to hide the logic behind a name that reveal the intention of the implemetation. So, me in the future, can read the code without trying to understand what is going on there exactly. In that particular case, I could end up having probably 5 methods.

[arifdevcoding]

why would you add "take(count($arrayOfIds))" when you arleady added "whereIn('product_id", $arrayOfIds)" ?

[BryanBurchfield]

Is there any advantage to using Carbon::today() vs simply the today() helper?

[luisuran]

Why did you add ->select('id') to $users and still works? Then you iterate on those users and use them as objects

[yungifez]

Hey Povilas do you still do code reviews

I've been working on a project for a while and I'd like you to see it and give me feedback