Hanna melnyk w2 databases #567
Conversation
| /*Connect to SQL server*/ | ||
| connection.connect(err => { | ||
| if (err) { | ||
| return console.error('Connection error: ' + err.stack); |
There was a problem hiding this comment.
Would be nice to know what the error message says too rather than the stack trace
|
|
||
| /*SQL queries*/ | ||
| const createDatabaseAndTables = | ||
| `DROP DATABASE IF EXISTS meetup; |
There was a problem hiding this comment.
Proposing to move meetup to a const. Like const DB_NAME = 'meetup' and use this in the template string. This way you can avoid typos.
| ); | ||
|
|
||
| CREATE TABLE Room ( | ||
| room_no INT AUTO_INCREMENT PRIMARY KEY, |
There was a problem hiding this comment.
Small pet peeve, indentations are a bit off. Your indentations on Meeting are different than the rest
|
|
||
|
|
||
| // Execute the queries | ||
| connection.query(createDatabaseAndTables, (error, results, fields) => { |
There was a problem hiding this comment.
| connection.query(createDatabaseAndTables, (error, results, fields) => { | |
| connection.query(createDatabaseAndTables, (error, _, __) => { |
Since you are not using the two params
| /*Connect to SQL server*/ | ||
| connection.connect(err => { | ||
| if (err) { | ||
| return console.error('Connection error: ' + err.stack); |
There was a problem hiding this comment.
Same comment as this: https://github.com/HackYourFuture/databases/pull/567/files#r1736012412
| queries.forEach((query, index) => { | ||
| connection.query(query, (error, results) => { | ||
| if (error) throw error; | ||
| console.log(`Query ${index + 1}:`); |
There was a problem hiding this comment.
| console.log(`Query ${index + 1}:`); | |
| console.log(`Query ${index + 1}: ${queries[index]}`); | |
| //C:\Users\knowl\Documents\hyf\databases\Week2\connection_query.js | ||
| import mysql from 'mysql'; | ||
|
|
||
| export const createNewConnection = () => { |
There was a problem hiding this comment.
I like how you are thinking here, but please keep in mind everytime you call this function gives an opportunity to create a new connection. Meaning you can easily overwhelm the MySQL server in a real use case scenario. You can allow only one object of connection to be passed around, this is called Singleton pattern or a fixed number of connection objects to be passed around, this is called Connection pooling.
But for the purposes of this exercise its okay 😄
| console.log('Mentor column already exists. No changes made.'); | ||
| resolve(); | ||
| } else { | ||
| const addMentorColumnQuery = ` |
There was a problem hiding this comment.
Was adding foreign key column conditionally part of the assignment? Because you could have created the foreign key column while creating the table, removing all the code related to conditions. Could've been simpler
| return new Promise((resolve, reject) => { | ||
| const query = ` | ||
| SELECT | ||
| SUM(IF(authors.gender = 'Female', 1, 0)) AS female_author_paper_count |
There was a problem hiding this comment.
Would prefer if you just used a WHERE clause
| export const useDatabase = (connection) => { | ||
| return new Promise((resolve, reject) => { | ||
| const createDatabaseAndUse = ` | ||
| CREATE DATABASE IF NOT EXISTS transactions; |
There was a problem hiding this comment.
I would not create a database named transactions. It is very close to the term transaction built into mysql. Can cause confusion in real life scenario. Maybe a database named dinner_club_app is more aptly named.
|
|
||
| const accountChangesTable = { | ||
| change_number: 'INT AUTO_INCREMENT PRIMARY KEY', | ||
| account_number: 'INT NOT NULL', |
There was a problem hiding this comment.
I would rename this to from_account_number and add a column named to_account_number
|
|
||
| const connection = createNewConnection(); | ||
|
|
||
| const transferAmount = (fromAccount, toAccount, amount) => { |
There was a problem hiding this comment.
Few things to consider:
- Don't you want to check if the
fromAccountactually contains the money available for the transfer? - What if they account does not even exist? Do you not want to just rollback then instead of storing it in
account_changes?
Bonus:
- You can wrap query functions that you use here to use promises, that way you can use
async-awaitsand simplifies the code a lot.
No description provided.