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

Constructor signature - new Pusher(...) #66

Closed
leggetter opened this issue Aug 11, 2015 · 6 comments
Closed

Constructor signature - new Pusher(...) #66

leggetter opened this issue Aug 11, 2015 · 6 comments

Comments

@leggetter
Copy link
Contributor

We want to keep the constructor as simple as possible. I'd suggest the use of the current Config object adds additional and unnecessary complexity.

A key part of doing this will be providing default values for things that were otherwise extracted from the URL:

  • Base URL
  • Encrypted (HTTP or HTTPS) - not actually part of the current Config, but probably should be

My vote is for one of the following:


1. Directly passing required parameters as arguments

$pusher = new Pusher('my-app-id', 'my-app-key', 'my-app-secret');

Additional options are added by a 4th options parameter:

$pusher = new Pusher('my-app-id', 'my-app-key', 'my-app-secret', array('encrypted' => true));

2. Using an Associative Array with named parameters

$pusher = new Pusher(array(appId => 'id_value', appKey' => 'key_value', 'appSecret' => 'secret_value', ...);

Optional parameters are just added to the array:

$pusher = new Pusher(array(appId => 'id_value', appKey' => 'key_value', 'appSecret' => 'secret_value', 'encrypted' => true);

The benefit of this approach is that it reduces the changes of getting key and secret mixed up.


What do others think? @zimbatm

@leggetter leggetter added this to the 3.0.0 release milestone Aug 11, 2015
@tanuck
Copy link

tanuck commented Aug 13, 2015

I would go with the first option. As long as any boolean type parameters are kept in the 4th argument array. I always feel passing boolean parameters in that way is much better for readability. I do think the Pusher\Config class should be kept though!

@leggetter
Copy link
Contributor Author

@tanuck To clarify: would you'd prefer:

use Pusher\Pusher;
use Pusher\Config;

$pusher = new Pusher(new Config('my-app-id', 'my-app-key', 'my-app-secret', array('encrypted' => true)));

For me:

  • the two use statements is one too many and an unnecessary additional step
  • it results in a bunch of extra parenthesis that people can make mistakes with results in one more annoyance on the way to getting things working

But happy to add this as a +1 for the Config based constructor.

@tanuck
Copy link

tanuck commented Aug 13, 2015

No. That option is a bit too cluttered for me. And I agree about the 2 use statements. I was happy with your first example from option 1:

$pusher = new Pusher('my-app-id', 'my-app-key', 'my-app-secret', array('encrypted' => true));

But keep the Config class for internal use. Instead of cluttered the Pusher class with additional config related properties.

It would be good to decide on this and merge the implementation before looking at #68

@leggetter
Copy link
Contributor Author

👍 Yep - good call on keeping the internal class.

Agree RE #68 too.

@tanuck
Copy link

tanuck commented Aug 13, 2015

Okay 👍

leggetter added a commit that referenced this issue Sep 2, 2015
Updates to the library constructor - #66
@leggetter
Copy link
Contributor Author

Fixed in #69.

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

2 participants