Code Review Report - benjaminsunliu/ConUMap GitHub Wiki

Release 1

#78 - tsk10403 display building info

The first part of the review was mostly refactoring, changing variable names, importing unified styles instead of hard-coding them, whereas the second half of the review was spent fixing a crash that only happens on IOS and collaborating to fix it.

#76 - Extra Building Info

This PR was about sharing the building information data format amongst our team members and making sure we all agree on it. Some other inconsistencies and potential problems were also addressed such as typos in field names and security vulnerabilities detected by sonarqube.

#75 - Add SGW and LOYOLA building outlines + retrieve user location

This was the first major PR of the project. It implemented a core feature of the app, displaying and moving a map that shows concordia buildings. Certain bugs such as some buildings not displaying correctly were found and fixed during review. Other refactoring opportunities were also addressed like moving a file its correct position in the file hierarchy, removing unused code, and extracting certain definitions to their correct location for easy reuse.

#87 - Unit Testing

This PR was about the addition of all of our unit tests to date. We discussed potential problems with certain tests that would always pass no matter the condition of the code, or that would always pass since there were no assertions. It also was used to spread knowledge about the testing frameworks that we will be using throughout the project

#79 - Implement Toggle Between SGW and Loyola

This PR looked at removing dead code, replacing values with references to have a single point of modification for app colours, and fixing a couple visual bugs with the help of several teammates.

Release 2

#100 - Refactoring and Fixing SonarQube Issues

This is the first main PR we have taken in terms of refactoring. We discussed mainly discussed small changes that would improve the overall quality of our code but also there were talks about understanding the code and why things were done a certain way. Opening discussions such as these on a variety of aspects of our code helped us make better QoL decisions for our codebase. However, it also made us realize that refactoring shouldn't simply be done with a single PR from time to time but rather constantly in all of our PRs. Essentially, when a refactoring issues is encountered, depending ont its severeness, it could be dealt within the same PR or in another small one.

#124 - Implement Directions with Google Routes API

The goal of this PR was to show directions to the user by using the Google API. We shared about potential improvements for the API behaviour, making sure that UI stay consistent across merges and purpose statements of functions in order to increase readability and auto-complete.

#121 - Shuttle Bus Schedule Integration

This PR was meant for implementing the shuttle schedule data in order to be used as a viable transportation method later on. The discussions included moving the schedule to be dynamically fetched instead of statically and addressing messy logs in our tests that make our CLI test report not concise.

#101 - Display Transportation Options Except Shuttle

The main goal of this PR was to show the different transportation options on the UI. We discussed changes that needed to be made in order to improving overall code health by refactoring code for better readability, less complexing, and higher cohesion.

#137 - Allow Setting Building Start Through Pop Up

The point of this PR was simple, allow the user to set the building they are viewing as a starting building for the directions they were going to search. We discussed a bug found where the start gets overwritten by the current location even after setting it. Furthermore, render optimization was discussed along some potential future refactoring tasks.

Release 3

#198 - Implement Indoor Room Selection

The goal of this PR was to implement the indoor navigation user interface, allowing users to select a start room and end room inside a building and generate navigation paths between them. This included improving the transition between outdoor and indoor navigation views, adding labels to hall floor plans, refining room search behavior, and improving the visual clarity of indoor paths by adjusting color and thickness.

During the review process, we discussed the importance of consistent naming to reflect the actual behavior of the system, particularly renaming variables that previously referred to navigation steps but now represented floor navigation. We also addressed maintainability concerns by moving hardcoded styling values (such as colors and overlays) into the shared theme configuration, ensuring consistency across the application. Additionally, fixes were made for mislabeled rooms and search edge cases, improving the reliability of the indoor navigation experience. These discussions focused on the importance of code readability, centralized configuration, and UI consistency when implementing user-facing features.

200 - Implement Hybrid Indoor-Outdoor Navigation

The goal of this PR was to integrate indoor and outdoor navigation into a unified navigation flow, enabling users to seamlessly transition between outdoor routes and indoor directions. This included implementing step-by-step indoor navigation with highlighted path segments, handling transitions between indoor and outdoor environments, supporting floor changes via elevators or escalators, and selecting the shortest entry or exit point to a destination room.

During the review process, we discussed improving code clarity and maintainability by replacing hardcoded string values with union types or constants, reducing the risk of errors and making the system easier to extend. We also reviewed the handling of edge cases in instruction formatting, such as returning empty values when no valid instruction is available, and emphasized documenting these decisions to avoid confusion during debugging. Another discussion focused on extracting "magic numbers" (such as angle thresholds used to determine turn instructions) into named constants, improving readability and making the logic easier to modify in the future. This strengthened the robustness of the navigation logic and helped make our code more maintainable, scalable and robust.

#131 - Integrating shuttle into routes and displaying

The point of this PR was to integrate the shuttle routes into our navigation feature and to display the route on the map. We discussed improvements for improper/vague documentation of certain functions to clarify what they do and their expected behaviour, using built-in comparators (which weren't specific enough for our use case), moving constants to the correct file, refactoring nested conditionals (by replacing them with guard clauses) to improve readability, improving our location comparison algorithm to use approximate locations instead of exact ones to reduce the number of fetches and unwanted behaviour when next to the location you want to head to, removing forgotten logs, possible alternatives to current algorithm implementation (such as fetching the shuttle schedule once and storing it rather than fetching it every time), and using the strategy pattern to properly encapsulate and decouple the shuttle navigation

#179 - feature: add 2d indoor map data showing rooms & within floor navigation

The goal of this PR was to add the skeleton for our indoor maps navigation, including map transition (outdoors to indoors) and Dijktra's shortest path algorithm. As part of the review process, we discussed the decision to use pngs instead of svgs as they had better performance, the potential bug of not doing a deep equality (but in this case we wanted to check for same reference), a request for documentation on what a long an complicated regex expression means, ensuring consistency in our floor numbering data, the reason why using text files for our json data is better for memory management purposes, a UX bug where the user could not leave the indoor map, the possible use of a Factory pattern to make the code for node icon generation more extendable, the ability to reuse a boolean function buildingHasNavigationData instead of rewriting it, the fixing of an if statement bug, and some slight UI tweaks for better usability.

#191 - Floor search

This PR implements indoor floor selection, room search for a specific building as well as showing the step by step directions for a specific floor. The review process in this PR was essential since we found many edge cases in the implementation that were only seen with human intervention. We discussed inconsitencies in the floor selection buidling name, placeholder information that was kept, search bar content, floor selection number. As testing implementations that were not up to par combined with end to end tests failing. This really proved that our entire review process is thourough and that our pipeline for accepting new features into the application will only accept the features that have been thouroughly tested and reviewed.