palantir / plottable

:bar_chart: A library of modular chart components built on D3
http://plottablejs.org/
MIT License
2.98k stars 221 forks source link

Consistent way to check for absence of arguments. #2076

Open jtlan opened 9 years ago

jtlan commented 9 years ago

Currently in our codebase we have a number of getter-setters:

public property(): Property; // getter
public property(property: Property): Class; // setter

... but we don't seem to have a consistent way to check for the absence of the argument to determine which version of the function to execute. The most common version is this:

if (property == null) {
  return this._property; // getter functionality
}
// setter logic

However, sometimes we do this instead:

if (property === undefined) {
  return this._property; // getter functionality
}
// setter logic

We should pick a convention and stick with it. One question we should answer is whether invoking methods with null should have the same effect as undefined, or if we want to save users who accidentally do property(null) when they meant property().

jtlan commented 9 years ago

For reference, the Typescript compiler uses this mechanism when a default value for an argument is specified:

if (property === void 0) {
  ...
}

This has the advantage of not being susceptible to trolling when someone redefines undefined, but it seems like that only happens on non-ES5-compliant browsers.

aicioara commented 9 years ago

I would still use double equal

if (property == void 0) {
  ...
}

That is because null !== void 0. And because sometimes we do things like

new Plottable.Interactions.PanZoom(null, yScale)

And also because we want to be backward compatible to users who decided to pass null instead of leaving the argument empty.