Yaroslav kazeev w3 databases #16
Conversation
yunchen4
left a comment
yunchen4
left a comment
There was a problem hiding this comment.
Hi Yaroslav,
LGTM. I left some suggestions for better code quality and readability. If you have any questions, please let me know.
| export const accounts = [ | ||
| { account_number: 101, balance: 5000 }, | ||
| { account_number: 102, balance: 12000 }, | ||
| { account_number: 103, balance: 7500 }, | ||
| { account_number: 104, balance: 3000 }, | ||
| { account_number: 105, balance: 9500 }, | ||
| ]; |
There was a problem hiding this comment.
Suggestion: Same suggestion as week 2, better to use JSON file for pure data. You don't have to change your code but it is better to know.
| async function createTables(client) { | ||
| const CREATE_ACCOUNT_TABLE = ` | ||
| CREATE TABLE IF NOT EXISTS account ( | ||
| account_number SMALLINT PRIMARY KEY, |
There was a problem hiding this comment.
If the bank is really small using SMALLINT as PK makes sense. It is not common to use a small type for PK.
| } catch (error) { | ||
| console.error("Error seeding database:", error); |
There was a problem hiding this comment.
Suggestion: although client.end() will also roll back the changes in case of errors, I will recommend to add await client.query("ROLLBACK") in the catch block to clearly state a rollback operation will be done. It has several advantages:
- When I read the code, I know in case of errors the changes will be rolled back instead of guessing whether rollback will be done.
client.end()will roll back changes in PostgreSQL, but not necessarily in other types of databases or in other versions. We shouldn't rely on this kind of implicit operations.
There was a problem hiding this comment.
I would like to save a couple of code lines and rely on the behavior of the Postgres database management system, which should be known to the developer's team. On top of that, I do not expect it to be changed to let's say, MySQL, which will require the rewrite of many functions, where a transaction block is just one of them.
Although I understand that you have more experience, your argument is not convincing for now. This approach creates safeguards twice: in the RDBMS and in the app's code.
| const bobRossCollection = await client | ||
| .db("databaseWeek3") | ||
| .collection("bob_ross_episodes"); |
There was a problem hiding this comment.
Suggestion: these lines for client initialization appears several times in the file. You can extract it as a function which returns the client. And if a string repeats several times in a file (in case you don't extract it as a function), it is better to have a constant for it, so you can avoid typos.
| const donator_account_number = 101; | ||
| const receiver_account_number = 102; |
There was a problem hiding this comment.
Suggestion: in real life situation it is better to check whether the account exists and the balance is enough before doing actual transactions.
There was a problem hiding this comment.
I disagree once again here. Limitations should be enforced in the DB, for example, allowing the balance only a positive integer. Then, we can rely on the RDBMS to throw an error if some of the transactional statements can not be executed and abort the whole transaction. No check inside the code is needed.
2 participants
No description provided.