manoa-inspire / MATP

MIT License
0 stars 0 forks source link

Review 6: auditedBalanceCollection.js and AuditedBalanceSchemaInput.jsx #108

Open blakewatanabe opened 1 week ago

blakewatanabe commented 1 week ago

Overview

The focus for this code review will be centered around the auditedBalanceCollection.js and AuditedBalanceSchemaInput.jsx.

Please pay attention too:

Review Branch

review-6

Files to review

Checklists

Due date

10/21/24

For more information

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

luisy-eng commented 6 days ago

auditedBalanceCollection.js Code Review

Architecture:

Design:

JavaScript:


AuditedBalanceSchemaInput.jsx Code Review

Architecture:

Design:

JavaScript:

React:

TannerBerry commented 2 days ago

AuditedBalanceCollection.js DE-01 (ln 15-160): The schema contains many fields, and some of them are optional. Consider abstracting some of the repeating parts into reusable schema definitions to improve maintainability. ES-01 (ln 158): The ESLint directive eslint-disable-next-line no-restricted-syntax should be avoided AuditedBalanceSchemaInput.jsx DE-06 (ln 98-159):possibly try refactoring some calculations into smaller helper functions for better readability and reusability. ES-01 (ln 136, ln 142): The ESLint directive eslint-disable-next-line max-len should be avoided by breaking the long lines into multiple shorter lines.

Sydnee-You commented 2 days ago

auditedBalanceCollection.js

AR-01 : directory has an uppercase starting letter which is inconsistent w/the other directories.

DE-07: line 17 gives a warning about duplicate code fragments as it is 138 lines of duplicates.

ES01 on line 158: docID variable is redundant.

RE-09 on line 15 - prefers function to classes.

AuditedBalanceSchemaInput.jsx

DE-03 on line 215 - code commented out.

DE-07 on line 13 (duplicate 138 line code fragment)

ES-01: return from swal ignored

RomaMalasarte commented 2 days ago

auditedBalanceCollection.js

Design-08, Group fields that are repetitive into sub schemas. JS-01: Many of the fields are not following camelCase convention. JS-04, L165-169: Use object deconstruction on data and assign it to updateData.

AuditedBalanceSchemaInput.jsx

Design-01, L163 and onwards: extract the calculations into helper functions to reduce code duplication. JS-01: Many of the fields are not following camelCase convention. JS-04, L164: Use object deconstruction to extract values in data.

blakewatanabe commented 2 days ago

auditedBalanceCollection.js

AR-01: Obey file name conventions - maybe it would be better to match the name of the file with the collection name established in the code https://github.com/manoa-inspire/MATP/blob/642f69fc650cca780c0b3ba9fc93c7464a2e0ad1/app/imports/api/Inputs/auditedBalanceCollection.js#L233

ES-01: No errors, avoid warnings - While it's used to disable an ESLint rule and avoid lint errors, try to avoid using eslint disables if possible. https://github.com/manoa-inspire/MATP/blob/642f69fc650cca780c0b3ba9fc93c7464a2e0ad1/app/imports/api/Inputs/auditedBalanceCollection.js#L223

AuditedBalanceSchemaInput.jsx

ES-01: No errors, avoid warnings - try to avoid using eslint disables if possible (multiple uses) https://github.com/manoa-inspire/MATP/blob/642f69fc650cca780c0b3ba9fc93c7464a2e0ad1/app/imports/ui/pages/AuditedBalanceSchemaInput.jsx#L173

DE-05: Ensure comments are appropriate - I think this comment could be removed and isn't really needed. Possibly the second one as well. https://github.com/manoa-inspire/MATP/blob/642f69fc650cca780c0b3ba9fc93c7464a2e0ad1/app/imports/ui/pages/AuditedBalanceSchemaInput.jsx#L224 https://github.com/manoa-inspire/MATP/blob/642f69fc650cca780c0b3ba9fc93c7464a2e0ad1/app/imports/ui/pages/AuditedBalanceSchemaInput.jsx#L219

DE-03: Eliminate dead code - If it's not going to be used then maybe it's best to remove the commented out line of code. https://github.com/manoa-inspire/MATP/blob/642f69fc650cca780c0b3ba9fc93c7464a2e0ad1/app/imports/ui/pages/AuditedBalanceSchemaInput.jsx#L215

cjochim commented 2 days ago

auditedBalanceCollection.js

AuditedBalanceSchemaInput.jsx

juncell-venzon commented 2 days ago

auditedBalanceCollection.js

AR-01: The file follows good structure with clear methods and publications.

DE-01: The schema has many repeated fields so try to consider creating smaller reusable schemas for common sections like Capital Assets and Liabilities to make the code cleaner.

DE-02: Use constants for collection names and publication names to avoid typos and make them easier to maintain.

JS-01: In update(), use restructuring to make the Object.entries(data) loop cleaner.

AuditedBalanceSchemaInput.jsx

AR-01: The file is structured properly and matches React standards.

DE-01: The submit() function contains several long calculations. Extract these into helper functions to make the code easier to maintain and read.

DE-02: Move the inline button styles to CSS classes for consistency.

JS-01: Use useRef instead of assigning refs directly to fRef.

TST-01: Ensure form validation errors are displayed properly with feedback to the user.

cash-baker commented 11 hours ago

auditedBalanceCollection.js

DE-07: Delete any duplicate code fragments ES-01: Avoid using eslint-disable to hide ESLint warnings/errors AR-01: Follow file naming conventions, changing file name to AuditedBalanceCollection.js would be a better alternative

AuditedBalanceSchemaInput

DE-03: Get rid of any dead code, would recommend reviewing any code fragments that are commented out