Closed kresli closed 4 years ago
I wouldn't use this in production right now. If you like what's going here, lets move all functinality from stocks to Stock and then merge it
First of all, I think the new Stocks.js
completely kills readability. I like some of the things you integrated, like using fetch, but there are a lot of things which I think should be written differently. I like the introduction for babel, but when I said that I wanted to use a constructor I did not mean:
const stock = new Stock({
+ "symbol": "MSFT",
+ "interval": "1min",
+ "apiKey": "demo",
+ "datatype": "json",
+ "outputsize": "full",
+ });
No, I meant exactly what you proposed:
const stock = new Stock({
+ "apiKey": "demo",
+ });
and then you could call:
stock.timeSeries({
+ "symbol": "MSFT",
+ "interval": "1min",
+ "datatype": "json",
+ "outputsize": "full",
});
I don't think you should create a new instance for every request you make. You should only need to create instances for every api key you use. I also don't think it is necessary to support csv
. And why are PERFORMANCES
SERIES
, INTERVALS
turned into dictionaries?
Some of the things I do like are using fetch
, .find
, .map
, better for loops and mocha tests. But i'm really questioning if using class
is better than using Stocks.prototype
, looking at readability.
tl;dr I like the main concept, but readability should be improved first before this gets merged.
I'll also create a seperate branch somewhere soon to show you the way I'd do it, and then we can find a way somewhere in between
No hard feelings. I'll try to explain why I did what I did however it's really up to you.
Why create new instance with full props?
Whenever you want to get stock data, you need to build url which need to contain all props not just apiKey so its just make sense to pass it to constructor. Because of that you can do this in feature
const stock = new Stock(props)
;
stock.listen( () => {} )
stock.listen
executing callback depend on interval. Callback is executed depend on stock.interval
.
Of course this can be done with your approach too and maybe this is not best approach. Some PROS/CONS would be nice.
Why dictionaries?
I love to see have enum, but this was workaround. You can't modify, change or add. Because the code is only wrapper around Alpha Vantage api, you stick with some dictionaries anyway.
What about readability? Personally I prefer to use class over prototype, using getter and setter over methods. But thats me.
TBH I was just releasing my own stockJS written in typescript, esnext and jest. It was on different principles which I like to point here in this PR.
Im writing from mobile, so ill give a response to your reponse later. But one more thing: i see no integration of the technicalIndicator() function yet or am i wrong? This means it isn't fully functional.
Additionally, does the code provided work for the browser as well?
So how do you indicate that you want technicalIndicator
or sectorPerformance
instead of timeSeries
when you supply all props every request? That means you have to specify more options. I'll get on some pros/cons later.
I'd like to use class > prototype as well, as we should stick to ES6 as much as possible. But I want it to be at least as readable as using prototype. I also wouldn't want multiple classes (Serializer
and Stock
) in the same file.
I dont want to break current workflow so for now I create module as plugin Stock.
import {Stock} from 'stock.js'
const stock = new Stock()
Please note, you can initialize Stock with no options. In that case, defaults settings are passed to constructor.missing features: