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

Rubamansour week4 databases #31

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

Conversation

RubaMansour
Copy link

No description provided.

@RubaMansour RubaMansour force-pushed the Rubamansour-week4-DATABASES branch from 5587909 to 6cb6fb9 Compare January 22, 2025 13:17
git commit -mdoen“
@RubaMansour RubaMansour force-pushed the Rubamansour-week4-DATABASES branch from 6cb6fb9 to 4a2e80f Compare January 22, 2025 16:27
@yunchen4 yunchen4 self-assigned this Jan 24, 2025
Copy link

@yunchen4 yunchen4 left a comment

Choose a reason for hiding this comment

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

Hi Ruba,
Your assignment looks good to me. There's only one comment that you need to rework.
Please let me know once you finish your rework.
And also pay attention to the changes in your commit. You only need to submit the assignment for the week. Try not to commit irrelevant files and changes.
If you have any questions please let me know. Have a nice weekend!

Comment on lines 17 to 39
const pipeline = [
{ $match: { Year: year, Age: ageGroup } }, {
$group: {
_id: "$Country",
Year: { $first: "$Year" },
Age: { $first: "$Age" },
M: { $sum: "$M" },
F: { $sum: "$F" }
}
},

{
$project: {
_id: 1,
Country: "$_id",
Year: 1,
Age: 1,
M: 1,
F: 1,
TotalPopulation: { $add: ["$M", "$F"] } }
},
{ $sort: { Country: 1 } }
];

Choose a reason for hiding this comment

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

Need to work: the question asks about the total population of each continent. If you take a look at data you may notice in column Country there are AFRICA, ASIA etc separating from the real countries. Try to use those continent name values in the country column.

And could you please tell me why you use group here?

Copy link

@yunchen4 yunchen4 left a comment

Choose a reason for hiding this comment

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

Just two small comments. LGTM!

Comment on lines +42 to +44
} catch (err) {
console.error('Error:', err);
return [];

Choose a reason for hiding this comment

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

Depending on the situation you may decide if you want to return empty result if there's an error. Since this is a business-required functionalities, usually you don't want to return empty result. Instead you want to throw it until some error handlers handle it.


{
$addFields: {
TotalPopulation: { $add: ['$M', '$F'] } // إضافة مجموع السكان

Choose a reason for hiding this comment

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

suggestion: unless you're working for a company with only people knowing Arabic, it's better to write comments in English :)

Comment on lines 34 to 48
await accountsCollection.updateOne(
{ account_number: fromAccount },
{
$set: { balance: updatedFromBalance },
$push: {
account_changes: {
change_number: fromChangeNumber,
amount: -amount,
changed_date: new Date(),
remark
}
}
},

);

Choose a reason for hiding this comment

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

Need to work: you need to add session if you want to include this operation in the transaction. Same as what you did for findOne above.

Comment on lines 50 to 64
await accountsCollection.updateOne(
{ account_number: toAccount },
{
$set: { balance: updatedToBalance },
$push: {
account_changes: {
change_number: toChangeNumber,
amount: amount,
changed_date: new Date(),
remark
}
}
},

);

Choose a reason for hiding this comment

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

Same here you need to add session.

Copy link

@yunchen4 yunchen4 left a comment

Choose a reason for hiding this comment

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

LGTM!

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