Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Named arguments vs immutable methods #193

Open
xepozz opened this issue Mar 14, 2023 · 6 comments · May be fixed by #196
Open

Named arguments vs immutable methods #193

xepozz opened this issue Mar 14, 2023 · 6 comments · May be fixed by #196

Comments

@xepozz
Copy link
Member

xepozz commented Mar 14, 2023

What's the point to create an object every method calls?

Let's take a look at the Route class:
image

The coolest thing is you can use auto-complete feature to call any of these methods. Is it all?
I think so. Let's imagine an average corporate app with more than 100 routes.
Every application bootstrap it create this 100+ routes. But if we count it comes to 100*3.5 created objects:
100 route * 3-4 immutable methods calls, so 350 created object every bootstrap just for the one matched route in the end. That's awful.
If we open Route's constructor and let users create it instead we can decrease the created objects numbers back to 100.

class Route {
    public function __construct(
        private string $pattern,
        private array $methods,
        private string $name = null,
        private array $middlewares = [],
}

That's enough to gain performance easy.

So the way to use Route changes as the following:

Route::get('/')
  ->name('site/index')
  ->action([SiteController::index, 'index'])
  ->middlewares([AuthMiddleware::class])
;
new Route(
  pattern: '/',
  name: 'site/index',
  action: [SiteController::index, 'index'],
  middlewares: [AuthMiddleware::class],
);

All of these properties can be readonly, but it's better to later it for the next release. Also all these methods are questionable. Do we really need keep both named arguments and methods?

Due to this changes #179 turns to be real just by one line above the class #[\Attribute]

@samdark
Copy link
Member

samdark commented Mar 14, 2023

Creating an object is quite cheap but the syntax change is to the worse in my opinion.

@xepozz
Copy link
Member Author

xepozz commented Mar 14, 2023

Cheap costs are still greater than zero

@samdark
Copy link
Member

samdark commented Mar 14, 2023

Need to measure these in order to make a decision about if it makes sense to change routing syntax.

@vjik
Copy link
Member

vjik commented Mar 14, 2023

I like named arguments. More shortly, but without loss of readability.

@rustamwin
Copy link
Member

I think we should keep the current syntax.

With these changes, #179 becomes real just one line above the #[\Attribute] class

We can do it differently. I have an idea how to do it but still in my head 😄

@xepozz
Copy link
Member Author

xepozz commented Mar 16, 2023

I told a few times long ago that Route should be just a dto, but now it's kind of god object and not the best one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants