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 parameter version of the API #20

Closed
OlsonDev opened this issue Nov 20, 2013 · 5 comments
Closed

Named parameter version of the API #20

OlsonDev opened this issue Nov 20, 2013 · 5 comments
Milestone

Comments

@OlsonDev
Copy link

This is just a generic request. I've noticed this client/API is very Boolean happy. I'd much rather see overloads which take in specific purpose enums.

I just put together a demo app and in reviewing the code I wrote yesterday, I'm having difficulty reading it. Compare what I have with what it could be.

What I have:
model.QueueDeclare(queueName, true, false, false, null);

What I could have:
model.QueueDeclare(queue:queueName, durable:true, exclusive:false, autoDelete:false, arguments:null);

What I'd like to have:
model.QueueDeclare(queueName, Durability.Durable, Scope.Public, OnEmpty.DoNotDelete);

The names of the enums are merely a suggestion -- if you can think of better names, go for it. I'd just like to see more overloads, default parameters, & enums.

Regarding default parameters, I'd even go so far as to say model.QueueDeclare(queueName) is acceptable so long as IntelliSense told me what all the defaults were.

@michaelklishin
Copy link
Member

Hi @OlsonDev.

I agree it would be nice to have a named arguments alternative but having OnEmpty.DoNotDelete when there is already a concept of auto-deleted queues seems excessive to me. In fact, Onempty.DoNotDelete is not correct,
while Scope.Public is not very descriptive. Both "auto-delete" and "exclusive" are boolean values in the
protocol.

Other clients have introduced their own terminology in the past and it always ends up confusing users. I'll file an issue
in the internal RabbitMQ bug tracker for the named argument alternative.

@michaelklishin
Copy link
Member

Internal bug number is 25891.

Using named arguments will mean moving the minimum required .NET version to 4.0. We currently support .NET 3.0
(up from 1.1 earlier this year). It may take some time before 4.0 can be considered as a baseline.

@OlsonDev
Copy link
Author

Thanks for looking at my issue.

While the enums may seem excessive, the current API seems to map the standard(s) 1-for-1 (not that I'm familiar with the standard)... which isn't bad but some abstraction could help a lot with readability -- especially with the sheer number of Booleans being passed around in this library.

Just Google around for Boolean vs. enum. You'll see a lot of results similar to this where the general consensus seems to be "prefer enums, let the compiler/JITer do the dirty work of converting to Boolean).

OnEmpty.DoNotDelete and Scope.Public were just examples -- call them what you will, whatever's most readable. I'd just prefer enums over someMethod(true, false, true, true, false). It's Microsoft's guideline... granted this was written in 2004 but a lot of it still holds true, in my opinion.

@michaelklishin michaelklishin changed the title Readable API Named parameter version of the API Feb 18, 2015
@michaelklishin
Copy link
Member

So, with .NET 4 (required in master), it is already possible to do something like

ch.QueueDeclare("", durable: false, exclusive: false, autoDelete: true, arguments: null)

This trades significant readability improvements for a tiny latency increase (for the I/O bound methods in our client). Looks like the only thing we should do is to update our tutorials to use named arguments and .NET guide to mention both ways of invoking methods.

Needless to say, I wish javac had this feature :)

@michaelklishin
Copy link
Member

Will file a separate issue for the tutorials.

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

3 participants