Refactor Find Sat Plugin #1244
Refactor Find Sat Plugin #1244
Conversation
…ization - Consolidated duplicate RAE filter methods (checkAz_, checkEl_, checkRange_) into single generic filterByRaeProperty_ method - Consolidated duplicate property filter methods (checkInc_, checkArgPe_, checkRightAscension_, checkPeriod_) into filterBySatelliteProperty_ method - Extracted utility functions (isValidNumber, getSatelliteRae, caseInsensitiveSort, extractPayloadPartial) to reduce code duplication - Improved input validation with validateSearchParams_ method - Extracted dropdown population logic into reusable methods - Fixed Promise handling in findByLooksSubmit_ to properly resolve/reject - Added comprehensive JSDoc documentation for all methods - Extracted displaySearchResults_ for better separation of concerns - Added constants for default margins and configuration values - Improved error handling and user feedback - Removed eslint-disable comments by fixing underlying issues This refactor significantly reduces code duplication, improves maintainability, and makes the codebase easier to understand and extend.
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Find Sat Plugin to improve code organization, maintainability, and readability by extracting utility functions, consolidating duplicate logic, and adding comprehensive documentation.
Key Changes:
- Removed complexity and prefer-const eslint disables, addressing code quality issues
- Consolidated duplicate filtering logic into generic
filterByRaeProperty_andfilterBySatelliteProperty_methods - Extracted helper methods for dropdown population, form parsing, and search result display
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const uniquePayloads = getUnique(payloadPartials).sort(caseInsensitiveSort); | ||
|
|
||
| uniquePayloads.forEach((payload) => { | ||
| if (payload && payload.length > MIN_PAYLOAD_LENGTH) { |
There was a problem hiding this comment.
There's an inconsistency in the payload length filtering. Line 382 filters for partial.length >= MIN_PAYLOAD_LENGTH (3 or more characters), but line 387 checks payload.length > MIN_PAYLOAD_LENGTH (more than 3 characters, i.e., 4+). This means payloads with exactly 3 characters pass the first filter but are then excluded from the dropdown. Either both should use >= or both should use > for consistency.
| if (payload && payload.length > MIN_PAYLOAD_LENGTH) { | |
| if (payload && payload.length >= MIN_PAYLOAD_LENGTH) { |
| // const parseFloatOrDefault = (value: string, defaultValue: number): number => { | ||
| // const parsed = parseFloat(value); | ||
|
|
||
| // return isValidNumber(parsed) ? parsed : defaultValue; | ||
| // }; |
There was a problem hiding this comment.
The commented-out code for parseFloatOrDefault should be removed rather than left as dead code. If this utility might be needed in the future, consider removing it entirely or implementing it if it has a use case.
| // const parseFloatOrDefault = (value: string, defaultValue: number): number => { | |
| // const parsed = parseFloat(value); | |
| // return isValidNumber(parsed) ? parsed : defaultValue; | |
| // }; |
| if (isSpecificCountry) { | ||
| const countries = countryCode | ||
| .split('|') | ||
| .filter((item, index, arr) => item && arr.indexOf(item) === index); // Remove duplicates and empty strings |
There was a problem hiding this comment.
[nitpick] The filter logic for duplicates in line 709 has a subtle issue. Using arr.indexOf(item) === index removes duplicates correctly, but the filter also includes item && which will filter out falsy values. If country codes could be empty strings (which seems handled), this is fine, but the comment says "Remove duplicates and empty strings" while the code doesn't explicitly check for empty strings - it checks for falsy values. Consider using item !== '' for clarity if that's the intent.
| .filter((item, index, arr) => item && arr.indexOf(item) === index); // Remove duplicates and empty strings | |
| .filter((item, index, arr) => item !== '' && arr.indexOf(item) === index); // Remove duplicates and empty strings |
| return satellites.filter((sat) => { | ||
| const value = sat[property]; | ||
|
|
||
| return value > min && value < max; |
There was a problem hiding this comment.
The filterBySatelliteProperty_ method uses value > min && value < max which excludes the boundary values. This is inconsistent with filterByRaeProperty_ which uses >= and <= (inclusive boundaries). Consider whether orbital parameters should use inclusive or exclusive boundaries for consistency, and ensure this matches the expected behavior. If exclusive boundaries are intended, this should be documented.
| return value > min && value < max; | |
| return value >= min && value <= max; |
| private static checkRcs_(satellites: DetailedSatellite[], minRcs: number, maxRcs: number): DetailedSatellite[] { | ||
| return satellites.filter((sat) => { | ||
| const rcs = sat.rcs ?? NaN; | ||
|
|
||
| private static checkRcs_(possibles: DetailedSatellite[], minRcs: number, maxRcs: number) { | ||
| return possibles.filter((possible) => (possible?.rcs ?? -Infinity) > minRcs && (possible?.rcs ?? Infinity) < maxRcs); | ||
| // Handle satellites with no RCS data | ||
| if (isNaN(rcs)) { | ||
| return false; | ||
| } | ||
|
|
||
| return rcs > minRcs && rcs < maxRcs; | ||
| }); |
There was a problem hiding this comment.
In the old code, checkRcs_ returned true for satellites with undefined RCS values (using -Infinity and Infinity as defaults). The new implementation returns false for satellites without RCS data. This is a breaking change in behavior that could filter out satellites that were previously included in results. If this behavior change is intentional, it should be noted; otherwise, consider maintaining backward compatibility.
| } else { | ||
| uiManager.toast(`Found ${this.lastResults_.length} satellite(s)`, ToastMsgType.normal); |
There was a problem hiding this comment.
[nitpick] The new implementation adds a success toast message when satellites are found (line 485), but this wasn't present in the original code. While this improves user feedback, it could be noisy if searches return results frequently. Consider if this aligns with the application's UX patterns for success feedback.
| } else { | |
| uiManager.toast(`Found ${this.lastResults_.length} satellite(s)`, ToastMsgType.normal); |
| currentSatellite.position, | ||
| ServiceLocator.getSensorManager().currentSensors[0], | ||
| ); | ||
| if (results.length > limit) { |
There was a problem hiding this comment.
[nitpick] The condition check on line 547 changed from possibles.length >= limit to results.length > limit. This means that when results exactly equal the limit, the toast message "Too many results, limited to X" will no longer be shown. The original behavior would notify users even when hitting the exact limit. Consider whether this change in UX feedback is intentional.
| if (results.length > limit) { | |
| if (results.length >= limit) { |
|
No description provided.