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.

Sign up for GitHub

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

[REVIEW] of fileToReview.js #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[REVIEW] of fileToReview.js #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 39 additions & 54 deletions review/fileToReview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@

var domain = "bank.local.fr"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const config = {
domain: "bank.local.fr",
defaultHeaders: {
"Content-type": "application/json",
"Accept": "application/json",
}
}

/**
* @description Fetch transactions recursively
Expand All @@ -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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

console.log(`--- Fetch Trasactions page n°${page} ---`);
try {
var headers = {"Authorisation": authorization }
console.log(`--- Fetch Trasactions page n°${page} ---`);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trasactions --> Transactions


if (jws) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

headers = {
"Authorisation": authorization,
"jws": jws,
"Content-type": "application/json",
"Accept": "application/json"
}
} else {
headers = {
"Authorisation": authorization,
"Content-type": "application/json",
"Accept": "application/json",
}
}
const headers = jws ? {
...config.defaultHeaders,
"jws": jws,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just { jws } can be used, the property name will have the same name as the variable.

"Authorisation": authorization
} : {...config.defaultHeaders, "Authorisation": authorization};

const {code, response} = await doRequest('GET', `${config.domain}/accounts/${id}/transactions?page=${page}`, headers);

var {code, response } = await doRequest('GET',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The url is not clear at all, and can be factorized as : ${config.domain}/accounts/${id}/transactions?page=${page}

domain + '/accounts/'+ id + '/transactions?' + `page=${page}`,
headers);
if (code < 200 || code >= 400 || !response?.data) {
console.error(`Bad request received from bankin API : ${code}`, response);
return [];
}

if (response?.data?.meta && response?.data?.meta?.hasPageSuivante) {
const mouvements = response.data.Mouvements;
const date = mouvements[mouvements.length - 1].dateValeur;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have checked if you got "mouvements" in the body, if it is an array and if you got an element.


if (date <= fromDate) {
console.log("FromDate is Reached - we don't need more transaction");
} else {

if (!mouvements)
throw new Error("Empty list of transactions ! " + JSON.stringify(previousTransactions));

if (response && code == 200 && response.data) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.data.meta) {
if (response.data.meta.hasPageSuivante) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double if can be factorized as : if (response?.data?.meta && response?.data?.meta?.hasPageSuivante) {

let mouvements = response.data.Mouvements;
var date = mouvements[mouvements.length -1].dateValeur;
if (date <= fromDate) {
console.log("FromDate is Reached - we don't need more transaction");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of console log and they also create a lot of extra IF / ELSE in your code. Are they all needed ?
In case of debug i think this is useful but in case of production they are not and they are reducing the clarity of the code BUT also the clarity of the debugger.

} else {
// if we have mouvements
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment must be useful and give context to the process.
If we read the line bellow we can see the context (well done the variable have a good name :) )

if (mouvements) {
if (assertTransactions(mouvements)) {
return [];
} else {
console.log(`Push transactions from page ${page}`);
}
} else {
throw new Error("Empty list of transactions ! " + JSON.stringify(previousTransactions));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why throwing everywhere and catch at the end ?
For a better control flow, you can organize error with a logger and change some of them as "warning" instead.

And also manage the return value at this moment.

}
let nextPagesTransactions = fetchTransactions(fromDate, authorization, (jws || null), id, page + 1, mouvements);
response.data.Mouvements = mouvements.concat(nextPagesTransactions);
}
}
if (assertTransactions(mouvements)) {
return [];
} else {
console.log(`Push transactions from page ${page}`);
}
return response.data.Mouvements;
} else throw new Error();

return [];
} catch (err) {
throw new CustomError({
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without saying again that is a bad idea you are throwing a custom error who have always the same statusCode.
And, the statusCode is not explicit.

function: 'fetchTransactions',
statusCode: 'CRASH',
rawError: e,
});
}
const nextPagesTransactions = fetchTransactions(fromDate, authorization, (jws || null), id, page + 1, mouvements);
response.data.Mouvements = mouvements.concat(nextPagesTransactions);
}
}

return response.data.Mouvements;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}