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

Why is $getShared = false the default? #55

Closed
kenjis opened this issue Apr 3, 2016 · 3 comments
Closed

Why is $getShared = false the default? #55

kenjis opened this issue Apr 3, 2016 · 3 comments

Comments

@kenjis
Copy link
Member

kenjis commented Apr 3, 2016

In Services, $getShared = false is the default.

    /**
     * The Response class models an HTTP response.
     */
    public static function response(App $config = null, $getShared = false)
    {
        if (! is_object($config))
        {
            $config = new App();
        }

        if (! $getShared)
        {
            return new \CodeIgniter\HTTP\Response($config);
        }

        return self::getSharedInstance('response');
    }

When I want to get the shared object, if I write Services::response(null, true);, it creates new App object.

If the code is like the following, I could write just Services::response().

    public static function response(App $config = null, $getShared = false)
    {
        if ($getShared)
        {
            return self::getSharedInstance('response');
        }

        if (! is_object($config))
        {
            $config = new App();
        }

        return new \CodeIgniter\HTTP\Response($config);
    }

And in the system code, it seems we have more true use cases than false cases.

My question is, do you have any reason that we set the default false?

@lonnieezell
Copy link
Member

Good point about the ordering there and it creating a new App instance. Definitely not needed and not intended.

My original thought on having it false, I guess, was that it should be left to the developer to decide which they wanted, so it seemed like the default should give them a non-shared one. However, the more I've worked with it and think about it, I think you're right that the majority of the time, the developer will want a shared instance. So it's probably a good idea to swap those defaults. Thanks.

@vonalbert
Copy link
Contributor

Dropping just a note here.
Maybe its better to move the $getShared parameter to the first position in the services factories. It will be easier to add new parameters to the factories and will simplify the shared_service function.

@lonnieezell
Copy link
Member

@vonalbert sorry, not a big fan of putting that variable first. While it would simplify the shared_service function, I think it makes more sense from a DX (developer experience - yeah, I know I'm getting fancy schmancy :) ) to have the parameters that directly pertain to the method they're calling first. It simply reads better when you're writing the code, and when looking at someone else's code, I think.

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

No branches or pull requests

3 participants