lukeoftheshire
commented
2 years ago In addition, this report regarding the LAMP codebase may be helpful. While some of the issues raised have been resolved or are outdated, there are many that have yet to be resolved. Many of the issues appear in the paragraphs above, some do not.
Currently, a number of things about the LAMP Dashboard code are unsatisfactory or do not live up to desired standards. In general, these fall into one of three categories:
@avaidyam and I have taken a short view of the code and found these issues, which I will group into the above categories. Issues marked with a * are ones that (in my personal opinion) may be of particular importance.
Code Cleanup
- MaterialUI's useStyles is used frequently - this would be better replaced with themes or inline styles \* - Icons should be part of LAMP activities, not the dashboard. - Better folder hierarchy needs to be enforced - currently many folders just live in the `src` directory which makes it difficult to locate specific files/components. \* - Better documentation. Currently, most React Components in LAMP are under/undocumented. This makes it very difficult to modify or improve existing code. \* - Files that are not used, like `NewMedication.tsx` need to be removed (along with any references to them in the codebase) - Unused node modules must be removed. \* - Misspellings (such as Steak instead of Streak) need to be found and removed to prevent unexpected bugs down the road. - The translation function `t` should not use full english text as the key. E.g. `t("This activity is not yet available in mindLAMP 2.") → t("ACTIVITY_UNAVAILABLE")` - There is a large, hard-coded multi-line query in `SaveResearcherData.tsx` that makes references to some now-unused tags, `to.unityhealth.psychiatry.settings`. These should be removed and the query refactored where possible. - Remove the unused `examples.story.tsx `(@avaidyam) - Remove unusued imports and variables - Rename Portal href to Prevent (@avaidyam ) - `BottomMenu.Tsx` has duplicated code - `PreventDBT.tsx` has hardcoded, unlocalized text, and inconsistent formatting - Remove `ActivityPopup.tsx` - Remove `lamp_activity` description tags and fold them into the activities themselves (e.g. for surveys)Code Modification/Improvements
- MaterialUI's useStyles is used frequently - this would be better replaced with themes or inline styles - The Data Portal is overly reliant on math and data manipulation - this should be simplified to be easier for a new/unfamiliar developer to work with. - Frequently, we load data we do not need to load, or load it inefficiently - the most jarring example of this is when all participant data is loaded each time the Researcher view is opened. See https://github.com/BIDMCDigitalPsychiatry/LAMP-platform/issues/491 for a recurring example of this issue. \* - We should mandate Typescript type information and disallow type inference and use of the 'all' type. - The Feed schedule calculation is non-ideal. It shoud be cleaned up and pulled into a separate file. Additionally, we should switch to using cron syntax. - In general, we should follow the standard shown in `ConfirmationDialog.tsx` when defining React functions: - Provide both properties and `...props` - Pass props to the first child div - Declare function parameters and their types - User-editable text boxes like descriptions should support markdown - Upgrade to MaterialUI5 - Figure out which components we are *unnecessarily* creating internally and use outside components instead.Improved Modularity
- Many functions are repeated in multiple locations throughout the codebase (e.g. getDateString). These files should be moved into a single location to reduce clutter and improve our ability to debug code \* - Each file needs to represent, where at all possible, a single react component.