-
Notifications
You must be signed in to change notification settings - Fork 10
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
Week 2 database Elias #26
base: main
Are you sure you want to change the base?
Conversation
…nstead of nested callbacks. This makes the code easier to read and maintain. I added a foreign key constraint for the invited_by field in the Invitee table. This ensures that each person invited actually exists in the table, improving data integrity. I updated how we manage the database connection to use promises, which makes it easier to handle errors that may occur. I also improved the comments in the code to make it clearer how we set up the database and create the tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Elias,
There are some comments which need rework. The main point is about the relationship between papers and authors. Thus relevant queries in exercise 3 and 4 need rework too.
Feel free to contact me on slack if you have any questions. Keep fighting! 💪
paper_title VARCHAR(255) NOT NULL, | ||
conference VARCHAR(255), | ||
publish_date DATE, | ||
author_id INT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs work: one paper can have several authors. Single author_id
column in paper table can only refer to one author. Try to find another way to reflect the relationship between authors and papers.
Week2/assignment/joins.js
Outdated
const queryAuthorsAndPapers = ` | ||
SELECT a.*, rp.paper_title | ||
FROM authors a | ||
LEFT JOIN research_papers rp ON a.author_id = rp.author_id;`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs work: as mentioned in another comment, the table structure needs to be changed to reflect the correct relationship between authors and papers. Hence the relevant queries need rework too.
Week2/assignment/aggregate.js
Outdated
SELECT rp.paper_title, COUNT(a.author_id) AS number_of_authors | ||
FROM research_papers rp | ||
LEFT JOIN authors a ON rp.author_id = a.author_id | ||
GROUP BY rp.paper_id;`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs work: as mentioned in another comment, the table structure needs to be changed to reflect the correct relationship between authors and papers. Hence the relevant queries need rework too.
Week2/assignment/aggregate.js
Outdated
const queryFemalePapersCount = ` | ||
SELECT COUNT(*) AS total_female_papers | ||
FROM research_papers rp | ||
JOIN authors a ON rp.author_id = a.author_id | ||
WHERE a.gender = 'Female';`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs work: as mentioned in another comment, the table structure needs to be changed to reflect the correct relationship between authors and papers. Hence the relevant queries need rework too.
Week2/assignment/aggregate.js
Outdated
// Query for sum of research papers of authors per university | ||
const queryTotalPapersPerUniversity = ` | ||
SELECT a.university, COUNT(rp.paper_id) AS total_papers | ||
FROM authors a | ||
LEFT JOIN research_papers rp ON a.author_id = rp.author_id | ||
GROUP BY a.university;`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs work: as mentioned in another comment, the table structure needs to be changed to reflect the correct relationship between authors and papers. Hence the relevant queries need rework too.
Week2/assignment/aggregate.js
Outdated
connection.query(queryMinMaxHIndex, (error, results) => { | ||
if (error) throw error; | ||
console.log('Min and Max H-index per university:', results); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: these lines are repeated several times. Only the query and the log message are different. You can extract these lines into a separate function which accepts log message and query as parameters. Always keep DRY principle (don't repeat yourself) in mind when you are coding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
No description provided.