search

Team-Half-and-Half / spirebooks

MIT License
0 stars 0 forks source link

Review: DataAnalysis.jsx and AddMoney.jsx #42

Closed beydlern closed 1 month ago

beydlern commented 2 months ago

Overview

Please pay attention too:

Review Branch

review-1

Files to review

Checklists

Due date

Monday, 9/16/2024

For more information

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

XavierBurt commented 1 month ago

DataAnalysis.jsx

Design-01: The code from line 57 to line 75 is essentially repeated, can make a new method/class with parameters that significantly reduces the amount of lines in the file.

Design-08: The array on line 6 to line 50 is very long and repetitive, I know it's just for testing purposes, but since this is a code review I'll say it anyway

React-01: Essentially the same as my comment from Design-01, can make the graphs their own components, and then make those components load onto the website.

AddMoney.jsx

React-01: The schema should be moved to its own file

PaytonHAH commented 1 month ago

Design-03 AddMoney Line 14 - 17 and Line 168 - 172: Unused data for the schema, previous snippet from add stuff file.

Design-01 DataAnalysis: Duplicate blocks of code for graphs.

caspascual commented 1 month ago

AddMoney.jsx

L13-173: Design-09: Could possibly make the SimpleSchema into its own component in case we need it again and reduce the amount of lines in AddMoney.jsx

L194: Design-05: Comment regarding uniforms seems unnecessary

L203-240: Design-09: Like above, I think we can separate all the assets/investments into their own components

DataAnalysis.jsx

L56-176: Design-09: Could make each card into their own components

L7-49: Design-09: Example data could be placed into settings file

L7-49: REACT-06: If data should stay in DataAnalysis.jsx, constants should be defined in withTracker()

beydlern commented 1 month ago

DataAnalysis.jsx

DESIGN-01: Code may be refactored to reduce duplication, a function may be implemented to generate the cards instead of duplicating each card code

L7: data array should be connected to the database to provide values for each individual graph instead of placeholder values

AddMoney.jsx

L184: Inappropriate reference to ListStuffs collection

L197: Inappropriate reference to ADD_STUFF PAGE_ID

L200-L246:Multiple columns may be used instead of one gigantic column containing all of the form fields

t-tirabassi commented 1 month ago

Design

AddMoney.jsx: the form card could be split up into multiple columns rather than just one DataAnalysis.jsx: code could maybe be condensed if possible, possibly made into a function that can create the graphs for each one while conforming to the names and values

JS

DataAnalysis.jsx: the properties of the data const could maybe be destructured for LineChart AddMoney.jsx: AddMoney could be renamed to something more fitting, the spread operator could be used for submit

React

AddMoney.jsx: withTracker could maybe be used at line 181

samallari commented 1 month ago

DataAnalysis.jsx could possibly use the built-in map function to pass data into a function that generates the graphs instead

AddMoney.jsx LN 8: get rid of import { Stuffs }, replace with correct database LN 184: uses Stuffs collection could rename file to something more descriptive, maybe like InputClientInfo

bksnelson commented 1 month ago

AddMoney.jsx

Design-03: Remove Stuffs related lines. Adjust for right subscriptions. L 181 - 192

JS-01: We should coordinate schema names to match in collection and form.

If using name and year, it should be a required field. L14, L18.

DataAnalysis.jsx

Design-01: The charts can be reduced to one component and separated from the DataAnalysis page.

We could add some info/summary for each chart so that the user can read some details about their numbers. For later milestones maybe. We should discuss how the charts are laid out on the page, the types of graphs, and what data we are going to use.

jsapolu99 commented 1 month ago

DataAnalysis.jsx

Design-01: Lines 58-175 are essentially all duplicates. Could make a separate graph component to alleviate some of the repetition.

Arch-03: file name could be changed to Dashboard.jsx

AddMoney.jsx

React-01: The schema should be its own file

XavierBurt commented 1 month ago

Outcome of Review Meeting

New Issues: