[REVIEW] of fileToReview.js #5
Conversation
| @@ -1,5 +1,10 @@ | |||
|
|
|||
| var domain = "bank.local.fr" | |||
There was a problem hiding this comment.
use 'const' instead of 'var'. Var have a biggest litter than const and should be avoided if possible. (use const / let instead)
Same for the next code
I have also put it inside a config object for more clarity since i have also merged common headers in this object.
Next step will be to set them in ENV vars for a better management
|
|
||
| if (jws) { |
There was a problem hiding this comment.
I have factorized the condition. Since just one value is changing inside the IF / ELSE condition,
I have extracted common values inside the "config" object and create a ternary (be careful with ternary, the condition must remain clear, otherwise use a common IF / ELSE, in this case it's readable)
|
|
||
| var {code, response } = await doRequest('GET', |
There was a problem hiding this comment.
The url is not clear at all, and can be factorized as : ${config.domain}/accounts/${id}/transactions?page=${page}
|
|
||
| if (response && code == 200 && response.data) { |
There was a problem hiding this comment.
When you have lot of condition to validate before doing an action, check the negativity of the condition first to avoid a IF inside another IF etc...
The max depth are commonly 2 or 3 IF inside each others before the clarity is impacted
|
|
||
| if (response && code == 200 && response.data) { | ||
| if (response.data.meta) { | ||
| if (response.data.meta.hasPageSuivante) { |
There was a problem hiding this comment.
Double if can be factorized as : if (response?.data?.meta && response?.data?.meta?.hasPageSuivante) {
| } | ||
| } | ||
|
|
||
| return response.data.Mouvements; |
There was a problem hiding this comment.
You are fetching the WHOLE list of movements before returning them. This is to avoid in case of lot of data.
It's a better practice the process by batch to avoid a huge allocated memory.
|
|
||
| if (response?.data?.meta && response?.data?.meta?.hasPageSuivante) { | ||
| const mouvements = response.data.Mouvements; | ||
| const date = mouvements[mouvements.length - 1].dateValeur; |
There was a problem hiding this comment.
You don't have checked if you got "mouvements" in the body, if it is an array and if you got an element.
| @@ -12,62 +17,42 @@ var domain = "bank.local.fr" | |||
| * @return {Object} All transactions available on the page | |||
| */ | |||
| async function fetchTransactions(fromDate, authorization, jws = null, id, page, previousTransactions) { | |||
There was a problem hiding this comment.
Optional parameters must be specified at the end of the function definition.
If you don't have the JWS parameter you must put NULL yourself in this case
| } | ||
| const headers = jws ? { | ||
| ...config.defaultHeaders, | ||
| "jws": jws, |
There was a problem hiding this comment.
just { jws } can be used, the property name will have the same name as the variable.
| console.log(`--- Fetch Trasactions page n°${page} ---`); | ||
| try { | ||
| var headers = {"Authorisation": authorization } | ||
| console.log(`--- Fetch Trasactions page n°${page} ---`); |
There was a problem hiding this comment.
Trasactions --> Transactions
AurelienConte
changed the title
No description provided.