This is an archived snapshot of W3C's public bugzilla bug tracker, decommissioned in April 2019. Please see the home page for more details.

Bug 25388 - Boolean arguments
Summary: Boolean arguments
Status: RESOLVED WONTFIX
Alias: None
Product: Web Cryptography
Classification: Unclassified
Component: Web Cryptography API Document (show other bugs)
Version: unspecified
Hardware: PC All
: P2 normal
Target Milestone: ---
Assignee: Ryan Sleevi
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-18 08:10 UTC by Anne
Modified: 2014-05-12 05:58 UTC (History)
3 users (show)

See Also:


Attachments

Description Anne 2014-04-18 08:10:04 UTC
Boolean arguments for methods are bad news. E.g. nobody ever remembers how addEventListener's third or XMLHttpRequest.prototype.open's third argument functions.

See http://c2.com/cgi/wiki?CodeSmellMetrics

It seems most of these methods could be designed to take a dictionary of sorts instead so the argument can be named and understood.
Comment 1 Ryan Sleevi 2014-04-29 00:09:22 UTC
(In reply to Anne from comment #0)
> Boolean arguments for methods are bad news. E.g. nobody ever remembers how
> addEventListener's third or XMLHttpRequest.prototype.open's third argument
> functions.
> 
> See http://c2.com/cgi/wiki?CodeSmellMetrics
> 
> It seems most of these methods could be designed to take a dictionary of
> sorts instead so the argument can be named and understood.

I can appreciate this feedback, however, it does not seem to be terribly useful nor correct from a usability perspective.

Please note that the core API objects already take a dictionary of arguments - the Algorithm dictionary (and it's sub-classes). This constitutes the bulk of the parameters one might specify to a given operation.

Unless you're suggesting a dictionary-of-dictionaries, which itself seems a code-smell, I'm not sure how you see this being an improvement.

In all of these methods, there is only a single boolean parameter - and it has the exact same meaning across all methods - "extractable".

Possible interpretations of what you see as a "better" API include:

dictionary DeriveKeyParams {
  AlgorithmIdentifier algorithm;  // Really, "object"
  Key key;
  AlgorithmIdentifier derivedKeyType;  // Really, "object"
  boolean extractable;
  Sequence<KeyUsage> keyUsages;  // Instead of array
};

Promise<any> deriveKey(DeriveKeyParams params);

And it makes the calling convention go from
window.crypto.subtle.deriveKey({ alg }, key, { derived alg }, false, [ "encrypt" ]);

to
window.crypto.subtle.deriveKey({ algorithm: { alg }, key: key, derivedKeyType: { derived alg }, extractable: false, keyUsages: [ "encrypt" ]);

I cannot see how that's a better syntax - in part, it forgoes any possibility of conciseness, because all the dictionary attributes cannot be minified or elided.

While I can understand holy wars have been raged over positional versus named arguments, that particular criticism doesn't seem to apply here, unless I'm missing something fundamental that has changed in how Web APIs are written.
Comment 2 Mark Watson 2014-04-29 00:46:07 UTC
I interpreted the suggestion as being just to replace the extractable boolean with an enum { "extractable", "nonextractable" }.
Comment 3 Anne 2014-04-29 10:12:36 UTC
I would not suggest to nest objects, that does indeed seem bad. Can it be a keyUsages parameter with values as per Mark's suggestion?

The main problem is that if you look at a call such as

window.crypto.subtle.deriveKey({ alg }, key, { derived alg }, false, [ "encrypt" ]);

it is not clear what is going on. The same with the last argument being a length of sorts. 

I would need a much better understanding of the problem space to give more informed feedback as to what a better API would be. :-(
Comment 4 Ryan Sleevi 2014-04-29 21:00:37 UTC
(In reply to Anne from comment #3)
> I would not suggest to nest objects, that does indeed seem bad. Can it be a
> keyUsages parameter with values as per Mark's suggestion?
> 
> The main problem is that if you look at a call such as
> 
> window.crypto.subtle.deriveKey({ alg }, key, { derived alg }, false, [
> "encrypt" ]);
> 
> it is not clear what is going on. The same with the last argument being a
> length of sorts. 

A length? Not sure I follow.

> 
> I would need a much better understanding of the problem space to give more
> informed feedback as to what a better API would be. :-(

I didn't take Mark's comment as suggesting it was a KeyUsage, but in fact a new enumeration. That is

window.crypto.subtle.deriveKey({ alg }, key, {derived alg}, "extractable", [ "encrypt " ]);

I took your reply as understanding it as

window.crypto.subtle.deriveKey({alg}, key, {derived alg}, ["extractable", "encrypt"]);

The issue here is that "extractable", as a key usage, doesn't have a 1:1 mapping to a given operation - that is "extractability" ties to both exportKey and wrapKey. Calling it "exportKey" doesn't conceptually fit, since you can extract a key via wrapping. Calling it "wrapKey" is off the table, since "wrapKey" is already a usage (indicating the given key can be used to wrap key, not that it is itself wrappable)

As a style design, I can certainly understand why boolean positional arguments are bad when used in abundance - eg: foo(true, false, true); However, it seems a bit dogmatic to suggest that any boolean arguments are bad.
Comment 5 Domenic Denicola 2014-04-29 21:04:37 UTC
Are there any downsides to changing it from { true, false } to { "extractable", "nonextractable" }? There seems to be at least a slighty readability upside.
Comment 6 Domenic Denicola 2014-04-29 21:06:50 UTC
I guess the downside would be that, for objects in the spec with the `.extractable` property, you then would either:

- Have them return true/false, making them inconsistent with the methods under question
- Have them return "extractable"/"nonextractable", which seems very error prone especially since both are truthy (e.g. `if (o.extractable) { ... }` will always pass).

With this in mind I think the boolean argument, while not very good for readability, is probably the best option.
Comment 7 Ryan Sleevi 2014-05-12 05:58:12 UTC
Closing, per Comment 6