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

Fady w4 database #34

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

Conversation

Fadysaadeddin
Copy link

No description provided.

@Fadysaadeddin
Copy link
Author

sorry i have the same problem that has other commits with this branch .. acually i use the same dir for all branches .. every week a create a new branch for the week name then i start working with the assignment .. but when come to push i select only the files that i changed for the week .. then i found pull request with other commits from the previous weeks . i dont know why .. now i saw that when comparing the pull request i would try to fix it but i didt have enough time .. i ll try to search for this problem and fix it .. thanks

@dyaboykyl
Copy link

Hey Fady no worries. Let's see if I can help

acually i use the same dir for all branches

This is totally ok 👍

every week a create a new branch for ### ### the week name then i start working with the assignmen

This is the right approach! 👍

when come to push i select only the files that i changed for the week

Also right, you should only include files that changed in your commit

then i found pull request with other commits from the previous weeks

What's probably happening is that when you create a new branch for the next week, you are still on the branch for the previous week. That means the commit from the previous week will be carried over into the new branch. However, when you post the pull request, github will compare it with the main branch (the original branch that you forked the repository from). So it will show any commits that are different from the main branch, even if they were already included in other pull requests.

In order to prevent that in the future you can switch back to main before you create a new branch:

git checkout main
git checkout -b fady-w4-database

Then you will only have commits from week4 in that branch.

In order to fix it now you can delete the commits from previous weeks (don't worry, they'll still be saved in your other branches). This is a little complicated because you need to edit the git commit history. An easier option is to just delete all the files not related to Week4 in a new commit on this branch. Then they should not show up in the pull request

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 looks really good, nice job using an appropriate amount of whitespace to create clean code!

Just left a few small comments

Comment on lines +8 to +10
const jsonArray = await csv().fromFile(csvFilePath);
fs.writeFileSync(jsonFilePath, JSON.stringify(jsonArray, null, 2));
console.log(`JSON file saved as ${jsonFilePath}`);

Choose a reason for hiding this comment

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

I'm not sure I fully understand the purpose of this code. You convert the data into a json file, but where is that file used? How does the data get into you mongodb database? Please add that code

Copy link
Author

Choose a reason for hiding this comment

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

yes .. when i searched i found many ways to convert csv into json ... but i found this way is the easiest .. i always follow the easiest and understandable.. ok i ll add it

Comment on lines +9 to +14
{
$addFields: {
M: { $toDouble: '$M' },
F: { $toDouble: '$F' },
},
},

Choose a reason for hiding this comment

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

When you import the data into mongodb you can import it as a number instead of a string. That way you do not need to convert it to a number when you query (thus you can remove this section)

Copy link
Author

Choose a reason for hiding this comment

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

that is the sturcture of data in my json file
{
"Country": "Afghanistan",
"Year": "1950",
"Age": "25-29",
"M": "321311",
"F": "272299"
},
i think i have two ways either keep the file as it is and convert the string into number the way i did .. or i change the whole json file into number then i can remove this code .. is that right ..

},
},
{
$match: {

Choose a reason for hiding this comment

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

You already have a match clause on line 9. You can put the country match in there as well so that all of the match information is one place

Copy link
Author

Choose a reason for hiding this comment

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

ok .. done

Comment on lines 41 to 49
$group: {
_id: '$Country',
TotalPopulation: { $sum: '$TotalPopulation' },
M: { $sum: '$M' },
F: { $sum: '$F' },
Year: { $first: '$Year' },
Age: { $first: '$Age' },
},
},

Choose a reason for hiding this comment

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

The data is a bit confusing because the continents are labeled as "countries", but because of that you don't actually need to group the data. If you remove this section, your query should still work

Copy link
Author

Choose a reason for hiding this comment

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

it took hours with me to get to this code .. kkk .. ok i try it

Comment on lines 51 to 59
$project: {
_id: 0,
Country: '$_id',
TotalPopulation: 1,
M: 1,
F: 1,
Year: 1,
Age: 1,
},

Choose a reason for hiding this comment

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

Since yo want all the fields of the document you can omit the $project altogether. All fields should be included by default

Comment on lines 33 to 34
'NORTH AMERICA',
'SOUTH AMERICA',
Copy link

@dyaboykyl dyaboykyl Jan 25, 2025

Choose a reason for hiding this comment

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

If you inserted the data without modifying then you should notice that these two continents don't show up in the output. Can you figure out why and fix these lines of code to make (at least one of them) show up?

};

const newRecipientChange = {
change_number: recipient.account_changes.length + 1,
Copy link

@dyaboykyl dyaboykyl Jan 25, 2025

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. This is especially possible because you are not reading the document inside the transaction

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

@dyaboykyl dyaboykyl self-assigned this Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants