laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.62k stars 11.03k forks source link

refactor: Some minor performance & readability enhancements #53596

Closed dshafik closed 16 hours ago

dshafik commented 1 day ago

During my latest Inside Laravel livestream I noticed some minor inefficiencies in the Dispatcher code. These most likely are just older code that never got updated.

This includes 3 changes:

  1. Useisset($uses[InteractsWithQueue::class], $uses[Queueable::class]) instead of a two expensive in_array() calls, this takes two O(n) operations to just O(1).
  2. It replaces an expensive call_user_func() call with a dynamic method call, using ($this->queueResolver) to dererefence the property before calling the closure within.
  3. Simplifies the pushCommandToQueue() implementation which previously checked if queue and/or delay were set and then calling pushOn() or laterOn() when with named params we can easily just call push() and later() — the use of queue: $command->queue ?? null rather than just queue: $command->queue is for readability. This one is the only one I'm concerned about, it's possible that an implementation of the Queue interface/child of the Queue abstract class has different behavior than just wrapping push() and later() — but as those must support the queue param I don't see any issue with it.
milwad-dev commented 1 day ago

The refactored version is harder to read than the previous version! (For me)

Diddyy commented 1 day ago

In contrast, I like it and can follow it better. I think it helps keep a clean code base so +1!

crynobone commented 1 day ago

Marking as draft since tests are failing.

dshafik commented 1 day ago

@crynobone all tests are passing now, only one minor change to a single test (mocking laterOn which is no longer called instead of later)