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

Constructors should not cause side effects #82

Closed
viglucci opened this issue Dec 31, 2017 · 2 comments
Closed

Constructors should not cause side effects #82

viglucci opened this issue Dec 31, 2017 · 2 comments

Comments

@viglucci
Copy link

viglucci commented Dec 31, 2017

Hi @agsh, first I would like to say; great package! Thank you for taking the time to put this together and for sharing it with all of us!

I've been playing around with your package lately as I learn more about the vast world of streaming IP security cameras, and for the most part, your library has worked great, and my issues have lied elsewhere.

However, in integrating with your library I have hit some integration pain points with the API that it exposes. The first being that the model objects exposed (Cam in this example) cause side effects when they are instantiated.

new Cam({
 hostname: <CAMERA_HOST>,
 username: <USERNAME>,
 password: <PASSWORD>
}, function(err) {
 // camera has connected
});

In the above example, the connect prototype method is called in the Cam constructor, which causes side effects such as firing network requests, etc.

In general, this is considered bad practice, as it prevents the consumer from separating the creation of objects, and the invocation of methods which cause side effects.

A more preferable pattern would be:

const camera = new Cam({
 hostname: <CAMERA_HOST>,
 username: <USERNAME>,
 password: <PASSWORD>
});

camera.connect((context) => {
 // camera has connected
});

Would you be open to a pull request that changes this behavior for Cam and other classes?

If so, I would recommend that once the PR was merged, a new major version of this package would be required, as changing the constructor behavior would be a breaking change to the current API contract.

Thanks for your time.

Resources for reference:
http://blog.millermedeiros.com/constructors-should-not-cause-side-effects/
https://stackoverflow.com/a/31392273

@fbertone
Copy link

I agree with you

@chriswiggins
Copy link
Collaborator

Closing out old issue - we've just merged in an autoconnect PR that addresses this

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