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

Positional -> Named Parameters #1

Closed
chocolateboy opened this issue Oct 21, 2015 · 6 comments
Closed

Positional -> Named Parameters #1

chocolateboy opened this issue Oct 21, 2015 · 6 comments

Comments

@chocolateboy
Copy link

I'm using spex.sequence on a project and it's working well, but the API is awkward to use IMO. Why not make the additional arguments to sequence (and other spex methods) an options object rather than trying to shoehorn everything into positional args e.g.:

Instead of:

spex.sequence(source, null, null, true);

How about:

spex.sequence(source, { track: true })

?

@vitaly-t
Copy link
Owner

I did consider using options instead of the parameters. And the decision was made based on two criteria:

  • How many parameters are there at the moment
  • Any possibility that another parameter needs to be added later

To answer the first one, 4 parameters isn't that bad, though almost a border case. If I had to add 5, I would have gone with the options.

I don't see any need in the future for adding another parameter, the current 4 ones seem comprehensive.

In addition, I would be breaking compatibility with the existing protocol, if I were to change it now, then the biggest library to suffer from this would be pg-promise.

The bottom line, if you need to call it frequently the way you showed in the example, then why not just wrap it up in a function? -

function sequence(source, track) {
    return spex.sequence(source, null, null, track);
}

or, even more generic:

function sequence(source, options) {
    options = options || {};
    return spex.sequence(source, options.dest, options.limit, options.track);
}

UPDATE: Other advantages to consider:

  • consistency with other methods - page, first of all
  • easier to document via jsDoc :)

Also, while your argument has some weight regarding method sequence, I would have to change it for the entire API, for consistency, but sequence is the only method with that 4 parameters, while all other methods are shorter.

@chocolateboy
Copy link
Author

You've forgotten one criterion: how easy is it to read/comprehend/maintain? As mentioned in that article:

sum(4, 2) is self-evident, showDialog(true, false) isn't

sequence(source, null, null, true) is cryptic: it doesn't document itself and to penetrate it one must consult the documentation.

I would be breaking compatibility with the existing protocol, if I were to change it now

Now is the perfect time to make such a change. That's what the 0.x means.

why not just wrap it up in a function

Why not optimize for clarity and convenience first and then consider providing the positional variant as the wrapper for backwards compatibility?

@vitaly-t
Copy link
Owner

That's what the 0.x means.

Actually, it doesn't mean it's a pre-release or something. It has been fully tested and released. It is production ready.

Why not optimize for clarity and convenience first and then consider providing the positional variant as the wrapper for backwards compatibility?

I was thinking about it, the other way round though, to provide a mirrored interface where all optional parameters are presented as option.

P.S. I believe that what makes a good library is good documentation, is where I'm putting in a lot of effort. With good, easy-to-read documentation it isn't really that cryptic ;)

@vitaly-t
Copy link
Owner

@chocolateboy

This has been implemented in v.0.2.5

@chocolateboy
Copy link
Author

Thanks!

@vitaly-t
Copy link
Owner

@chocolateboy you are welcome, and thanks for the idea, it does look better now.

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

No branches or pull requests

2 participants