Team-Half-and-Half / spirebooks

MIT License
0 stars 0 forks source link

Review 7: AuditedBalanceCollection.js and DataInputBridge.js #139

Closed beydlern closed 2 weeks ago

beydlern commented 2 weeks ago

Overview

Please pay attention too:

Review Branch

review-7

Files to review

Checklists

Due date

Monday, 10/28/2024

For more information

The review process is documented at: http://courses.ics.hawaii.edu/ics414s21/morea/review/reading-idpm-review.html

beydlern commented 2 weeks ago

AuditedBalanceCollection.js

Multi-line comments possess two asterisks at the start, one is enough.

DataInputBridge.js

Minor comments explaining some of the less obvious data parameters would bolster clarity.

caspascual commented 2 weeks ago

AuditedBalanceCollection.js

Can’t find any issues, nice job on the comments, very detailed.

DataInputBridge.js

Design-09/Arch-02: Should this instead be in utilities?

t-tirabassi commented 2 weeks ago

AuditedBalanceCollection.js:

Design-02: The publish method on line 75 could be split into two separate methods to avoid the nested conditionals, one for users and one for admins.

DataInputBridge.js:

JS-08: JSDoc comments could be used to clarify what each schema does.

jsapolu99 commented 2 weeks ago

AuditedBalanceCollection.js Design-02: We can separate the publish method into two methods: publishUserBalance and publishAdminBalance, removing the nested conditional structure. Each function will handle a specific publication type, making the code clearer and easier to maintain.

DataInputBridge.js JS-08: Add JSDoc comments to explain each schema, as the purpose of each schema (such as CashAndCashEquivalents, OtherAssets, etc.) might not be immediately clear without context. Adding brief descriptions of each schema's intent and main fields will make the code more understandable to others.

XavierBurt commented 2 weeks ago

AuditedBalanceCollection

The comments on lines 79 and 88 are java doc comments, it should be /* ... instead of /** ... Or even better since it's just a one line comment, //

DataInputBridge

Lines 215 and 216 can be turned into 1 "line", should be able to use escape characters to conform to the line length requirements I think.

samallari commented 2 weeks ago

AuditedBalanceCollection.js

DataInputBridge.js

PaytonHAH commented 2 weeks ago

AuditedBalanceCollection.js

DataInputBridge.js

bksnelson commented 2 weeks ago

AuditedBalanceCollection.js Publication method could be split based on role. More roles will be added, making the method larger than needed.
But would require changing base collection.

DataInputBridge.js React-08: File name should indicate which spreadsheet it is for. Or a comment to indicate the spreadsheet that is being worked on. File could also be put into utilities or separate folder from components. // Revenues (General Revenues) not putting this in its own sub schema might not work with the update/define methods in FScollection.