Oleksandr Starshynov week4 Databases #27
Conversation
RHSebregts
left a comment
RHSebregts
left a comment
There was a problem hiding this comment.
Hi Oleksandr,
Good effort, but I feel like it could use another round of polish. What I like to do, to make sure I stay fresh an don't forget things, is to make the PR, and then read through it again on the Github website. The different formatting makes me notice different things and refreshes my senses a bit.
Good luck!
Best wishes,
Robbert-Jan
| require("dotenv").config(); | ||
|
|
||
| async function main() { | ||
| const client = new MongoClient(process.env.MONGODB_URL); |
There was a problem hiding this comment.
this is a bit dangerous, what if the value doesn't exist?
There was a problem hiding this comment.
Good point, I have never though about this from suth corner. I don't usually do that way as here. Production will definitely have some kind of env file. Hoeever, I develop and test locally. That's why I often just put const url = process.env.MONGODB_URL || "mongodb://localhost:27017/mydb"
But that will only solve the local problem.
I'll try to write secure code so that it works not only locally.
There was a problem hiding this comment.
const url = process.env.MONGODB_URL || "mongodb://localhost:27017/mydb" would be fine!
What I like to do is to create a config file which exports a config object. That file will then handle all the checks on the env as well and has the defaults. That way in your code you could just require config and use config.dbConnectionString or something.
| await client.connect(); | ||
| const db = client.db("databaseWeek4"); | ||
| const collection = db.collection("aggregation"); | ||
| // taks 1. I have uploaded data via Atlas |
There was a problem hiding this comment.
Sorry, but I'd like to see what csv magic you performed to get it into the db.
There was a problem hiding this comment.
No magic here.
First I have found noi-line service to convert JSON-CSV. Than, since I am using Atlas, I have used its web-interface. That open needed Collection, and manually add file, click "Import" button. Finally, I just checked visually that all data is in correct structure.
| }, | ||
| ]); | ||
|
|
||
| console.log("\n📊 Task 2 Result:\n", await findPopulation.toArray()); |
There was a problem hiding this comment.
Please don't make a habit of using function calls in your console logs. And especially not asynchronous ones.
There was a problem hiding this comment.
Wow. I thought it was a good practice. If we need to do something just to output it to the console, then the code should be stored in that place and not in the middle of the rest of the working code. I'll change it.
| const result = await collection.aggregate([ | ||
| { | ||
| $match: { | ||
| Country: { $regex: /^[A-Z ]+$/ }, |
There was a problem hiding this comment.
I really like this regex based on the data structure, smart!
| const uri = process.env.MONGODB_URI; | ||
| const client = new MongoClient(uri); |
There was a problem hiding this comment.
Please make the database connection reusable here and in transfer.js.
| const { MongoClient } = require("mongodb"); | ||
| require("dotenv").config(); | ||
|
|
||
| const client = new MongoClient(process.env.MONGODB_URL); |
There was a problem hiding this comment.
This is going to fail, please tell me why!
There was a problem hiding this comment.
I was lucky. Since the condition exactly matched and the env was in the right place and contained the correct credentials - everything worked. I'll redo this place
| const client = new MongoClient(process.env.MONGODB_URL); | ||
|
|
||
| async function transferMoney(fromAcc, toAcc, amount, remark) { | ||
| const client = new MongoClient(process.env.MONGODB_URL); |
There was a problem hiding this comment.
Why are we doing this for the second time in this file?
| if (!fromDoc || !toDoc) throw new Error("Account not found."); | ||
| if (fromDoc.balance < amount) throw new Error("Insufficient funds."); |
There was a problem hiding this comment.
I like the grouping of the database calls in principle, but if we move these checks up we could save ourselves an unnecessary call.
| const lastFromChange = fromDoc.account_changes.at(-1)?.change_number || 0; | ||
| const lastToChange = toDoc.account_changes.at(-1)?.change_number || 0; |
| const newFromBalance = fromDoc.balance - amount; | ||
| const newToBalance = toDoc.balance + amount; |
There was a problem hiding this comment.
I like that you do this here, and I would love to see you also create the object you give to $push here. That way we can see what you're planning to do, and in the db query itself we can just focus on the db language, and not on what you are putting in it.
2 participants
No description provided.