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

assignment week4 Fatemeh Alinejad #51

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

Conversation

Fatialnd
Copy link

@Fatialnd Fatialnd commented Nov 6, 2024

No description provided.

@Fatialnd Fatialnd changed the title assignment week4 assignment week4 Fatemeh Alinejad Nov 7, 2024
@yunchen4 yunchen4 self-assigned this Nov 8, 2024
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 Fatemeh,
Good job! I will approve it.
I left some comments related to real working situation. If you have any questions don't hesitate to contact me on slack.
Have a nice weekend!

Comment on lines +5 to +6
const db = client.db("databaseWeek4");
const collection = db.collection("ex1-aggregation");
Copy link

Choose a reason for hiding this comment

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

suggestion: since these two strings are used across the files, it's better to make them constants.

Comment on lines +5 to +6
const db = client.db("databaseWeek4");
const collection = db.collection("ex1-aggregation");
Copy link

Choose a reason for hiding this comment

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

Same constant suggestion applies here too.

Comment on lines +5 to +6
const db = client.db("databaseWeek4");
const collection = db.collection("accounts");
Copy link

Choose a reason for hiding this comment

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

Same constant suggestion applies here too.

Comment on lines +15 to +17
if (from.balance < amount) {
throw new Error("Insufficient balance");
}
Copy link

Choose a reason for hiding this comment

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

Early returns and throwing are always good if applicable :)

Comment on lines +19 to +20
const changeNumberFrom = from.account_changes.length + 1;
const changeNumberTo = to.account_changes.length + 1;
Copy link

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 using the same network), 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.

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