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

aziz-w4-databases #45

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

azizsigar
Copy link

…ation

Copy link

@dyaboykyl dyaboykyl left a comment

Choose a reason for hiding this comment

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

Overall the code is really clean and well-written, nicely done! Just a couple of comments to take a look at

Comment on lines 30 to 34
$lookup: {
from: "continents",
localField: "_id",
foreignField: "Country",
as: "continent_info",

Choose a reason for hiding this comment

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

This is an cool way to get the continent info for each country, but it's more SQL-style as it would imply there's a continent collection that relations country and continent info.

Since the original data doesn't have a continents collection, could you find a way to get the population by contintent with only the original data? (Hint: Some of the documents have continents instead of countries for the Country field, e.g. "AFRICA", "OCEANIA". You need to find a way to filter for them)

Comment on lines +22 to +27
const fromAccountDoc = await accountsCollection.findOne({
account_number: fromAccount,
});
const toAccountDoc = await accountsCollection.findOne({
account_number: toAccount,
});

Choose a reason for hiding this comment

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

It's a good idea to include the session object inside the findOne documents. If they got modified outside the transaction mongodb will retry the transaction

$inc: { balance: -amount },
$push: {
account_changes: {
change_number: Date.now(),

Choose a reason for hiding this comment

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

Using the current date and time is an interesting approach, but it doesn't accomplish the request in the instructions: "The change number should be incremented, so if the latest change_number is 30, the change_number for the new change should be 31."

How might you go about using an incrementing number here instead of a date?

@dyaboykyl dyaboykyl self-assigned this Nov 9, 2024
Copy link

@dyaboykyl dyaboykyl left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for responding to the comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants