Refactoring Plan - benjaminsunliu/ConUMap GitHub Wiki
| Refactoring Type | Reason | Commit |
|---|---|---|
| Change value to reference | To unify styles across our app so changes only need to be made in a single location | Change (Line 6) & Usage (Line 24) |
| Rename Variable | Variable names such as g are unreadable and require extra effort to understand. Renaming it to gestureState makes it more obvious |
Change (lines 61-67 building-info-popup.tsx) |
| Function Caching | To reduce the number of recomputations of functions, we moved them to useCallbacks |
Change (Lines 32, 41, 63). useCallback hook lets us cache a function definition between re-renders |
| Move Definition | Two heavily used type definitions that were placed in a component file were moved to our type definition file for easier maintainability |
Extraction (Lines 16-21 of mapTypes.ts) & Removal (Lines 109-114 of map-viewer.tsx) |
| Dead Code | Removing unused imports helps keep the code readable | Change (Line 2) |
| Change value to reference | To unify styles across our app so changes only need to be made in a single location |
Extraction (Lines 44-49 & 90-95 of theme.ts) & Removal (Lines 33-39 of campus-toggle.tsx) |
| Rename Variable | Fix a typo in a variable name so that future refactoring will remain consistent with naming throughout the repo |
Change (Lines 39, 41, 45, 47 of campus-toggle.tsx) |
| Refactoring Type | Reason | Commit |
|---|---|---|
| Implement Testing Pattern | Changed tests to adopt the Arrange Act Assert pattern for unit tests |
Change (Lines 8-25 of highlight-user-building.test.ts), Example: |
| Remove Dead Code | Removed unused test setup and teardown to increase readability and performance |
Change (Lines 353-360 of map-viewer.test.js): Remove mocking time for tests of polygons on map |
| Move Definition | Moved the definition of getBuildingPolygons to a util file to be used in other parts of the code instead of just in tests |
Extraction (all of getBuildingPolygons.ts) & Removal (Lines 45-70 of map-viewer.test.js) |
| Remove Duplicate Code | Replaced code that duplicated a function in the same file |
Change (Line 250 of map-viewer.tsx) & Existing Code (Lines 73-77 of mapT-viewer.tsx) |
| Improve Performance | Memoized functions to improve performance |
Change (Lines 39, 54, 58, 62, 67, 72, 78, etc. of building-selection.tsx) Example: useCallback hook lets us cache a function definition between re-renders |
| Move Definition | Moved type definition to type file to be used in many places |
Extraction (all of buildingTypes.ts) &
Removal (Lines 8-15 of building-selection.tsx) |
| Refactoring Type | Reason | Commit |
|---|---|---|
| Replace Inline Code with Function Call | Replaced usage of replace with replaceAll when using the global regex flag to ensure correct global replacements |
Change (Lines 148-150 of building-info-popup.tsx) |
| Restrict Modification Permissions to Component Props | Added readonly to component props to prevent unintended mutation and improve code safety |
|
| Incomplete Library Type Definition | Added type declarations for previously untyped modules to improve type safety and maintainability |
|
| Remove Dead Code | Removed an unnecessary theme provider wrapper to simplify component hierarchy |
Change (Line 17 of _layout.tsx) &
Justification
|
| Remove Duplication | Simplified the useColorScheme hook implementation to reduce unnecessary usage of ?? "light" in multiple places |
Wrap useColorScheme (all of use-color-scheme.ts) to simplify code such as this (Line 23 of building-selection.tsx) |
| Rename/Retype Variable | Renamed magic constants (LOY and SGW) and replaced them with an enum Campus to improve readability and maintainability |
Addition of Enum (Lines 14-17 of campus-toggle.tsx) &
Example Usage (Line 76 of campus-toggle.tsx)
|
| Redistribution of Building Data | Merge different building information and divide buildings into files for easier maintainability and searchability of data |
Extraction of data merge logic to a script: Addition (all of building-aggregation.ts), Removal (all of mapData.ts). New data under data/buildings/<building_code>.json
|
| Remove Dead Code | Removed unnecessary tests that did not provide additional coverage or value |
Removal due to addressed (Lines 258-289 of map-viewer.test.js) : comment
|
| Function Extraction | Extract current building prioritization of current building logic into its own function |
Removal (Lines 49-58 of building-selection.tsx) |
Extraction (Lines 22-39 of building-selection.tsx)
|
| Class (React Component) Extraction | Extracted popup to create a generic popup component to be able to reuse. A component is equivalent to a class in the react framework |
Change (all of popup.tsx) & Usages Examples:
|
| Remove Duplicate Code | Removed leftover duplicate style sheet from generic popup extraction |
Removal (Lines 287-306 of building-info-popup.tsx) due to extraction to General Info Popup
|
| Extract Type | Extracted type definition to seperate file to be reused in other places if needed |
Change (Line 10 of buildingTypes.ts, was originally defined in routes-info-popup.tsx) |
| Remove Dead Code | Removed unused imports of Scrollview |
Change (Line 6 and 5 of building-info-popup.tsx and routes-info-popup.tsx respectively) |
Update in progress
| Refactoring Type | Reason | Commit |
|---|---|---|
| Rename Variable | Several local variables (r, durSecs, stepDurSecs, td) were lengthened for clarity within the normalizeRoute method. |
Change (Lines 197, 198, 201, 204) of directions.ts. |
| Replace Loop with Pipeline | Instead of manually indexing the results (i.e walking = results[0], transit = results[1], etc..) to build the final object, this was replaced with a functional transformation. |
Change (Lines 447-449). in directions.ts
|
| Extract Class | The logic for API interaction and request construction was moved out of the global function into a dedicated class structure. The abstract class GoogleRoutesStrategy now encapsulates the buildRequestBody and fetch logic that was previously inline in the main function. The RouteStrategy interface encapsulates the fetch logic. |
Extraction (Lines 290-292, 297-318, 320-374) of directions.ts
|
| Move Function | The internal logic of the old fetchDirections was moved into the fetch method of the strategy classes |
Change (Line 425) of directions.ts
|
| Replace Constructor with Factory | Different transportation modes were found using a lookup (MODE_MAP). This has been replaced by a RouteStrategy interface and several concrete implementations (WalkingRouteStrategy, TransitRouteStrategy, etc.), which are created using a RouteStrategyFactory. |
Strategy Classes (Lines 376-400) and Factory (Line 403) of directions.ts
|
Update in progress
| Refactoring Type | Reason | Commit |
|---|---|---|
| Move Hardcoded Colors to Themes file | Some colors used for the indoor navigation view and path were hardcoded. A suggestion was made through a comment from PR #198 to move these color values into the theme.ts file. This improves maintainability, consistency, and makes it easier to update styling across the application. |
Commit theme.ts.
|
| Rename variables related to floor switching | When the indoor navigation UI was initially designed, next and prev buttons were intended for steps of indoor navigation, but was changed to allow viewing different floors. The remark was made through a comment from PR #198 to rename the related variables so that their names accurately reflect their updated responsibility. This improves code readability and reduces potential confusion for future developers. |
Commit [buildingCode].tsx.
|
| Extract magic values to named constants | Angle threshold values used to determine navigation instructions were extracted into named constants to improve readability and maintainability. PR #200 |
Commit indoorStepInstructions.ts.
|
| Replace hardcoded edge-type strings with constants | Hardcoded string literals representing edge types were replaced with a typed constant to improve type safety and prevent typos. PR #200 |
Commit indoorStepInstructions.ts.
|
| Refactor Indoor Next/Prev Buttons to use Command Pattern | Refactored the indoor Next/Prev buttons to use a command-style pattern so the same controls can switch between floor navigation and step navigation without coupling the UI to mode-specific logic. The screen now creates mode-specific command objects, while the control component simply renders and executes them. PR #216 |
Commit indoorNavigationCommand.ts.
|
| Extract Function | Extracted common logic for navigation into navigate function. This function is then called in navigateToBuilding and the new funciton navigateToPOI PR #197
|
Commit (Lines 522-543, 559, 570) map-viewer.tsx.
|
| Change Function Declaration | Changed the function signature of focusBuilding to go from a BuildingInfo object to a pair of coordinate. This was to make it more generic and so that it could be reused for POI as well as Buildings.PR #197 |
Commit (Lines 345, 375, 416, 672) map-viewer.tsx.
|
| Extract Function | Extracted all pre routings steps such as clearing variables and animations to a seperate function so it can be used in routing for POI and BuildingsPR #197 |
Commit (Lines 429-441, 417, 425) map-viewer.tsx.
|
| Move function/component definition | Moved the definition of functions responsible for rendering the indoor POI markers to a separate file for separations of concern and as a pre-step to the factory pattern refactoring |
Commit map-viewer.tsx (Lines 242-472) moved to indoor-marker.tsx.
|
| Replace manual rendering with Factory Method | The previous way of rendering the different POI markers tried to render all of them at once and only kept the ones that succeeded. This could be improved and so we decided to use a variation of the factory method to slightly improve it. See Architecture for more details |
Commit building-floor.tsx (Lines 53) Factory Used default-poi-marker.tsx and default-marker-factory.tsx added to implement the factory pattern.
|