KseniiaMazan / stocks-api

0 stars 1 forks source link

Review #1

Open IlyaLytvynov opened 4 years ago

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/app.ts#L16 - Here i suggest to move callback in to separate controller, it makes code more readable, also let's specify resource at url, e.g. /api/v1/stocks or api/v1/prices, because /api/v1/ it's root route for any resource, when you'll need to add other resources to get/post this route will also catch the request

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/app.ts#L14 - all default exports should be at the bottom, after all code

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/types.ts#L1 here and cross all app you are duplicate keys '1. symbol' and other, I suggest to move it in string

enum export enum COMPANY_TYPES_KEYS {
  SYMBOL = '1. symbol',
  NAME = '2. name'
}

and everywhere use COMPANY_TYPES_KEYS.NAME instead of string, cause in future you solution with strings will make refactoring really tough

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/types.ts#L1 also it shouldn't be type, type - is something really simple e.g. type test = 'SOME'|'VALUE', in your case and in any case when you use some structure and wnat to add strong types for it please use interface e.g.

export interface CompanyType {
  '1. symbol': string;
  '2. name': string;
  '3. type': string;
  '4. region': string;
  '5. marketOpen': string;
  '6. marketClose': string;
  '7. timezone': string;
  '8. currency': string;
  '9. matchScore': string;
};

and better will be to use keays from previous comments

export interface CompanyType {
  [COMPANY_TYPES_KEYS.SYMBOL]: string;
  [COMPANY_TYPES_KEYS.NAME]: string;
...
IlyaLytvynov commented 4 years ago

export type CompaniesList = CompanyType[]; - here is right usage of type

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/types.ts#L21 - the same as 2 comments above

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/services/company.ts#L10 - redundant constructor, you don't need it

IlyaLytvynov commented 4 years ago

https://github.com/KseniiaMazan/stocks-api/blob/22ca61c628a3f87f6a9c35930d04df8c20e83498/src/services/company.ts#L32 - here https://www.alphavantage.co/ this part you can move in env variables, if you don't know what is it, i can show and explain how it works also i suggest to move all query logic in to separate service

 const response = await axios.get(`https://www.alphavantage.co/query?function=SYMBOL_SEARCH&keywords=${symbol.toUpperCase()}&apikey=${apikey}`);
        const companies: CompaniesList = response.data.bestMatches;

that will handle requests, and in service it self you only need to call specific method e.g

   await StockApi.queryBySymbols()