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

ALEX P Week4 Databases #28

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

Conversation

op117
Copy link

@op117 op117 commented Jan 18, 2025

Week 4 assignment is complete. Please check it.

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.

The code looks great! Just left a few comments

const csv = require("csv-parser");
require("dotenv").config();

async function loadCSVToMongoDB() {

Choose a reason for hiding this comment

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

Overall looks great! One thing I would recommend is to add some logic around making sure that you don't re-add the data if you run the script again. For example, you can check if there are any documents in the collection and if so, return early


const result = await collection.aggregate([
{ $match: { Country: country } },
{ $group: { _id: "$Year", countPopulation: { $sum: { $add: ["$M", "$F"] } } } },

Choose a reason for hiding this comment

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

minor: The $sum is unnecessary

const collection = db.collection("population");

const result = await collection.aggregate([
{ $match: { Year: year, Age: ageGroup } },

Choose a reason for hiding this comment

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

This will return all countries and their total populations, however the question only asks for populations per continent. You'll need the examine the data to see how the continent information is stored (hint: it's not intuitive). The output given in the assignment details may help you

TotalPopulation: { $add: ["$M", "$F"] }
}
},
{ $group: { _id: "$Country", data: { $push: "$$ROOT" } } },

Choose a reason for hiding this comment

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

I'm not sure what you get but when I run your code it logs [Object] for the results and I suspect that's because of the data: { $push: "$$ROOT" } .

This grouping will actually have no effect on the data because it's already matched on a specific year and age, so you can remove this clause completely

const fs = require("fs");
require("dotenv").config();

async function importAccounts() {

Choose a reason for hiding this comment

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

Do you need this file? It looks like it's accomplishing the same purpose as setup.js

throw new Error("Insufficient funds in the source account!");
}

const fromChangeNumber = fromAccountDoc.account_changes.length + 1;

Choose a reason for hiding this comment

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

Using the number of items in the account_changes array as a change number may work, but will likely lead to the following problems:

  1. An account with thousands of changes will be very inefficient to read every time you need to calculate the next change number
  2. If for some reason a change was removed from the array (decreasing the length), you may end up with duplicate change numbers
  3. Multiple concurrent transfers may cause a race condition that results in duplicate change numbers because both transfers see the same number of changes

Can you think of a more robust way to calculate change numbers that handles all of these problems?

remark
}
}
}

Choose a reason for hiding this comment

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

An essential part of mongodb transactions is that the transaction session must be included in all of the operations like this:

await accounts.findOneAndUpdate(
    { account_number: fromAccount },
                {
                    $inc: { balance: -amount },
                    $push: {
                        account_changes: {
                            change_number: fromChangeNumber,
                            amount: -amount,
                            changed_date: new Date(),
                            remark
                        }
                    }
                },
               { session } // must be present
  );

If you don't include the session then mongodb will not reliably be able to handle rollbacks and race conditions

@dyaboykyl dyaboykyl self-assigned this Jan 25, 2025
@op117
Copy link
Author

op117 commented Jan 25, 2025

Hi @dyaboykyl,
thanks for such a detailed review. I've already fixed it.

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.

Great job!

Comment on lines +31 to +38
$group: {
_id: "$Country",
Year: { $first: "$Year" },
Age: { $first: "$Age" },
TotalPopulation: { $sum: "$TotalPopulation" },
M: { $sum: "$M" },
F: { $sum: "$F" },
},

Choose a reason for hiding this comment

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

This grouping will have no effect because there is only 1 document per year,age, and country filter. You can remove it altogether and you will still get the same results

$match: {
Year: year,
Age: ageGroup,
Country: { $regex: /^[A-Z\s]+$/ },

Choose a reason for hiding this comment

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

Nice solution! Technically it also includes "WORLD" which is not one of the continents in the desired output, but I think that's ok

await accountsCollection.updateOne(
{ account_number: fromAccount },
{
$inc: { balance: -amount, change_counter: 1 },

Choose a reason for hiding this comment

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

Using a separate field for the change counter is a great approach, nicely done. If you wanted to get really fancy and let mongodb do all the work, you could use an $set aggregation operator to reference the existing change counter in the new account_changes array element. That way you would not need to calculate the change number (lines 36 and 37)

Something like this:

await accounts.findOneAndUpdate(
    { account_number: account },
    [
      {
        $set: {
          balance: { $add: ["$balance", amount] },
          next_change_number: { $add: ["$next_change_number", 1] },
          account_changes: {
            $concatArrays: [
              "$account_changes", [{
                change_number: "$next_change_number",
                amount,
                changed_date: new Date(),
                remark,
              }]
            ]
          }
        },
      }
    ],
    { session }
  );

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