-
Notifications
You must be signed in to change notification settings - Fork 10
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
Sofiia-week4-databases #46
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sofiia,
Nice work, I will approve it.
I left some suggestions for real life working situation. If you have questions don't hesitate to ping me on slack.
Have a nice weekend! 😊
module.exports = { | ||
getTotalPopulationByCountryAndYear, | ||
getPopulationByContinentYearAndAge, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very nice that you use modules. It really helps with the cleaner and readable code!
$inc: { balance: -amount }, | ||
$push: { | ||
account_changes: { | ||
change_number: fromAccount.account_changes.length + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: using fromAccount.account_changes.length + 1
to know the next change_number
is OK.
However, in real life one account may have a lot more change records. We don't really care what those changes are, we just want to know the maximum change number. Fetching all changes with the account can be expensive (since you need to transfer more bytes in the network. When you download something big from Internet, it is always slower than dowloading smaller files), and we try to only fetch the columns we need.
One solution is to have a function to get only the maximum change number (you have learned MAX
sql syntax), and when you fetch the account data you don't fetch the changes column.
change_number: toAccount.account_changes.length + 1, | ||
amount: amount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same suggestion for max change number applies here too.
if (!fromAccount || !toAccount) | ||
throw new Error("One or both accounts not found."); | ||
if (fromAccount.balance < amount) throw new Error("Insufficient funds."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early returns or throwing are always good things to do!
Just notice you didn't close the client; remember to close the client after your code finish running! |
No description provided.