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

Stock as module #6

Closed
wants to merge 2 commits into from
Closed

Stock as module #6

wants to merge 2 commits into from

Conversation

kresli
Copy link

@kresli kresli commented Jul 25, 2017

  • added tests (mocha, sinon)
  • enable full ES6 syntax (import, export)
  • added node-fetch as further replacement of XmlHttpRequest

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:

  • indicators
  • performance

@kresli
Copy link
Author

kresli commented Jul 25, 2017

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

@wagenaartje
Copy link
Owner

wagenaartje commented Jul 25, 2017

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

@kresli
Copy link
Author

kresli commented Jul 25, 2017

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.

@wagenaartje
Copy link
Owner

wagenaartje commented Jul 26, 2017

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.

@ghost ghost mentioned this pull request Jan 7, 2018
@wagenaartje wagenaartje closed this Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants