AsyncTI4 / TI4_map_generator_bot

Async TI4 Game Management Bot
Other
33 stars 22 forks source link

Fixing some bugs #2091

Closed Fingon00 closed 1 week ago

Fingon00 commented 1 week ago

@CodiumAI-Agent /review

CodiumAI-Agent commented 1 week ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Refactoring
The method `getTileDistances` has been refactored to include a new parameter `forMap`. This change affects multiple files and methods that call `getTileDistances`. Ensure that the new parameter is correctly used and that it doesn't introduce any side effects or bugs. Logic Change
The logic for handling `skipbuild` and `orbital` in `placeUnitAndDeleteButton` has been modified. It now checks for substrings "skipbuild" and "orbital" in the `skipbuild` string. Ensure that this change is intended and correctly handles all cases where these conditions are used.
Fingon00 commented 1 week ago

@CodiumAI-Agent /improve

CodiumAI-Agent commented 1 week ago

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible bug
Add a null check for player2 to prevent potential null pointer exceptions ___ **Ensure that the combat is initiated only if player2 is not null to avoid potential
null pointer exceptions.** [src/main/java/ti4/commands/units/AddRemoveUnits.java [264-268]](https://github.com/AsyncTI4/TI4_map_generator_bot/pull/2091/files#diff-20a33298f6bf9c106113264a0e3c72dd57ebc7048bd291112054bae00646ade2R264-R268) ```diff -if (player1 != player2 && !tile.getPosition().equalsIgnoreCase("nombox") && !player1.getAllianceMembers().contains(player2.getFaction())) { +if (player2 != null && player1 != player2 && !tile.getPosition().equalsIgnoreCase("nombox") && !player1.getAllianceMembers().contains(player2.getFaction())) { if ("ground".equals(combatType)) { StartCombat.startGroundCombat(player1, player2, game, event, tile.getUnitHolderFromPlanet(planetName), tile); } else { StartCombat.startSpaceCombat(game, player1, player2, tile, event); ```
Suggestion importance[1-10]: 9 Why: Adding a null check for `player2` is crucial to prevent potential null pointer exceptions, which could cause the application to crash. This is a significant improvement in terms of code robustness.
9
Add a null check for tile to ensure it is not null before accessing its methods ___ **Add a null check for tile before accessing its methods to prevent potential null
pointer exceptions.** [src/main/java/ti4/helpers/ButtonHelperAbilities.java [122-130]](https://github.com/AsyncTI4/TI4_map_generator_bot/pull/2091/files#diff-113b617c9c34a3b70b10a9ec3d4768efb27400358dc680dd900111946447f423R122-R130) ```diff if (!forMap) { if (tile == null || (tile.isNebula() && player != null && !player.getAbilities().contains("voidborn") && !ButtonHelper.isLawInPlay(game, "shared_research")) || (tile.isSupernova() && player != null && !player.getAbilities().contains("gashlai_physiology")) || (tile.isAsteroidField() && player != null && !player.getTechs().contains("amd") && !player.getTechs().contains("absol_amd"))) { continue; } - if (tile != null && tile.isGravityRift(game)) { - num = -1; - } - if (distances.get(existingPosition) != null) { - distance = distances.get(existingPosition) + 1; + if (tile != null) { + if (tile.isGravityRift(game)) { + num = -1; + } + if (distances.get(existingPosition) != null) { + distance = distances.get(existingPosition) + 1; + } } ```
Suggestion importance[1-10]: 8 Why: The suggestion correctly identifies a potential null pointer exception and provides a solution to prevent it, improving the code's stability and reliability.
8
Enhancement
Use getOrDefault to simplify the retrieval of distance values and handle potential nulls ___ **Refactor the method to handle potential null values in distances.get(tilePosition2)
to avoid NullPointerException.** [src/main/java/ti4/commands/special/CheckDistance.java [68-70]](https://github.com/AsyncTI4/TI4_map_generator_bot/pull/2091/files#diff-40ab8bd19d846771c25b227f1c9f313b1d66b83166e77b31ddbb97fc37b943f9R68-R70) ```diff -if (distances.get(tilePosition2) != null) { - return distances.get(tilePosition2); -} -return 100; +return distances.getOrDefault(tilePosition2, 100); ```
Suggestion importance[1-10]: 7 Why: Using `getOrDefault` simplifies the code and handles potential null values, enhancing code clarity and preventing possible exceptions, though it is not a critical fix.
7
Refactor the player comparison to use .equals() for object equality check ___ **Refactor the condition to check if p2 is not the same as player or if they are
alliance members, to simplify the logic and improve readability.** [src/main/java/ti4/helpers/FoWHelper.java [700-702]](https://github.com/AsyncTI4/TI4_map_generator_bot/pull/2091/files#diff-b046bd2d9aace0788ae75125e00acff3c413b8dabb8806403dfd9eb2d7712e6dR700-R702) ```diff -if (p2 == player || player.getAllianceMembers().contains(p2.getFaction())) { +if (p2.equals(player) || player.getAllianceMembers().contains(p2.getFaction())) { continue; } ```
Suggestion importance[1-10]: 6 Why: The refactoring suggestion improves code readability by using `.equals()` for object equality, which is a minor enhancement but does not address a critical issue.
6