RFC: refactor overbought/oversold lines

[briancappello]

When drawing horizontal lines, by definition, there's only one y value. Currently however, for overbought/oversold lines, the y value needs to be passed in for every x value. Indicator calculations do not need knowledge of these values, only the plotters. Furthermore, there are "standard" defaults for all the indicators, so my idea is to instantiate the plotters with overrides if desired, like so:

// these are the defaults, just showing proposed syntax
var rsi = techan.plot.rsi({overbought: 70, oversold: 30})
    .xScale(xScale)
    .yScale(rsiYScale)
// everything else stays the same

There wouldn't be any BC breaks with such a change. Working code for RSI attached. What do you think?

[andredumas]

I was looking at something similar recently thinking 'why did i do it that way'. Particularly where I had defined zero as generated indicator data (macd) 😕.

I think my rationale at the time was it is 'accessor' type information, hence ending up in the accessor itself. So right now you can do this out of the box via:

techan.plot.rsi()
    .xScale(xScale)
    .yScale(rsiYScale)
    .accessor() // Dealing with accessor now
        .overbought(function() { return 70; }) // Would be better to auto wrap value with function in accessor
        .oversold(function() { return 30; }); // note an accessor is returned here not the plot!!

It's a round about way to do it, but at the time the vision was to group all these, to have accessors defined once and shared between components within techan (indicators, plots, legends and others in future) and outside in the consumers app. I generally use the library conventions and never have to think about it.

Side note a general theme for the library for optional items like this is to supply empty constructor and expose builder methods so that it would be:

techan.plot.rsi()
    .overbought(70)
    .oversold(30)
    .xScale(xScale)
    .yScale(rsiYScale)

[andredumas]

The more I think about this change the more I believe it should a change to the accessor not rely on the data to have the values https://github.com/andredumas/techan.js/blob/master/src/accessor/rsi.js#L6 rather to add the members to the plot itself. They should simply return constants so that:

      overbought = function(d) { return 70; },
      oversold = function(d) { return 30; },
      middle = function(d) { return 50; };

It's a minimal change and future proof's it for legend/tooltips.

I've already started changes to value accessor, that I have yet to push to return 0 for zero, to not rely on the data which is silly and confusing but still allows a portable redefine if required.

[briancappello]

Cool, seems like a good solution to me! Were you refactoring just the places where zero is returned, or also the overbought/middle/oversold levels?

[andredumas]

Yes, I have yet to push, but in accessor.value i've simply changed:

zero = function(d) { return d.zero; };

to

zero = function(d) { return 0; };

But after you raised this it made me think of changing them all. Basically anywhere there is a constant. And possibly add accessor factory methods on the indicators so it produces a correctly initialised read accessor (not an issue for you i believe since you use pre generated data). This will keep certain constants in the indicator in sync with what you'd use for the plot (legends, tooltips, etc). Just thinking over the design at the moment. The 0 change was a no brainer.

In the end I like the portability and reuse of the accessor, but there is a bit of indirection that may not seem natural at first.