Yaroslav kazeev w2 databases #15
Conversation
yunchen4
left a comment
yunchen4
left a comment
There was a problem hiding this comment.
Hi Yaroslav,
Overall it is a good assignment. I can see you try to learn more advanced concepts for database. That's good.
There are several points that you need to rework, mainly about the relationship between paper and authors. I also don't see the answers for exercise 4.
Some comments are suggestions. You can choose to apply them or not.
Please let me know once you finish the rework, so I can review it again. Have a nice weekend!
| }, | ||
| ]; | ||
|
|
||
| module.exports = { authors, researchPapers }; |
There was a problem hiding this comment.
Suggestion: It is good that you separate code and data. Since this JS file is purely for data, you can convert it to JSON files (one for authors, one for research paper) and read these JSON files in the code.
There was a problem hiding this comment.
The proposed change does not bring any improvements in terms of code clarity and the application's overall volume:
import { readFile } from "fs/promises";
const authors = JSON.parse(
await readFile("./Week2/assignment/authors.json", "utf-8")
);
const researchPapers = JSON.parse(
await readFile("./Week2/assignment/researchPapers.json", "utf-8")
);
You suggest using the code above with the use of the additional package (fs) instead of just this:
import { authors, researchPapers } from "./generateSampleData.js";
Unless there is an unwritten law among developers or a convention regarding data storage and handling, I cannot see why I should use your approach.
There was a problem hiding this comment.
Simply said, it is a convention to separate code and pure data.
Using JSON for data can show clearly this is a file about pure data, nothing else. It can help with future maintainability. If I see the JS file I might think: "Well, is there any additional logic in order to generate the data?" So I need to open the file to check. When I see the JSON file I don't need to think about it, it is just data, I can use it anywhere.
JSON also has better compatibility because it can also be used across different platforms and coding languages, but JS doesn't (you need to have JS environment to get data).
In this assignment since it is just for learning purpose, using JS for data is fine, but it is not recommended in real working situations.
Week2/assignment/Ex2Relationships.js
Outdated
| const ALTER_RESEARCH_PAPERS_TABLE = ` | ||
| ALTER TABLE research_Papers | ||
| ADD COLUMN IF NOT EXISTS author_id SMALLINT, | ||
| ADD CONSTRAINT fk_author_id | ||
| FOREIGN KEY (author_id) REFERENCES authors(author_id) ON DELETE CASCADE | ||
| `; |
There was a problem hiding this comment.
Needs rework: One paper can have several authors. If research_papers table has one column for author and has FK constraints, we cannot use it for several authors.
And also, it is better not to mix the camel case and snake case: research_papers or ResearchPaper.
| const CREATE_AUTHORS_PAPERS_VIEW = ` | ||
| CREATE OR REPLACE VIEW authors_papers AS | ||
| SELECT | ||
| a.author_name, | ||
| 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.
Needs rework: since the relationship between author and paper needs to be changed, you need to rework on this query as well.
| // Execute the seeding function | ||
| seedDatabase(); |
There was a problem hiding this comment.
Needs rework: I didn't see the answers for exercise 4?
There was a problem hiding this comment.
Added them in this version
| const CREATE_GENDER_TYPE = ` | ||
| CREATE TYPE GENDER AS ENUM ('Male', 'Female', 'X'); | ||
| `; | ||
|
|
yunchen4
left a comment
yunchen4
left a comment
There was a problem hiding this comment.
LGTM. I have replied to your comment about why using JSON for data.
2 participants
No description provided.