⚡ Bolt: Optimize GetRackCount with O(1) network map lookup#171
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 high |
🟢 Metrics 4 complexity · 0 duplication
Metric Results Complexity 4 Duplication 0
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR introduces an O(1) optimization for GetRackCount by utilizing NetworkMap.instance, but Codacy analysis indicates the implementation is not currently up to standards. While the optimization is valuable, the current approach introduces maintenance risks and lacks diagnostic visibility.
The most critical issues involve the use of a hardcoded magic index for device identification and an empty catch block that swallows exceptions in the optimized path. Additionally, there is a potential functional discrepancy: FindObjectsOfType generally counts only active objects, whereas NetworkMap may include inactive ones, which could lead to incorrect logic in certain game states.
About this PR
- No unit tests were included in the PR to verify the new logic or the safety of the fallback mechanism when the NetworkMap is uninitialized or the internal array structure changes.
Test suggestions
- Verify GetRackCount returns value from NetworkMap index 2 when instance and array are valid
- Verify GetRackCount falls back to FindObjectsOfType when NetworkMap.instance is null
- Verify GetRackCount falls back to FindObjectsOfType when GetNumberOfDevices returns an array with length <= 2
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify GetRackCount returns value from NetworkMap index 2 when instance and array are valid
2. Verify GetRackCount falls back to FindObjectsOfType when NetworkMap.instance is null
3. Verify GetRackCount falls back to FindObjectsOfType when GetNumberOfDevices returns an array with length <= 2
Low confidence findings
- There may be a functional discrepancy between the two methods:
FindObjectsOfType(by default) only counts active objects, whileNetworkMapcounts might include inactive ones. This should be verified to ensure the mod's logic remains consistent.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| } | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
🔴 HIGH RISK
Suppressing all exceptions without documentation or logging can hide runtime failures in the IL2CPP interop layer. While a fallback to 'FindObjectsOfType' is provided, the empty catch block should at least contain a comment explaining this intentional behavior. If a logger is available, log the exception at a 'Debug' level to aid in future troubleshooting.
|
|
||
| // Index mapping for NetworkMap.instance.GetNumberOfDevices() | ||
| // [0] = servers, [1] = switches, [2] = racks | ||
| private const int DEVICE_INDEX_RACKS = 2; |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The hardcoded index 2 for DEVICE_INDEX_RACKS is fragile and depends on the internal implementation of NetworkMap. If the game updates and reorders the device array, this method will return incorrect data without failing. Try searching the global::Il2Cpp namespace for an enum or constant definitions related to NetworkMap device types to replace the hardcoded DEVICE_INDEX_RACKS index.
FindObjectsOfType<global::Il2Cpp.Rack>()call inGregFacilityModule.GetRackCountwith an O(1) lookup usingIl2Cpp.NetworkMap.instance.GetNumberOfDevices().FindObjectsOfTypescans the entire object hierarchy and is highly inefficient to call, particularly in late-game scenes with many devices.GetRackCount()is frequently queried. The array index lookup takes negligible time compared to scanning the full Unity hierarchy.PR created automatically by Jules for task 14290982680870210554 started by @mleem97