Igor w2 databases #12
Conversation
yunchen4
left a comment
yunchen4
left a comment
There was a problem hiding this comment.
Hi Igor,
Overall it looks good. There are some points that you need to rework, and I have made them explicit. I also left some suggestions to just remind possible blind spots.
Please let me know once you finish the rework, so I can review it again. Have a nice weekend!
Week2/databases/exercise 1 Keys.js
Outdated
| university VARCHAR(100), | ||
| date_of_birth DATE, | ||
| h_index INT, | ||
| gender VARCHAR(10) |
There was a problem hiding this comment.
Suggestion: You can consider gender as enums in most cases. Hence you can have some constraints about the possible value using CHECK. For enums, you can use a number (like SMALLINT). The disadvantage of this way is it is not very readable, but this is a common practice.
Week2/databases/exercise 1 Keys.js
Outdated
| await client.end(); | ||
| } | ||
|
|
||
| main().catch(console.error); |
There was a problem hiding this comment.
Needs rework: In case of error, the DB client is not closed and the connection remains open. In real life situation the error might not be solved immediately, so when next request comes, the same error will occur, and the application will open more connections without closing them. This will cause resource waste and finally, when the maximum number of connections are reached, the application will fail.
| await client.end(); | ||
| } | ||
|
|
||
| main().catch(console.error); |
There was a problem hiding this comment.
Needs rework: same issue here, in case of error, the client is not closed.
Week2/databases/exercise 3 Joins.js
Outdated
| await client.end(); | ||
| } | ||
|
|
||
| main().catch(console.error); |
There was a problem hiding this comment.
Needs rework: same issue here, in case of error, the client is not closed.
| const q2 = await client.query(` | ||
| SELECT SUM(cnt) AS female_paper_count FROM ( | ||
| SELECT COUNT(ap.paper_id) AS cnt | ||
| FROM authors a | ||
| JOIN author_papers ap ON a.author_id = ap.author_id | ||
| WHERE a.gender = 'Female' | ||
| GROUP BY a.author_id | ||
| ) sub; | ||
| `); |
There was a problem hiding this comment.
Suggestion: If a paper has two different female authors, this paper will be counted twice. Depending on the business requirements, this might be what we want. If you don't want to count the same paper twice, you need to change the query (use multiple JOIN instead of sub query).
| const q4 = await client.query(` | ||
| SELECT a.university, COUNT(ap.paper_id) AS paper_count | ||
| FROM authors a | ||
| LEFT JOIN author_papers ap ON a.author_id = ap.author_id | ||
| GROUP BY a.university; | ||
| `); |
There was a problem hiding this comment.
Suggestion: similar as the query for female authors, same paper might be counted several times if the paper has several authors from the same university. Depending on business requirements, this might be what we want.
| await client.end(); | ||
| } | ||
|
|
||
| main().catch(console.error); |
There was a problem hiding this comment.
Needs rework: same issue here, in case of error, the client is not closed.
2 participants
No description provided.