GH-874 Expand Random Teleport configuration. #874
Conversation
vLuckyyy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (11)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java (2)
8-8: LGTM: Good architectural improvementThe change from returning
inttoRandomTeleportRadiusRepresenteris a solid improvement that allows for more flexible border shapes beyond just square regions. This abstraction will make it easier to support different border configurations.
1-22: Consider adding documentationWhile the interface methods are fairly self-explanatory, adding JavaDoc comments would help clarify:
- Valid ranges for minHeight
- Expected behavior when blocks are in both unsafe and air sets
- Constraints for the RandomTeleportRadiusRepresenter implementation
package com.eternalcode.core.feature.randomteleport; import java.util.Set; import org.bukkit.Material; +/** + * Settings for random teleportation functionality. + */ public interface RandomTeleportSettings { + /** + * @return The radius representer defining the teleport boundaries + */ RandomTeleportRadiusRepresenter randomTeleportRadius(); RandomTeleportType randomTeleportType(); String randomTeleportWorld(); int randomTeleportAttempts(); + /** + * @return Set of blocks considered unsafe for teleportation + */ Set<Material> unsafeBlocks(); + /** + * @return Set of blocks treated as air for teleportation purposes + */ Set<Material> airBlocks(); + /** + * @return Minimum allowed height for teleportation + */ int minHeight(); }eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java (3)
5-6: Add class-level documentation.Consider adding Javadoc to explain the purpose of this class, its role in random teleportation, and the significance of the
@Contextualannotation for configuration handling.@Contextual +/** + * Implementation of RandomTeleportRadiusRepresenter that defines the boundaries + * for random teleportation in a world. This class is used for configuration + * serialization through CDN library. + */ public class RandomTeleportRadiusRepresenterImpl implements RandomTeleportRadiusRepresenter {
23-41: Add method documentation.Consider adding Javadoc to the getter methods to describe their purpose and return values, especially since they implement an interface contract.
Example improvement for one method:
@Override + /** + * Gets the minimum X coordinate for the teleport boundary. + * @return the minimum X coordinate + */ public int getMinX() { return this.minX; }
6-42: Consider adding utility methods for common operations.To improve usability, consider adding methods for:
- Checking if coordinates are within bounds
- Generating random coordinates within bounds
- Creating builder pattern for easier instantiation
Example additions:
public boolean isWithinBounds(int x, int z) { return x >= minX && x <= maxX && z >= minZ && z <= maxZ; } public static Builder builder() { return new Builder(); } public static class Builder { private int minX, maxX, minZ, maxZ; public Builder minX(int minX) { this.minX = minX; return this; } // ... other builder methods }eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java (1)
49-57: Enhance method documentation with implementation detailsWhile the documentation is thorough, it could be more helpful by providing additional context about the border shape functionality.
Enhance the documentation with more details:
/** * Asynchronously retrieves a safe random location within the specified world, using radius. * * @param world The world in which to find a random location. - * @param radius The radius around the player to search for a safe location. + * @param radius The radius representer that defines the boundary shape for random location selection. + * This can accommodate various border shapes (e.g., square, circular, or custom polygonal borders). * @param attemptCount The number of attempts to find a safe location. * @return A CompletableFuture containing the random Location that is deemed safe. + * @throws IllegalArgumentException if the radius representer provides invalid boundaries + * @see RandomTeleportRadiusRepresenter */eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (3)
94-97: Refactor to use existing method and avoid code duplicationIn the
getSafeRandomLocation(World world, int radius, int attemptCount)method, you're creating a newRandomTeleportRadiusRepresenterImpland then callinggetSafeRandomLocationwithRandomTeleportType.STATIC_RADIUS. To maintain consistency and reduce code duplication, consider delegating to the newly added overload method that acceptsRandomTeleportRadiusRepresenter.Apply this diff to streamline the method:
public CompletableFuture<Location> getSafeRandomLocation(World world, int radius, int attemptCount) { RandomTeleportRadiusRepresenter radiusRepresenter = new RandomTeleportRadiusRepresenterImpl(-radius, radius, -radius, radius); - return this.getSafeRandomLocation(world, RandomTeleportType.STATIC_RADIUS, radiusRepresenter, attemptCount); + return this.getSafeRandomLocation(world, radiusRepresenter, attemptCount); }This leverages the existing method and improves code maintainability.
112-117: OverloadgetSafeRandomLocationfor clarity and robustnessThe private method
getSafeRandomLocationacceptsradiusandtype, but whentypeisWORLD_BORDER_RADIUS, theradiusis effectively ignored. To improve clarity, overload this method without theradiusparameter for cases where the world border is used.Apply this diff to create an overload without the
radiusparameter:+ private CompletableFuture<Location> getSafeRandomLocation( + World world, + RandomTeleportType type, + int attemptCount + ) { + // Existing logic when using world border radius + } private CompletableFuture<Location> getSafeRandomLocation( World world, RandomTeleportType type, - RandomTeleportRadiusRepresenter radius, int attemptCount ) { // Existing logic when using provided radius }This separation enhances code readability and reduces the possibility of
NullPointerException.
136-137: UseThreadLocalRandomfor better performanceInstead of using a shared
Randominstance, consider usingThreadLocalRandom.current()to generate random coordinates. This avoids potential contention and improves performance in multi-threaded environments.Apply this diff to utilize
ThreadLocalRandom:- int randomX = spawnX + (int) (this.random.nextDouble() * (radius.getMaxX() - radius.getMinX()) + radius.getMinX()); - int randomZ = spawnZ + (int) (this.random.nextDouble() * (radius.getMaxZ() - radius.getMinZ()) + radius.getMinZ()); + int randomX = spawnX + ThreadLocalRandom.current().nextInt(radius.getMinX(), radius.getMaxX() + 1); + int randomZ = spawnZ + ThreadLocalRandom.current().nextInt(radius.getMinZ(), radius.getMaxZ() + 1);This also simplifies the code by directly generating integers within the desired range.
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (2)
159-174: Consider externalizing the list of unsafe blocks to enhance maintainability.Hardcoding a long list of
Materialenums can make future updates and maintenance more challenging. By externalizing this list to a configuration file, server administrators can adjust unsafe blocks without modifying the codebase, increasing flexibility.
176-227: Consider externalizing the list of air blocks to improve flexibility and ease of updates.Similar to the unsafe blocks list, hardcoding the
airBlocksset may lead to difficulties when updates or changes are necessary. Externalizing this list to a configuration file can simplify modifications and allow for dynamic adjustments based on different server environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenter.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(6 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenter.java
🔇 Additional comments (11)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java (2)
16-18: Verify block configurations for safety
The addition of unsafeBlocks() and airBlocks() improves teleportation safety, but we should verify the default configurations.
✅ Verification successful
Block configurations are properly defined and safe
The default configurations for unsafe and air blocks are well-defined and comprehensive:
Unsafe blocks include hazardous materials like:
- Lava, Water, Fire, Cactus
- Magma Block, TNT, Cobweb
- Sweet Berry Bush, Powder Snow
- Bubble Column, Bedrock
Air blocks include safe passable blocks like:
- Air variants (Air, Cave Air, Void Air)
- Vegetation (Grass, Flowers, Vines)
- Rails and Redstone components
- Buttons and other non-solid blocks
The implementation in RandomTeleportServiceImpl properly validates:
- Floor block is solid
- Destination blocks are air blocks
- No unsafe blocks at destination
- Location is within world border
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for default configurations of unsafe and air blocks
rg -A 10 "unsafeBlocks|airBlocks" --type java
# Look for any existing block safety checks in teleport logic
ast-grep --pattern 'Material.$_'
Length of output: 200
Script:
#!/bin/bash
# Search for default configurations or implementations related to unsafe and air blocks
rg -A 10 "unsafeBlocks|airBlocks" --type java
# Look for Material enums or block-related configurations
rg -A 5 "Material\." --type java
# Search for any configuration files that might contain block settings
fd -e yml -e yaml -e json -X cat
Length of output: 75830
20-20: Consider world height limits
The minHeight() method is a good addition for vertical safety, but ensure it respects world-specific height limits.
✅ Verification successful
Based on the code analysis, I can now provide a final response:
World height limits are already respected in the implementation
The minHeight() setting is properly handled in RandomTeleportServiceImpl.getSafeRandomLocation() with additional environment-specific validations:
- For normal worlds, it uses world's highest block Y and respects the configured minimum height
- For nether, it has a special height limit of 127 blocks
- For end dimension, it uses standard height limits
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for height limit validations
rg -A 5 "getMinHeight|getMaxHeight" --type java
# Look for world height checks in teleport logic
ast-grep --pattern 'World.$_ { $$$ getMinHeight $$$ }'
Length of output: 200
Script:
#!/bin/bash
# Search for height-related validations with broader patterns
rg -A 5 "height|Height" --type java
# Look for world or location related validations
rg -A 5 "Location|World" --type java
# Search for the implementation of RandomTeleport
fd -e java "RandomTeleport" --exec cat {}
Length of output: 264761
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java (1)
13-21: Review empty constructor necessity and document its purpose.
The empty constructor could lead to an invalid state with uninitialized fields. If it's required for CDN serialization, this should be documented.
Let's verify if the empty constructor is required:
Consider these improvements:
+ /**
+ * Constructor for CDN serialization.
+ * @implNote Required for CDN library configuration deserialization
+ */
public RandomTeleportRadiusRepresenterImpl() {
+ this(0, 0, 0, 0); // Or appropriate default values
}✅ Verification successful
The search results confirm that the RandomTeleportRadiusRepresenterImpl class is annotated with @Contextual, which is a CDN annotation. This indicates that the class is indeed used for CDN serialization, justifying the presence of the empty constructor.
Empty constructor is required for CDN serialization - needs documentation
The no-args constructor is necessary as the class is marked with @Contextual for CDN deserialization. However, it should be properly documented and initialized with default values to prevent invalid states.
- Add documentation explaining the CDN requirement
- Initialize fields with safe default values in the empty constructor
Example:
/**
* Required for CDN serialization.
* Initializes radius values to safe defaults.
*/
public RandomTeleportRadiusRepresenterImpl() {
this(0, 0, 0, 0);
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CDN usage patterns that might require empty constructor
rg -l "net\.dzikoysk\.cdn" | xargs rg "@Contextual"
Length of output: 9148
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (1)
172-177: Verify that block checks accurately determine safe locations
In the isSafeLocation method, ensure that the sets returned by this.randomTeleportSettings.unsafeBlocks() and airBlocks() are correctly populated and used. Misconfiguration may lead to unsafe teleportation spots.
Run the following script to confirm the contents of unsafeBlocks and airBlocks:
This will display the configured blocks, allowing you to review and ensure they include all necessary materials.
✅ Verification successful
Block checks for safe locations are properly configured
The verification shows that both unsafeBlocks and airBlocks are well-defined with appropriate materials:
unsafeBlocksincludes hazardous materials like lava, water, cactus, fire, cobweb, etc.airBlocksincludes all variants of air blocks and non-solid blocks like grass, vines, and small plants.
The implementation correctly uses these sets to ensure safe teleportation by checking:
- The floor block is not unsafe
- Both the destination block and the block above are air-like blocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'unsafeBlocks' and 'airBlocks' are properly configured.
# Assuming the settings are stored in a configuration file named 'random_teleport_settings.yml'
# Check 'unsafeBlocks'
echo "Unsafe Blocks:"
grep -A 10 'unsafeBlocks:' random_teleport_settings.yml
# Check 'airBlocks'
echo "Air Blocks:"
grep -A 10 'airBlocks:' random_teleport_settings.yml
Length of output: 304
Script:
#!/bin/bash
# Let's first find the configuration file and settings class
fd -e yml -e yaml
fd -e java . | xargs rg -l "RandomTeleportSettings|unsafeBlocks|airBlocks"
# Then examine the implementation
ast-grep --pattern 'class RandomTeleportSettings {
$$$
}'
Length of output: 473
Script:
#!/bin/bash
# Let's examine the RandomTeleportSettings class implementation
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java
# Also check how these settings are initialized in PluginConfiguration
rg -A 10 "unsafeBlocks|airBlocks" eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
Length of output: 1773
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (7)
10-11: LGTM on the new imports for random teleportation features.
The imports for RandomTeleportRadiusRepresenter and RandomTeleportRadiusRepresenterImpl are correctly added, supporting the new random teleportation functionality.
232-234: LGTM on introducing minHeight for random teleportation.
The minHeight parameter adds valuable control over the vertical range for teleportation, enhancing player safety by preventing spawns below a certain altitude.
236-239: LGTM on the overridden randomTeleportRadius() method.
The method correctly returns the randomTeleportStaticRadius, aligning with the updated teleportation logic.
255-258: LGTM on the implementation of unsafeBlocks() method.
The method appropriately provides access to the set of unsafe blocks, complying with the RandomTeleportSettings interface.
260-263: LGTM on the implementation of airBlocks() method.
The method accurately returns the set of air blocks, ensuring the teleportation logic can identify safe landing areas.
265-268: LGTM on the implementation of minHeight() method.
The method correctly supplies the minimum height constraint, enhancing the safety of random teleportation.
147-148:
Verify the initialization parameters of randomTeleportStaticRadius.
The RandomTeleportRadiusRepresenterImpl is initialized with identical values for minX, maxX, minZ, and maxZ all set to 5000. This setup may not define a proper range and might result in unexpected behavior. Typically, min and max values should differ to establish a valid teleportation area.
To ensure the teleportation area is correctly defined, please confirm that these parameters are intended. If 5000 defines the maximum positive coordinate, consider setting minX and minZ to -5000 for a symmetrical range around the origin.
...in/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java
Outdated
Show resolved
Hide resolved
...ore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java (2)
30-48: Add method documentationConsider adding Javadoc to the getter methods to describe the meaning and valid ranges of the coordinates.
+/** + * @return The minimum X coordinate for random teleport boundary + */ @Override public int getMinX() { return this.minX; }
6-49: Consider making the class immutableGiven that this class represents configuration data, consider making it immutable to prevent accidental modifications during runtime. This would involve:
- Making fields private and final
- Removing the default constructor
- Using a builder pattern for CDN serialization
This change would improve thread safety and prevent bugs from unexpected modifications.
Would you like me to provide an example implementation using the builder pattern?
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (2)
136-137: Simplify random coordinate generationThe current implementation of random coordinate generation is verbose and could be simplified for better readability.
- int randomX = spawnX + (int) (this.random.nextDouble() * (radius.getMaxX() - radius.getMinX()) + radius.getMinX()); - int randomZ = spawnZ + (int) (this.random.nextDouble() * (radius.getMaxZ() - radius.getMinZ()) + radius.getMinZ()); + int randomX = spawnX + this.random.nextInt(radius.getMinX(), radius.getMaxX() + 1); + int randomZ = spawnZ + this.random.nextInt(radius.getMinZ(), radius.getMaxZ() + 1);
177-182: Optimize safety checks order for early exitThe order of safety checks could be optimized by checking the least expensive operations first. The world border check should be moved before block-related checks since it doesn't require block state lookups.
private boolean isSafeLocation(Chunk chunk, Location location) { if (location == null || location.getWorld() == null) { return false; } World world = chunk.getWorld(); + WorldBorder worldBorder = world.getWorldBorder(); + if (!worldBorder.isInside(location)) { + return false; + } Block block = world.getBlockAt(location); Block blockAbove = block.getRelative(BlockFace.UP); Block blockFloor = block.getRelative(BlockFace.DOWN); if (this.randomTeleportSettings.unsafeBlocks().contains(blockFloor.getType())) { return false; } Set<Material> airBlocks = this.randomTeleportSettings.airBlocks(); if (!airBlocks.contains(block.getType()) || !airBlocks.contains(blockAbove.getType())) { return false; } if (!blockFloor.getType().isSolid()) { return false; } - WorldBorder worldBorder = world.getWorldBorder(); - if (!worldBorder.isInside(location)) { - return false; - } return switch (world.getEnvironment()) { case NORMAL, THE_END -> true; case NETHER -> location.getY() <= NETHER_MAX_HEIGHT; default -> false; }; }eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (4)
147-148: Consider parameterizing the radius valuesThe hardcoded values (5000) for all directions might not suit all server configurations. Consider making these values configurable through the configuration file.
- public RandomTeleportRadiusRepresenterImpl randomTeleportStaticRadius = - new RandomTeleportRadiusRepresenterImpl(5000, 5000, 5000, 5000); + @Description({ + "# Static radius configuration for each direction", + "# These values are used when randomTeleportType is set to STATIC_RADIUS" + }) + public int northRadius = 5000; + public int southRadius = 5000; + public int eastRadius = 5000; + public int westRadius = 5000; + + @Exclude + public RandomTeleportRadiusRepresenterImpl randomTeleportStaticRadius = + new RandomTeleportRadiusRepresenterImpl(northRadius, southRadius, eastRadius, westRadius);
159-173: Consider adding more unsafe blocksThe unsafe blocks list is good but could be enhanced with additional dangerous blocks:
public Set<Material> unsafeBlocks = EnumSet.of( Material.LAVA, Material.WATER, Material.CACTUS, Material.FIRE, Material.COBWEB, Material.SWEET_BERRY_BUSH, Material.MAGMA_BLOCK, Material.BEDROCK, Material.TNT, Material.SEAGRASS, Material.TALL_SEAGRASS, Material.BUBBLE_COLUMN, - Material.POWDER_SNOW + Material.POWDER_SNOW, + Material.WITHER_ROSE, + Material.POINTED_DRIPSTONE, + Material.CAMPFIRE, + Material.SOUL_CAMPFIRE, + Material.SOUL_FIRE );
231-232: Consider world-specific minimum heightsThe fixed minimum height of 60 might not be suitable for all world types (e.g., void worlds, custom worlds). Consider making this configurable per world.
- public int minHeight = 60; + @Description({ + "# Minimum height configuration per world", + "# Use 'default' key for worlds not specified" + }) + public Map<String, Integer> minHeights = new LinkedHashMap<>() {{ + put("default", 60); + put("void_world", 0); + }};
Line range hint
147-267: Enhance configuration documentationWhile the code implementation is solid, consider adding more detailed documentation for the new configuration options:
- Explain the impact of different radius values
- Document the behavior when unsafe/air blocks are encountered
- Add examples of typical configurations for different server types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(3 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java
🔇 Additional comments (4)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java (3)
8-11: Skipping comment as it's covered by past review
27-28: 🛠️ Refactor suggestion
Document default constructor and add initialization safeguards
The empty constructor, while necessary for CDN serialization, leaves fields uninitialized. This could lead to NullPointerException or invalid states.
+/**
+ * Default constructor for CDN serialization.
+ * Should not be used directly in application code.
+ */
public RandomTeleportRadiusRepresenterImpl() {
+ // Initialize with safe defaults
+ this.minX = 0;
+ this.maxX = 1000;
+ this.minZ = 0;
+ this.maxZ = 1000;
}Likely invalid or redundant comment.
5-6: Verify CDN serialization configuration
The class uses @Contextual for CDN serialization. Ensure that the CDN configuration properly handles this class, especially with public fields.
✅ Verification successful
Let me gather more information about the CDN configuration and usage patterns in the codebase.
Let me check one more aspect about the CDN configuration setup specific to this feature.
Based on the gathered information, I can now provide the final response:
The CDN serialization configuration is properly set up
The class is correctly using @Contextual annotation, and the codebase shows proper CDN configuration with custom composers for similar serializable types. The public fields are a valid pattern in this context as they are used by CDN for serialization/deserialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CDN configuration and other @Contextual usages
rg -l "@Contextual" | xargs rg "cdn\.entity"
# Look for similar implementations
ast-grep --pattern 'class $_ implements RandomTeleportRadiusRepresenter'
Length of output: 2747
Script:
#!/bin/bash
# Look for CDN configuration patterns in similar @Contextual classes
rg -A 5 "@Contextual" eternalcore-core/src/main/java/com/eternalcode/core/configuration/contextual/ConfigItem.java
rg -A 5 "@Contextual" eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/WarpInventoryItem.java
# Check if there's any CDN configuration setup
rg -l "CDN|cdn" | xargs rg "register|compose|serialize"
Length of output: 3665
Script:
#!/bin/bash
# Check for CDN configuration in the random teleport feature
rg -A 10 "RandomTeleport" eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/
# Look for any custom composers related to coordinates or positions
rg -l "Composer" | xargs rg "Position|Coordinate|Location"
Length of output: 35440
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1)
176-226: Review certain air block entries
Some entries in the air blocks list might need reconsideration:
- Rails (RAIL, POWERED_RAIL, etc.) might cause players to slide unexpectedly after teleportation
- Redstone components (COMPARATOR, REPEATER) might be accidentally triggered
- SNOW might cause confusion as it's not truly "air-like"
✅ Verification successful
Based on the code analysis, I can now generate the final response since I have enough context about how the random teleport functionality works. The implementation shows that the safety checks are thorough and the air blocks list is used appropriately.
Rails and redstone components are safe in the air blocks list
The current implementation is actually well-designed and safe because:
- The teleport location requires a solid block below (
if (!blockFloor.getType().isSolid()) { return false; }) - Both the target block and block above must be in the air blocks list for safety
- The location is offset by +0.5 in X/Z and +1 in Y (
add(0.5, 1, 0.5)), ensuring the player stands centered above the floor block - Additional checks for unsafe blocks, world border, and environment-specific conditions are performed
The current configuration is safe because:
- Rails and redstone components won't cause sliding as the player is teleported 1 block above them
- The player can't accidentally trigger components since they're teleported centered and elevated
- Snow is appropriately categorized as it's a non-solid, passable block that doesn't impede movement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these blocks are commonly considered as air blocks in other Minecraft plugins
rg -l "(?i)(rail|comparator|repeater|snow).+(air|passable)" --type java
Length of output: 73
Script:
#!/bin/bash
# Let's search for usages of this airBlocks configuration to understand its purpose
rg -A 5 "airBlocks" --type java
# Also search for any teleport-related code that might use these blocks
ast-grep --pattern 'teleport($$$)'
Length of output: 3521
Script:
#!/bin/bash
# Let's examine the random teleport implementation to understand the safety checks
rg -A 10 "RandomTeleportServiceImpl" --type java
# Also check for any block-related safety checks
ast-grep --pattern 'isSafe($$$)'
Length of output: 3258
Script:
#!/bin/bash
# Let's check the specific location validation logic in the random teleport service
rg -B 5 -A 15 "isSafeLocation|checkLocation|validateLocation" --type java
# Also look at the actual teleport execution code
rg -B 5 -A 15 "randomTeleport" --type java
Length of output: 81835
...in/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Outdated
Show resolved
Hide resolved
…ndomteleport/RandomTeleportRadiusRepresenterImpl.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
...ore/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
Outdated
Show resolved
Hide resolved
P1otrulla
left a comment
P1otrulla
left a comment
There was a problem hiding this comment.
Good job, I think Mike has good idea and you can seperate rtp config as other static class in main configuration
…enterImpl#of` methods.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
…ndomteleport/RandomTeleportRadiusRepresenterImpl.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ndomteleport/RandomTeleportServiceImpl.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ndomteleport/RandomTeleportSettingsImpl.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (1)
134-135: Extract coordinate calculation logicThe random coordinate calculation logic is complex and would benefit from extraction into a dedicated method for better readability and maintainability.
+ private int calculateRandomCoordinate(int spawnCoord, double min, double max) { + return spawnCoord + (int) (this.random.nextDouble() * (max - min) + min); + } - int randomX = spawnX + (int) (this.random.nextDouble() * (radius.getMaxX() - radius.getMinX()) + radius.getMinX()); - int randomZ = spawnZ + (int) (this.random.nextDouble() * (radius.getMaxZ() - radius.getMinZ()) + radius.getMinZ()); + int randomX = calculateRandomCoordinate(spawnX, radius.getMinX(), radius.getMaxX()); + int randomZ = calculateRandomCoordinate(spawnZ, radius.getMinZ(), radius.getMaxZ());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(6 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
🔇 Additional comments (4)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (4)
37-42: LGTM! Constructor formatting improves readability
The constructor parameters are now properly aligned and formatted, making the code more readable while maintaining the same functionality.
145-153: LGTM! Proper height bounds checking
The addition of both minimum and maximum height checks ensures the generated location stays within valid world boundaries. This is a good improvement for preventing out-of-bounds teleportation.
175-180: LGTM! Improved configurability of safety checks
The safety checks now properly utilize configurable block sets from settings instead of hardcoded values, making the system more flexible and maintainable.
107-107: 🛠️ Refactor suggestion
Replace null with Optional or dedicated constant
Using null as a parameter value is a code smell. Consider using Optional or a dedicated constant to represent the world border radius case.
- return this.getSafeRandomLocation(world, RandomTeleportType.WORLD_BORDER_RADIUS, null, attemptCount);
+ return this.getSafeRandomLocation(world, RandomTeleportType.WORLD_BORDER_RADIUS, RandomTeleportRadiusRepresenter.worldBorder(), attemptCount);Likely invalid or redundant comment.
...in/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java
Outdated
Show resolved
Hide resolved
...in/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Show resolved
Hide resolved
WalkthroughThe changes involve significant modifications to the random teleportation feature within the codebase. The Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java (1)
16-18: Consider documenting block set purposesThe new methods
unsafeBlocks()andairBlocks()would benefit from JavaDoc comments explaining:
- What constitutes an "unsafe" block
- How these sets affect teleportation behavior
- Whether empty sets are valid
+/** + * @return Set of blocks considered unsafe for teleportation landing + */ Set<Material> unsafeBlocks(); +/** + * @return Set of blocks considered as air for teleportation purposes + */ Set<Material> airBlocks();eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java (2)
5-5: Document the purpose of @contextual annotationThe
@Contextualannotation suggests this class is used for configuration deserialization. Consider adding a class-level JavaDoc comment explaining how this class interacts with the configuration system.
20-21: Document the default constructor's purposeThe default constructor is likely required for configuration deserialization but leaves the object in an invalid state. Add JavaDoc explaining its purpose and any constraints.
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java (2)
41-49: Add 'since' attribute to @deprecated annotationThe deprecation notice in the Javadoc is well-documented with clear migration guidance. However, the @deprecated annotation should include the 'since' attribute to help track when this deprecation was introduced.
-@Deprecated(forRemoval = true) +@Deprecated(since = "1.0.0", forRemoval = true)
52-60: Enhance Javadoc for the new radius parameterThe Javadoc should better explain the purpose and advantages of using RandomTeleportRadiusRepresenter.
/** * Asynchronously retrieves a safe random location within the specified world, using radius. * * @param world The world in which to find a random location. - * @param radius The radius around the player to search for a safe location. + * @param radius The radius representer that defines the boundaries for teleportation. + * This allows for custom border shapes beyond traditional square boundaries. * @param attemptCount The number of attempts to find a safe location. * @return A CompletableFuture containing the random Location that is deemed safe. */eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (6)
14-20: Improve description grammarThe description contains a grammatical error in the STATIC_RADIUS explanation.
@Description({ "# Type of random teleportation,", "# WORLD_BORDER_RADIUS - radius based on the world-border size.", - "# STATIC_RADIUS - radius based on the manually value." + "# STATIC_RADIUS - radius based on a manually configured value." })
21-30: Enhance configuration documentation
- The description could better explain the significance of the default 5000-block radius.
- The compatibility note should be part of the official description.
@Description({ "# Radius of random teleportation, this uses for starting point spawn via /setworldspawn.", "# If you want to use a static radius, set the type to STATIC_RADIUS and set the radius here.", - "# If you using WORLD_BORDER_RADIUS, this value will be ignored." + "# If you are using WORLD_BORDER_RADIUS, this value will be ignored.", + "# Default value: 5000 blocks in each direction from spawn", + "# Note: Field name must remain as 'randomTeleportStaticRadius' for compatibility with older configs" })
31-35: Clarify world configuration descriptionThe description for
randomTeleportWorldcould be more specific about the expected format and default behavior.- @Description("# Teleport to a specific world, if left empty it will teleport to the player's current world") + @Description({ + "# The target world for random teleportation", + "# Format: world name as defined in server.properties or multiverse", + "# Default: 'world'", + "# Set to empty string to use player's current world" + })
37-53: Consider additional unsafe blocksThe unsafe blocks list is comprehensive but could include additional dangerous blocks from recent Minecraft versions.
Consider adding these blocks to
unsafeBlocks:public Set<Material> unsafeBlocks = EnumSet.of( // ... existing blocks ... + Material.POINTED_DRIPSTONE, // Added in 1.17, causes damage when falling + Material.BIG_DRIPLEAF, // Added in 1.17, can cause falling + Material.CAMPFIRE, // Causes fire damage + Material.SOUL_CAMPFIRE // Causes fire damage );
55-106: Optimize air blocks configurationThe air blocks configuration could be improved in several ways:
- Legacy materials might not be necessary for modern Minecraft versions
- Button types could be grouped more efficiently using pattern matching
Consider these improvements:
public Set<Material> airBlocks = EnumSet.of( Material.AIR, Material.CAVE_AIR, Material.VOID_AIR, - Material.LEGACY_AIR, - Material.LEGACY_LONG_GRASS, - Material.LEGACY_DEAD_BUSH, // ... other blocks ... ); // Consider moving this to a separate method for better maintainability: private static Set<Material> getButtonMaterials() { return Arrays.stream(Material.values()) .filter(material -> material.name().endsWith("_BUTTON")) .collect(Collectors.toCollection(() -> EnumSet.noneOf(Material.class))); }
108-113: Enhance minimum height descriptionThe minimum height configuration could benefit from a more detailed description.
- @Description("# Minimum height for random teleportation") + @Description({ + "# Minimum height (Y-coordinate) for random teleportation", + "# This prevents teleporting players too close to bedrock level", + "# Default: 60 (slightly above sea level in most biomes)", + "# Range: Should be between world's min_height and max_height" + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenter.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(6 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenter.java
🔇 Additional comments (23)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java (3)
3-5: LGTM: Import changes are appropriate
The new imports for Set and Material are necessary for the added methods.
8-8: Verify the migration path for existing implementations
The return type change from int to RandomTeleportRadiusRepresenter is a breaking change that will require updates in all implementing classes.
✅ Verification successful
Let me check the implementation and usage to verify the migration completeness.
The return type change is properly handled across the codebase
The implementation has been properly migrated:
RandomTeleportSettingsImplcorrectly implements the new return type withrandomTeleportStaticRadiusRandomTeleportServiceImplhandles the newRandomTeleportRadiusRepresentertype appropriately in its methods- A comment in the implementation explains the field naming choice for backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of RandomTeleportSettings to ensure they're updated
ast-grep --pattern 'class $_ implements $$$RandomTeleportSettings$$$'
# Find all usages of randomTeleportRadius() to ensure they handle the new return type
rg -l 'randomTeleportRadius\(\)'
Length of output: 422
Script:
#!/bin/bash
# Check the implementation in RandomTeleportSettingsImpl
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
# Check how the return type is used in RandomTeleportServiceImpl
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Length of output: 12918
20-20: Validate minHeight constraints
The minHeight() method should be documented with:
- Valid range of values
- Relationship with world height limits
- Interaction with randomTeleportRadius
✅ Verification successful
Let me check the implementation of RandomTeleportServiceImpl.java to understand how minHeight is used.
Add minHeight constraints documentation
The implementation shows that minHeight is used as a lower bound for random teleport locations with the following constraints:
- Must be greater than or equal to 0 (world minimum)
- Must be less than world's maximum height
- In Nether dimension, effectively capped at 127
- Used as fallback when generated Y coordinate is too low
Add the following documentation to the minHeight() method:
/**
* @return The minimum height for random teleport locations
* Must be >= 0 and less than world's max height
* For Nether dimension, effective maximum is 127
* Used as fallback when generated Y coordinate is below this value
*/🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for world height limit references to ensure consistency
rg -l 'getMinHeight|getMaxHeight'
# Look for existing height validation logic
ast-grep --pattern 'if ($_ < $_) {
$$$
}'
Length of output: 4114
Script:
#!/bin/bash
# Check RandomTeleportServiceImpl implementation
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Length of output: 7985
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java (1)
50-68: LGTM! Getter methods are properly implemented.
The getter methods correctly implement the interface contract and provide access to the coordinate boundaries.
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java (1)
41-61: LGTM! Well-structured interface evolution
The changes effectively support the PR's objective of expanding random teleport configuration for different border shapes. The interface maintains backward compatibility through proper deprecation while introducing a more flexible approach using RandomTeleportRadiusRepresenter.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (2)
1-13: LGTM!
The class structure and imports are well-organized, with clear interface implementations and appropriate use of CDN annotations.
114-153: LGTM!
The interface implementations are correct and properly protect mutable collections using Collections.unmodifiableSet().
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (2)
11-11: LGTM!
The import statement is correctly placed with other feature-related imports.
127-127: Verify backward compatibility with existing configurations.
The field declaration looks good and aligns with the PR objectives to expand teleport configuration for non-square border shapes.
Let's verify the backward compatibility:
✅ Verification successful
Based on the shell script results, I can now generate the final response as there's sufficient information to verify backward compatibility:
No backward compatibility concerns with the RandomTeleport configuration changes
The changes maintain backward compatibility through:
- Preserving the existing field name
randomTeleportinPluginConfiguration - Including a compatibility note in the code explaining why the radius field is named
randomTeleportStaticRadius - Maintaining the existing enum values in
RandomTeleportType - Supporting both square and non-square border shapes through the new
RandomTeleportRadiusRepresenterinterface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in configuration files
# Search for any existing config.yml files in test resources that might contain old configuration
fd 'config.yml' -t f | while read -r config; do
echo "Checking $config for old RandomTeleport settings..."
rg -A 5 'RandomTeleport|randomTeleport' "$config"
done
# Search for any tests that might be affected by this change
rg -t java 'RandomTeleport' -l | while read -r file; do
echo "Checking $file for potential test updates needed..."
rg -A 5 'RandomTeleport' "$file"
done
Length of output: 27061
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (14)
3-3: New import of PositionAdapter is appropriate
The addition of PositionAdapter import allows for effective conversion of positions, enhancing code functionality.
11-12: Added imports for Random and Set are necessary
The imports for java.util.Random and java.util.Set are required for the random generation logic and managing collections.
37-42: Constructor parameters formatted for improved readability
Reformatting the constructor parameters enhances code readability without altering functionality.
57-58: Grammatical correction in exception message
The exception message now correctly states "does not exist!", improving clarity.
87-87: Retrieve radius from settings in getSafeRandomLocation
Assigning radius from randomTeleportSettings allows for flexible radius configuration.
94-95: Overloaded getSafeRandomLocation method with radius parameter
The method now accepts a radius, enhancing modularity and reusability.
97-101: Introduced overload accepting RandomTeleportRadiusRepresenter
Adding this overload increases flexibility by allowing different radius representations.
110-115: New private method for centralized teleportation logic
Creating a private method that handles teleportation with different radius types improves code organization and maintainability.
122-123: Corrected border radius calculation to prevent truncation errors
Casting after division ensures accurate calculation of borderRadius, maintaining precision.
145-154: Added bounds checking for randomY
Implementing checks for minHeight and maxHeight ensures randomY stays within valid world height limits, enhancing stability.
161-161: Recursive call with decremented attempt count
This ensures the method will terminate after the specified number of attempts, preventing infinite recursion.
175-175: Use configurable unsafeBlocks instead of hardcoded values
Refactoring to use randomTeleportSettings.unsafeBlocks() increases flexibility and allows for dynamic configuration.
179-180: Use configurable airBlocks instead of hardcoded values
Utilizing randomTeleportSettings.airBlocks() enhances configurability and adapts to different game environments.
107-107: 🛠️ Refactor suggestion
Avoid passing null for the radius parameter
Passing null can lead to NullPointerException and reduces code clarity.
Consider using a dedicated constant or factory method to represent the world border radius. This enhances readability and prevents potential null issues.
Apply this diff to adjust the method call:
- return this.getSafeRandomLocation(world, RandomTeleportType.WORLD_BORDER_RADIUS, null, attemptCount);
+ return this.getSafeRandomLocation(world, RandomTeleportType.WORLD_BORDER_RADIUS, RandomTeleportRadiusRepresenter.worldBorder(), attemptCount);Likely invalid or redundant comment.
...in/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusRepresenterImpl.java
Outdated
Show resolved
Hide resolved
...ore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (11)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPlaceholders.java (2)
6-6: Consider making the class package-private constructor explicitThe class is correctly marked as
finalsince it only contains static members. However, to make the utility nature of this class more explicit, consider adding a private constructor.final class RandomTeleportPlaceholders { + private RandomTeleportPlaceholders() { + throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); + }
8-14: Consider using a decimal format for coordinatesFor better readability and consistency, consider using a decimal format for coordinate values.
+ private static final DecimalFormat COORDINATE_FORMAT = new DecimalFormat("#,##0"); + static final Placeholders<Player> PLACEHOLDERS = Placeholders.<Player>builder() .with("{PLAYER}", Player::getName) .with("{WORLD}", player -> player.getWorld().getName()) - .with("{X}", player -> String.valueOf(player.getLocation().getBlockX())) - .with("{Y}", player -> String.valueOf(player.getLocation().getBlockY())) - .with("{Z}", player -> String.valueOf(player.getLocation().getBlockZ())) + .with("{X}", player -> COORDINATE_FORMAT.format(player.getLocation().getBlockX())) + .with("{Y}", player -> COORDINATE_FORMAT.format(player.getLocation().getBlockY())) + .with("{Z}", player -> COORDINATE_FORMAT.format(player.getLocation().getBlockZ())) .build();eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (2)
Line range hint
63-77: Critical: Missing success feedback for teleportationThe removal of success notifications creates an inconsistent user experience. While failures are communicated to users, successful teleports are now silent. This could lead to user confusion, especially in cases where the teleport might not be immediately obvious (e.g., similar-looking terrain).
Consider adding back success notifications. Here's a suggested implementation:
this.randomTeleportService.teleport(player) .whenCompleteAsync((result, error) -> { if (error != null || !result.success()) { this.handleTeleportFailure(player); return; } + this.noticeService.create() + .notice(translation -> translation.randomTeleport().randomTeleportSuccess()) + .player(player.getUniqueId()) + .send(); });Also applies to: 89-105
Line range hint
124-146: Optimize permission check in hasRandomTeleportDelayThe
isOp()check is redundant as permission plugins typically include OP status in permission checks. Additionally, the method could be optimized by checking permissions before accessing the delay system.Consider this optimization:
private boolean hasRandomTeleportDelay(Player player) { - UUID uniqueId = player.getUniqueId(); + if (player.hasPermission(RTP_BYPASS_PERMISSION)) { + return false; + } - if (player.hasPermission(RTP_BYPASS_PERMISSION) || player.isOp()) { - return false; - } + UUID uniqueId = player.getUniqueId(); if (this.delay.hasDelay(uniqueId)) { Duration time = this.delay.getDurationToExpire(uniqueId);eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (2)
15-16: Enhance configuration descriptionConsider expanding the description to include:
- The purpose of this waiting period
- Whether this is a cooldown or loading time
- The acceptable range of values
- @Description("# Time to wait for the random teleportation") + @Description({ + "# Time to wait before executing the random teleportation", + "# This delay allows for loading chunks and cancelling if needed", + "# Recommended range: 3-10 seconds" + })
47-62: Consider adding more unsafe blocksThe unsafe blocks list could be expanded to include:
- Campfire and Soul Campfire
- Pointed Dripstone
- Deep Water (via depth check)
- Lava Cauldron
- Powdered Snow Cauldron
public Set<Material> randomTeleportUnsafeBlocks = EnumSet.of( Material.LAVA, Material.WATER, Material.CACTUS, Material.FIRE, + Material.CAMPFIRE, + Material.SOUL_CAMPFIRE, + Material.POINTED_DRIPSTONE, + Material.LAVA_CAULDRON, + Material.POWDER_SNOW_CAULDRON, // ... rest of the blocks );eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1)
245-250: LGTM! Consider adding a sound notification for consistency.The enhanced notice implementation using the builder pattern with multiple components (chat, title, subtitle) provides better user feedback during the random teleport process. This aligns well with the PR's objective of expanding Random Teleport configuration.
Consider adding a sound notification for consistency with other teleport notices in the codebase:
public Notice randomTeleportStarted = Notice.builder() .chat("<green>► <white>Rozpoczynanie procesu losowania lokalizacji...") .title("<green>Losowy teleport") .subtitle("<white>Wyszukiwanie lokalizacji, proszę czekać...") + .sound(Sound.ENTITY_ENDERMAN_TELEPORT, 1.0f, 1.0f) .build();eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java (1)
23-23: Consider making the classpublicThe
RandomTeleportTaskServiceclass is package-private. If it's intended to be used outside its package, consider declaring it aspublicfor broader accessibility.Apply this diff to change the class visibility:
-class RandomTeleportTaskService { +public class RandomTeleportTaskService {eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java (3)
73-73: Clarify the adjustment ofmaxHeightSubtracting
1fromheightRange.getMaxY()may lead to confusion regarding whethermaxYis intended to be inclusive or exclusive. IfmaxYis meant to be inclusive, subtracting1makes it exclusive, potentially causing off-by-one errors in height calculations. Please verify whether subtracting1is necessary, and consider adding comments or adjusting the code to make this clear.
39-84: Consider refactoring recursion to iteration to prevent potential stack overflowThe
getSafeRandomLocationmethod uses recursion to retry finding a safe location when the current attempt fails. IfattemptCountis large, this could lead to aStackOverflowErrordue to deep recursion. Consider refactoring the method to use a loop instead of recursion to improve robustness and prevent potential stack issues.
88-89: Avoid unnecessary null checks afterlocationvalidationSince you've already checked that
locationis notnullin line 87, the subsequent check forlocation.getWorld() == nullis redundant iflocationis guaranteed to be non-null. Consider simplifying the condition to improve code readability.Apply the following simplification:
-if (location == null || location.getWorld() == null) { +if (location == null) { return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPermissionConstant.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPlaceholders.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPermissionConstant.java
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java
🔇 Additional comments (9)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPlaceholders.java (1)
8-14: Verify the usage of these placeholders in messages
Let's verify how these placeholders are used in the message configuration to ensure they match the expected format.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the placeholders usage:
Placeholders are correctly used in the codebase
The placeholders defined in RandomTeleportPlaceholders are properly utilized in the codebase:
- They are imported and used in
RandomTeleportCommandandRandomTeleportTaskService - The placeholders are correctly passed to the translation system using
PLACEHOLDERS.toFormatter(player) - The messages are handled through a translation system rather than direct YAML configurations, which is a more robust approach
- The code shows consistent usage in multiple locations:
- When teleporting players to random locations
- When notifying about teleport status
- When handling teleport failures
- When teleporting other players
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these placeholders in message configurations
rg -l "{PLAYER}|{WORLD}|{X}|{Y}|{Z}" --type yaml
Length of output: 154
Script:
#!/bin/bash
# Search for usage of these placeholders in message configurations, escaping the curly braces
rg -l "\{PLAYER\}|\{WORLD\}|\{X\}|\{Y\}|\{Z\}" --type yaml
Length of output: 209
Script:
#!/bin/bash
# Also search in the Java files to understand how these placeholders are used
rg -A 5 "PLACEHOLDERS" eternalcore-core/src/main/java/
Length of output: 5484
Script:
#!/bin/bash
# Look for message configuration files
fd -e yml -e yaml
Length of output: 19
Script:
#!/bin/bash
# Look for configuration files in a more specific location
fd -e yml -e yaml . eternalcore-core/src/main/resources/
# Also try to find any references to translations or messages in Java code
rg -A 3 "translation.*randomTeleport" eternalcore-core/src/main/java/
Length of output: 3743
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (4)
5-5: LGTM! Clean import and field additions
The new imports and field declarations align well with the service's async functionality and dependency injection pattern.
Also applies to: 16-17
20-25: LGTM! Well-structured constructor injection
The constructor properly initializes all required dependencies through dependency injection.
Also applies to: 28-29
64-70: Add @deprecated annotation to legacy method
This method has been superseded by the new RandomTeleportRadiusRepresenter version and should be properly marked as deprecated.
Apply this diff to properly mark the method as deprecated:
+ @Deprecated(since = "1.0", forRemoval = true)
@Override
public CompletableFuture<Location> getSafeRandomLocation(World world, int radius, int attemptCount) {87-94: 🛠️ Refactor suggestion
Replace null radius with proper representation
Passing null as the radius parameter is a code smell that was previously identified. This should be replaced with a proper radius representation.
Apply this diff to use a proper radius representation:
return this.safeLocationService.getSafeRandomLocation(
world,
RandomTeleportType.WORLD_BORDER_RADIUS,
- null,
+ RandomTeleportRadiusRepresenter.worldBorder(),
attemptCount
);eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (2)
3-4: LGTM! Good refactoring of constants.
Moving constants to dedicated constant classes and using static imports improves maintainability and follows the DRY principle.
Line range hint 63-77: Verify integration with new RandomTeleportRadiusRepresenter
The command implementation should be updated to utilize the new radius representation system mentioned in the PR description.
Let's verify the integration:
✅ Verification successful
The command implementation correctly integrates with the radius representation system
The code shows proper integration with the new radius representation system:
- The
RandomTeleportServiceImplusesRandomTeleportRadiusRepresenterfrom settings - The configuration (
RandomTeleportSettingsImpl) properly defines and manages the radius representation throughrandomTeleportStaticRadius - The command delegates to the service which handles both world border and static radius types correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the RandomTeleportService implementation uses the new radius system
ast-grep --pattern 'class $_ implements RandomTeleportService {
$$$
teleport($player) {
$$$
}
$$$
}'
# Check for any test coverage of the new functionality
rg -l "test.*RandomTeleport.*" -g "**/*Test.java"
Length of output: 166
Script:
#!/bin/bash
# Let's try a broader search to find the service implementation and radius representation
rg -A 10 "RandomTeleportService"
# Search for radius-related code in random teleport files
rg -A 5 "radius" -g "*RandomTeleport*.java"
# Find all random teleport related files
fd "RandomTeleport.*\.java$"
Length of output: 42858
Script:
#!/bin/bash
# Let's check the implementation of the command to verify radius usage
rg -A 10 "execute.*Player" eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java
# Check the service implementation for radius handling
rg -A 10 "teleport.*Player" eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
# Check the configuration setup for radius
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Length of output: 7776
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (1)
71-111: Review air blocks list
-
Consider removing legacy materials unless supporting very old versions:
- LEGACY_AIR
- LEGACY_LONG_GRASS
- LEGACY_DEAD_BUSH
-
Consider adding modern passable blocks:
- SMALL_DRIPLEAF
- SPORE_BLOSSOM
- SCULK_VEIN
- GLOW_LICHEN
✅ Verification successful
Let me gather more information about the server version compatibility by checking the plugin.yml and any other configuration files.
Let me try a different approach to find version-related information in the build files.
Based on the build files, I can now provide a final response since I found that the plugin supports Minecraft versions from 1.17.x to 1.21.x and has an API version of 1.13+.
Legacy materials can be safely removed
The legacy materials in the air blocks list can be safely removed as the plugin's minimum supported version is 1.13:
- LEGACY_AIR
- LEGACY_LONG_GRASS
- LEGACY_DEAD_BUSH
Your suggestion to add modern passable blocks is valid:
- SMALL_DRIPLEAF
- SPORE_BLOSSOM
- SCULK_VEIN
- GLOW_LICHEN
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check minimum supported Minecraft version
rg "getMinecraftVersion|MinecraftVersion|VERSION" --type java
Length of output: 1225
Script:
#!/bin/bash
# Check plugin.yml for version compatibility
fd "plugin.yml" --exec cat {}
# Check for any other version-related configurations
rg "api-version|minecraft.version" --type yaml
Length of output: 229
Script:
#!/bin/bash
# Check build files for version info
fd "build.gradle" --exec cat {}
fd "pom.xml" --exec cat {}
# Look for any version constants or configurations
rg "minecraft.*version|spigot.*version|paper.*version" --type-add 'props:*.properties' --type props --type gradle --type xml
Length of output: 9779
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
244-249: LGTM!
The use of Notice.builder() to enhance the randomTeleportStarted notification with a title and subtitle is appropriate and improves the user experience.
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPlaceholders.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java
Show resolved
Hide resolved
...main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java
Outdated
Show resolved
Hide resolved
…ndomteleport/RandomTeleportTaskService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ndomteleport/RandomTeleportSafeLocationService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (14)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/SimpleRandomTeleportRadius.java (2)
3-12: Consider enhancing validation and adding utility methodsWhile the basic validation is good, consider adding these improvements:
Additional validations:
- Check for negative values
- Validate minimum area size
- Add maximum bounds validation to prevent excessive values
Utility methods:
- Add methods to calculate area size
- Add methods to check if a location is within bounds
- Add factory methods for common shapes (square, rectangle)
Here's a suggested enhancement:
record SimpleRandomTeleportRadius(int minX, int maxX, int minZ, int maxZ) implements RandomTeleportRadius { public SimpleRandomTeleportRadius { if (minX > maxX) { throw new IllegalArgumentException("minX cannot be greater than maxX"); } if (minZ > maxZ) { throw new IllegalArgumentException("minZ cannot be greater than maxZ"); } + if (minX < 0 || minZ < 0) { + throw new IllegalArgumentException("Minimum coordinates cannot be negative"); + } + // Prevent excessive values that might cause performance issues + if (maxX > 30000000 || maxZ > 30000000) { + throw new IllegalArgumentException("Maximum coordinates exceed allowed limits"); + } } + + public boolean isLocationInBounds(int x, int z) { + return x >= minX && x <= maxX && z >= minZ && z <= maxZ; + } + + public long getArea() { + return (long) (maxX - minX) * (maxZ - minZ); + } + + public static SimpleRandomTeleportRadius square(int size) { + return new SimpleRandomTeleportRadius(-size, size, -size, size); + } }
1-1: Consider adding package documentationSince this is a new feature for handling different border shapes, consider adding package-level documentation to explain the purpose and usage of these classes.
Add a
package-info.javafile with documentation about the random teleport feature and its configuration options.eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadius.java (2)
3-8: Add JavaDoc documentation to interface and methods.The interface and its methods lack documentation explaining their purpose and contract. This makes it harder for other developers to understand and implement the interface correctly.
Consider adding documentation like this:
+/** + * Represents the boundaries for random teleportation in a world. + * The boundaries are defined by minimum and maximum coordinates on X and Z axes. + */ public interface RandomTeleportRadius { + /** @return The maximum Z coordinate for teleportation */ int maxZ(); + /** @return The minimum Z coordinate for teleportation */ int minZ(); + /** @return The maximum X coordinate for teleportation */ int maxX(); + /** @return The minimum X coordinate for teleportation */ int minX();
1-17: Consider adding methods for radius calculation and boundary checking.The interface could benefit from utility methods to calculate the effective radius and check if coordinates are within bounds.
Consider adding these methods:
public interface RandomTeleportRadius { // ... existing methods ... + /** + * Checks if the given coordinates are within the boundaries. + * @return true if coordinates are within bounds + */ + default boolean isWithinBounds(int x, int z) { + return x >= minX() && x <= maxX() + && z >= minZ() && z <= maxZ(); + } + + /** + * @return The effective radius (maximum distance from origin) + */ + default int getEffectiveRadius() { + return Math.max( + Math.max(Math.abs(maxX()), Math.abs(minX())), + Math.max(Math.abs(maxZ()), Math.abs(minZ())) + ); + } }eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportHeightRange.java (4)
5-6: Add class-level documentation.Consider adding Javadoc to describe the purpose and usage of this class, including the meaning of the
@Contextualannotation in this context.@Contextual +/** + * Represents a vertical height range for random teleportation. + * The range is defined by minimum and maximum Y coordinates. + * This class is marked as @Contextual for CDN configuration serialization. + */ class RandomTeleportHeightRange {
19-21: Add documentation and make factory method public.The static factory method should be public and documented to encourage its use over constructors.
- static RandomTeleportHeightRange of(int minY, int maxY) { + /** + * Creates a new height range with the specified minimum and maximum Y coordinates. + * + * @param minY the minimum Y coordinate (inclusive) + * @param maxY the maximum Y coordinate (inclusive) + * @return a new RandomTeleportHeightRange instance + * @throws IllegalArgumentException if minY > maxY or coordinates are outside valid range + */ + public static RandomTeleportHeightRange of(int minY, int maxY) { return new RandomTeleportHeightRange(minY, maxY); }
23-29: Make getters public and add documentation.The getter methods should be public since they're the primary interface for accessing the range values.
- int getMinY() { + /** + * @return the minimum Y coordinate (inclusive) + */ + public int getMinY() { return this.minY; } - int getMaxY() { + /** + * @return the maximum Y coordinate (inclusive) + */ + public int getMaxY() { return this.maxY; }
1-30: Consider implementing value object methods.Since this is a value object, consider implementing
equals(),hashCode(), andtoString()methods.+ @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof RandomTeleportHeightRange that)) return false; + return minY == that.minY && maxY == that.maxY; + } + + @Override + public int hashCode() { + return Objects.hash(minY, maxY); + } + + @Override + public String toString() { + return "RandomTeleportHeightRange{" + + "minY=" + minY + + ", maxY=" + maxY + + '}'; + }eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusConfig.java (2)
1-6: Consider making the class public and adding documentation.The class is currently package-private which might be too restrictive. Since it implements a public interface and represents configuration, it should probably be public. Also, consider adding JavaDoc to document the purpose and usage of this class.
+/** + * Configuration implementation of RandomTeleportRadius that defines the boundaries + * for random teleportation in X and Z coordinates. + */ +public class RandomTeleportRadiusConfig implements RandomTeleportRadius {
23-41: Add JavaDoc to interface method implementations.The implementations are correct, but adding JavaDoc would improve code documentation and maintainability. Consider documenting the return values and their meaning.
+ /** + * @return The minimum X coordinate of the teleport radius + */ @Override public int minX() { return this.minX; }eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java (1)
Line range hint
1-57: Consider interface segregation for teleportation concernsThe interface currently handles both teleportation and location generation responsibilities. Consider splitting these into separate interfaces following the Interface Segregation Principle:
RandomTeleportServicefor teleportation operationsSafeLocationProviderfor location generation operationsThis would make the API more focused and easier to implement for different use cases.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (2)
Line range hint
69-91: Ensure consistent error handling in executeOther.The error handling between executeSelf and executeOther is inconsistent. In executeOther, the delay is also marked before confirming successful teleportation.
Apply similar improvements to executeOther:
void executeOther(@Context Viewer sender, @Arg Player player) { UUID uuid = player.getUniqueId(); if (this.hasRandomTeleportDelay(player)) { return; } this.noticeService.create() .notice(translation -> translation.randomTeleport().randomTeleportStarted()) .player(player.getUniqueId()) .send(); this.randomTeleportService.teleport(player) .whenCompleteAsync((result, error) -> { if (error != null || !result.success()) { this.handleTeleportFailure(player); return; } + this.delay.markDelay(uuid, this.config.randomTeleport.delay()); this.handleAdminTeleport(sender, player); + this.noticeService.create() + .notice(translation -> translation.randomTeleport().randomTeleportSuccess()) + .player(player.getUniqueId()) + .send(); }); - this.delay.markDelay(uuid, this.config.randomTeleport.delay()); }
Line range hint
114-119: Review permission bypass logic.The
isOp()check might bypass the configured permission system. Consider removing it to maintain consistent permission management through the permission system.- if (player.hasPermission(RTP_BYPASS_PERMISSION) || player.isOp()) { + if (player.hasPermission(RTP_BYPASS_PERMISSION)) { return false; }eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (1)
47-62: Consider adding more unsafe blocksThe unsafe blocks list could be enhanced with additional hazardous materials:
CAMPFIREandSOUL_CAMPFIRE(cause fire damage)POINTED_DRIPSTONE(causes fall damage)SCAFFOLDING(can cause fall damage)POWDER_SNOW_CAULDRON(can freeze players)public Set<Material> randomTeleportUnsafeBlocks = EnumSet.of( Material.LAVA, Material.WATER, Material.CACTUS, Material.FIRE, Material.COBWEB, Material.SWEET_BERRY_BUSH, Material.MAGMA_BLOCK, Material.BEDROCK, Material.TNT, Material.SEAGRASS, Material.TALL_SEAGRASS, Material.BUBBLE_COLUMN, Material.POWDER_SNOW, - Material.WITHER_ROSE + Material.WITHER_ROSE, + Material.CAMPFIRE, + Material.SOUL_CAMPFIRE, + Material.POINTED_DRIPSTONE, + Material.SCAFFOLDING, + Material.POWDER_SNOW_CAULDRON );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadius.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResult.java(1 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java(3 hunks)eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/SimpleRandomTeleportRadius.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportHeightRange.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusConfig.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportType.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResult.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportType.java
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java
🔇 Additional comments (11)
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/SimpleRandomTeleportRadius.java (1)
3-3: Verify interface contract implementation
The class implements RandomTeleportRadius, but we should verify that all required interface methods are properly implemented.
✅ Verification successful
Interface contract is properly implemented
The SimpleRandomTeleportRadius record automatically implements all required methods of the RandomTeleportRadius interface. The interface requires maxZ(), minZ(), maxX(), and minX() methods, which perfectly match the record's components. Java records automatically generate these accessor methods with the exact names needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the interface definition and its usage
ast-grep --pattern 'interface RandomTeleportRadius {
$$$
}'
Length of output: 63
Script:
#!/bin/bash
# Let's try to find the interface definition first
fd -e java RandomTeleportRadius.java
# Then let's search for any interface definition containing RandomTeleportRadius
rg "interface\s+RandomTeleportRadius" -A 5
Length of output: 994
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadius.java (1)
10-10: Verify SimpleRandomTeleportRadius implementation.
Let's ensure the implementation class exists and properly implements this interface.
✅ Verification successful
SimpleRandomTeleportRadius implementation verified successfully
The implementation exists as a record class that properly implements the RandomTeleportRadius interface. It includes appropriate validation in its canonical constructor to ensure coordinate bounds are valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SimpleRandomTeleportRadius class definition
ast-grep --pattern 'class SimpleRandomTeleportRadius implements RandomTeleportRadius {
$$$
}'
# Alternatively, search for the file
fd SimpleRandomTeleportRadius.java
Length of output: 240
Script:
#!/bin/bash
# Let's check the implementation details of SimpleRandomTeleportRadius
cat eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/SimpleRandomTeleportRadius.java
Length of output: 563
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadiusConfig.java (1)
1-42: Verify integration with world borders and coordinate system.
Please ensure that:
- The coordinates work correctly with Minecraft's world border system
- The implementation handles edge cases when teleport radius exceeds world border
- The coordinate system aligns with Minecraft's coordinate system (especially for negative values)
Consider adding methods to validate coordinates against world border and handle cases where the random teleport radius exceeds the world border.
✅ Verification successful
Let me check the implementation details of these files to verify how the world border is handled.
World border integration is properly implemented
The implementation correctly handles world borders and coordinate systems:
- World border checks are implemented in
RandomTeleportSafeLocationService.isSafeLocation()usingworldBorder.isInside(location) - Border radius is properly calculated in
RandomTeleportServiceImpl.getWorldBorderRadius()by usingworldBorder.getSize() / 2 - Coordinate system handling is correct with:
- Proper handling of negative coordinates in border calculations
- Safe height range validation
- Proper chunk loading and world environment checks
- Center offset adjustments (+0.5) for player positioning
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for world border related code
rg -l "WorldBorder|getBorderSize"
# Search for other uses of coordinate systems
ast-grep --pattern 'Location($$$)'
Length of output: 393
Script:
#!/bin/bash
# Check implementation details of the files found
echo "=== RandomTeleportServiceImpl.java ==="
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
echo -e "\n=== RandomTeleportService.java ==="
cat eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java
echo -e "\n=== RandomTeleportSafeLocationService.java ==="
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java
Length of output: 11428
eternalcore-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportService.java (1)
48-56: Enhance documentation for RandomTeleportRadius parameter
The JavaDoc could be more specific about the purpose and benefits of using RandomTeleportRadius over the integer-based radius method, especially given the support for different border shapes.
Consider expanding the documentation:
/**
* Asynchronously retrieves a safe random location within the specified world, using radius.
*
* @param world The world in which to find a random location.
- * @param radius The radius around the player to search for a safe location.
+ * @param radius The radius representation defining the search area shape and boundaries.
+ * This parameter supports various border shapes beyond traditional square boundaries.
* @param attemptCount The number of attempts to find a safe location.
* @return A CompletableFuture containing the random Location that is deemed safe.
*/Additionally, consider deprecating the integer-based getSafeRandomLocation(World, int, int) method to encourage usage of the more flexible RandomTeleportRadius parameter.
Let's check if the integer-based method is still being used:
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (4)
17-30: LGTM! Well-structured dependency injection
The constructor changes improve separation of concerns by introducing dedicated services for safe location generation and teleport task management.
57-62: LGTM! Clean implementation using modern Java features
The switch expression and delegation pattern are well implemented, making the code both maintainable and readable.
80-84: LGTM! Correct border radius calculation
The implementation properly handles the world border size calculation and creates an appropriate RandomTeleportRadius instance.
71-72: Verify attemptCount parameter validation
The attemptCount parameter should be validated to prevent potential infinite loops or excessive attempts.
Consider adding parameter validation:
public CompletableFuture<Location> getSafeRandomLocation(World world, RandomTeleportRadius radius, int attemptCount) {
+ if (attemptCount <= 0) {
+ throw new IllegalArgumentException("Attempt count must be positive");
+ }
return this.safeLocationService.getSafeRandomLocation(world, radius, attemptCount);
}eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (2)
3-4: LGTM! Good refactoring of constants.
Moving constants to dedicated classes improves maintainability and follows the Single Responsibility Principle.
Line range hint 52-67: Review the delay and feedback handling in executeSelf.
There are two potential issues to consider:
- The delay is marked before confirming successful teleportation
- Users receive no feedback on successful teleportation
Consider this improved implementation:
void executeSelf(@Context Player player) {
UUID uuid = player.getUniqueId();
if (this.hasRandomTeleportDelay(player)) {
return;
}
this.noticeService.create()
.notice(translation -> translation.randomTeleport().randomTeleportStarted())
.player(player.getUniqueId())
.send();
this.randomTeleportService.teleport(player)
.whenCompleteAsync((result, error) -> {
if (error != null || !result.success()) {
this.handleTeleportFailure(player);
+ return;
}
+ this.delay.markDelay(uuid, this.config.randomTeleport.delay());
+ this.noticeService.create()
+ .notice(translation -> translation.randomTeleport().randomTeleportSuccess())
+ .player(player.getUniqueId())
+ .send();
});
- this.delay.markDelay(uuid, this.config.randomTeleport.delay());
}eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (1)
125-169: LGTM! Interface implementations are well-structured
The implementation:
- Correctly implements all interface methods
- Properly protects collections from modification
- Maintains clean and consistent return statements
...core-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadius.java
Show resolved
Hide resolved
...core-api/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportRadius.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportHeightRange.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportHeightRange.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportHeightRange.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResolveWorldUtil.java (1)
8-9: Add private constructor to prevent instantiation.The class structure looks good with appropriate access modifiers. Since this is a utility class with only static methods, consider adding a private constructor to prevent instantiation.
final class RandomTeleportResolveWorldUtil { + private RandomTeleportResolveWorldUtil() { + throw new UnsupportedOperationException("This is a utility class and cannot be instantiated"); + }eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (2)
Line range hint
62-70: Inconsistent service usage between self and other teleportThe
executeSelfmethod usesrandomTeleportTaskService.createTeleport()whileexecuteOtherusesrandomTeleportService.teleport(). This inconsistency could lead to different behavior between self and other teleportation.Consider using the same service in both methods:
- this.randomTeleportService.teleport(player) + this.randomTeleportTaskService.createTeleport(player)Also applies to: 89-98
Line range hint
128-132: Optimize permission check orderConsider checking
isOp()before the permission check for better performance, asisOp()is a simpler operation.- if (player.hasPermission(RTP_BYPASS_PERMISSION) || player.isOp()) { + if (player.isOp() || player.hasPermission(RTP_BYPASS_PERMISSION)) {eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java (2)
34-34: Fix constructor parameter formattingThe
RandomTeleportServiceparameter should be on a new line to maintain consistent formatting with other parameters.- RandomTeleportSafeLocationService randomTeleportSafeLocationService, RandomTeleportService randomTeleportService + RandomTeleportSafeLocationService randomTeleportSafeLocationService, + RandomTeleportService randomTeleportService
20-21: Consider making the class finalSince this is a service class with specific responsibilities, consider making it
finalto prevent inheritance and ensure implementation consistency.@Service -class RandomTeleportTaskService { +final class RandomTeleportTaskService {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java(5 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPermissionConstant.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResolveWorldUtil.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
🔇 Additional comments (13)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResolveWorldUtil.java (2)
1-24: Implementation looks solid overall!
The utility class is well-designed with clear responsibility and good error handling. The suggested improvements are minor and the core functionality is implemented correctly.
13-20: Verify thread safety of world operations.
The world resolution logic looks correct, but we should ensure it's being called from the main server thread when used in async teleport operations.
✅ Verification successful
Based on the evidence gathered, I can now provide the final response:
World resolution is safely handled in the async teleport flow
The world resolution is used within RandomTeleportServiceImpl and RandomTeleportTaskService, both of which implement async teleportation using CompletableFuture. The world resolution happens at the start of the teleport operation, before any async operations begin, making it thread-safe by design. The implementation correctly handles the world resolution synchronously before proceeding with the async safe location finding and teleportation.
RandomTeleportServiceImpl.teleport()resolves the world first, then proceeds with the async operationsRandomTeleportTaskService.teleport()follows the same pattern, resolving world before chaining async operations withthenCompose
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for async usage of RandomTeleportResolveWorldUtil
ast-grep --pattern 'async$$$RandomTeleportResolveWorldUtil.resolveWorld($$$)'
# Search for CompletableFuture or async task usage with this util
rg -A 5 "CompletableFuture.*RandomTeleportResolveWorldUtil"
Length of output: 141
Script:
#!/bin/bash
# Search for any usage of RandomTeleportResolveWorldUtil
rg -A 5 "RandomTeleportResolveWorldUtil"
# Search for async patterns in files containing RandomTeleportResolveWorldUtil
fd "RandomTeleportResolveWorldUtil" --exec rg -l "CompletableFuture|async|Runnable|BukkitScheduler" {}
# Look for the implementation to understand the context better
ast-grep --pattern 'class RandomTeleportResolveWorldUtil {
$$$
}'
Length of output: 3092
Script:
#!/bin/bash
# Check the implementation of RandomTeleportTaskService and RandomTeleportServiceImpl
# to see how they use the world resolution
rg -A 10 "resolveWorld\(.*\)" eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java
# Check if there's any async teleport implementation
rg -A 5 "class.*Teleport.*implements"
rg -A 5 "interface.*Teleport"
Length of output: 24488
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPermissionConstant.java (1)
5-7: Verify permission string usage across the codebase.
Let's ensure these permission strings are used consistently and there are no conflicting permissions defined elsewhere.
✅ Verification successful
Let me check the actual usage of these permissions in the RandomTeleportCommand.java file to ensure they are used correctly.
Let me check the actual permission checks in the command implementation using AST to find the permission validation code.
Let me check the command annotations to see how these permissions are used in command declarations.
Permission strings are used correctly and consistently
The verification shows that the permission constants are properly:
- Imported and used in RandomTeleportCommand.java
- Applied correctly via @Permission annotations for command methods
- Used in permission checks (hasRandomTeleportDelay method)
- No duplicate definitions found in the codebase
- Each permission serves a distinct purpose:
- RTP_COMMAND_SELF for self-teleportation
- RTP_COMMAND_OTHER for teleporting others
- RTP_BYPASS_PERMISSION for cooldown bypass
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for permission string usage and potential duplicates
# Search for permission string usage
echo "Checking permission string usage:"
rg -l "eternalcore\.rtp\.(bypass|other)?" --type java
# Search for potential duplicate permission constant definitions
echo -e "\nChecking for potential duplicate permission definitions:"
ast-grep --pattern 'String $_ = "eternalcore.rtp$$$"'
Length of output: 532
Script:
#!/bin/bash
# Check the implementation in RandomTeleportCommand.java
echo "Content of RandomTeleportCommand.java:"
rg -A 3 "eternalcore\.rtp" eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java
Length of output: 461
Script:
#!/bin/bash
# Search for permission checks in RandomTeleportCommand.java
echo "Searching for permission checks:"
ast-grep --pattern 'hasPermission($$$)' eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java
# Also check for any other permission-related code
echo -e "\nSearching for permission constant usage:"
rg -B2 -A2 "RandomTeleportPermissionConstant" eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java
Length of output: 950
Script:
#!/bin/bash
# Search for command annotations with permissions
echo "Searching for command annotations with permissions:"
ast-grep --pattern '@Command($$$)' eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java
# Also get the full content to see the complete context
echo -e "\nFull command class content:"
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java
Length of output: 6130
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
244-248: LGTM! Enhanced notification structure for random teleport.
The changes improve user experience by adding title and subtitle notifications during random teleportation, making the process more informative and visually appealing.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (3)
3-6: LGTM! Well-structured dependency management
The refactoring improves code organization through:
- Centralized permission and placeholder constants
- Clear separation of concerns with the new task service
- Proper dependency injection pattern
Also applies to: 29-29, 37-37, 42-42
103-109: LGTM! Well-structured notification handlers
The notification handlers are well-organized with:
- Clear separation of concerns
- Consistent translation service usage
- Proper placeholder formatting
29-29: Verify service implementations
Let's confirm the existence and compatibility of the new task service and its dependencies.
Also applies to: 37-37, 42-42
✅ Verification successful
Required service implementations are present and compatible
The verification shows that all necessary components are properly implemented:
RandomTeleportTaskServiceexists with required dependenciesRandomTeleportServiceinterface is defined with the expectedteleportmethodsRandomTeleportServiceImplprovides the implementationRandomTeleportResultrecord is properly defined and used consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for the required service implementations
ast-grep --pattern 'class RandomTeleportTaskService {
$$$
}'
# Verify the service interface methods
ast-grep --pattern 'interface RandomTeleportService {
$$$
CompletableFuture<RandomTeleportResult> teleport($$$);
$$$
}'
Length of output: 192
Script:
#!/bin/bash
# Let's try a broader search for related files and implementations
rg -l "RandomTeleport" --type java
# Check for specific class implementations
rg "class.*RandomTeleportTaskService" --type java -A 5
# Look for service interface
rg "interface.*RandomTeleportService" --type java -A 5
# Search for RandomTeleportResult usage
rg "RandomTeleportResult" --type java -A 3
Length of output: 11277
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (5)
3-4: LGTM! Good use of static import
The static import of resolveWorld improves code readability while maintaining clear intent.
21-21: LGTM! Good separation of concerns
The introduction of RandomTeleportSafeLocationService improves modularity by properly separating the safe location generation logic into its own service.
Also applies to: 25-31
Line range hint 35-57: LGTM! Well-structured async teleport implementation
The implementation properly handles:
- Pre-teleport event cancellation
- Async teleportation using PaperLib
- Post-teleport event notification
- Proper result propagation
88-92: LGTM! Proper world border radius calculation
The implementation correctly:
- Calculates border radius from world border size
- Creates a symmetric radius representation
- Handles floating-point division before integer casting
70-71: 🛠️ Refactor suggestion
Add proper deprecation marking
This method adapts the legacy integer-based radius API. It should be properly marked as deprecated to guide users to the new RandomTeleportRadius-based API.
Apply this diff:
+ @Deprecated(since = "1.0", forRemoval = true)
+ /**
+ * @deprecated Use {@link #getSafeRandomLocation(World, RandomTeleportRadius, int)} instead
+ */
@Override
public CompletableFuture<Location> getSafeRandomLocation(World world, int radius, int attemptCount) {
return this.safeLocationService.getSafeRandomLocation(world, RandomTeleportRadius.of(radius), attemptCount);
}eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java (1)
53-77:
Fix missing event dispatch and add exception handling
The method needs to dispatch events and handle potential exceptions from teleportation.
Previous review comments identified these issues. Please refer to the existing comments for the suggested fixes regarding:
- Missing event dispatch for
PreRandomTeleportEvent - Exception handling for teleportation results
...rc/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResolveWorldUtil.java
Show resolved
Hide resolved
.../main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPermissionConstant.java
Show resolved
Hide resolved
.../main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportPermissionConstant.java
Show resolved
Hide resolved
...ore/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java
Show resolved
Hide resolved
| if (this.teleportTaskService.isInTeleport(player.getUniqueId())) { | ||
| this.noticeService.create() | ||
| .notice(translation -> translation.teleport().teleportTaskAlreadyExist()) | ||
| .player(player.getUniqueId()) | ||
| .send(); | ||
|
|
||
| return CompletableFuture.completedFuture(new RandomTeleportResult(false, player.getLocation())); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Synchronization needed for teleport task management
The verification confirms that TeleportTaskService uses a non-concurrent HashMap (private final Map<UUID, Teleport> teleports = new HashMap<>();) to store teleport tasks, which is not thread-safe. Multiple threads accessing this map through isInTeleport() and other methods could indeed lead to race conditions.
eternalcore-core/src/main/java/com/eternalcode/core/feature/teleport/TeleportTaskService.java: ReplaceHashMapwithConcurrentHashMapor add synchronization to methods accessing the map- Methods affected:
isInTeleport()and related methods that modify theteleportsmap
🔗 Analysis chain
Add synchronization for teleport task check
The check for existing teleport tasks could lead to race conditions if multiple threads attempt to create teleport tasks simultaneously.
Let me gather more information about the TeleportTaskService implementation to check for any concurrent collections or synchronization mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for synchronized blocks or concurrent collections in TeleportTaskService
rg -A 10 "class TeleportTaskService" | rg -E 'synchronized|Concurrent|Atomic'
# Check for other services accessing isInTeleport
rg "isInTeleport\(" -B 2 -A 2
Length of output: 3118
Script:
#!/bin/bash
# Check TeleportTaskService implementation
ast-grep --pattern 'class TeleportTaskService {
$$$
}'
# Check for Map or Collection field declarations
rg "private.*Map|private.*Set|private.*List" eternalcore-core/src/main/java/com/eternalcode/core/feature/teleport/TeleportTaskService.java
# Check for concurrent imports
rg "^import.*concurrent" eternalcore-core/src/main/java/com/eternalcode/core/feature/teleport/TeleportTaskService.java
Length of output: 387
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (7)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/ConfigurationManager.java (1)
82-84: LGTM! Well-implemented defensive programming practice.The implementation correctly uses
Collections.unmodifiableSet()to prevent external modification of the internal state.Consider adding Javadoc to document the method's purpose and return value:
+ /** + * Returns an unmodifiable view of all registered configurations. + * + * @return An unmodifiable set of ReloadableConfig instances + */ public Set<ReloadableConfig> getConfigs() { return Collections.unmodifiableSet(this.configs); }eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/MigrationController.java (2)
11-12: Consider making the class public for better extensibility.The
MigrationControllermight need to be accessed or extended from other packages in the future. Consider making it public unless there's a specific reason to keep it package-private.@Controller -class MigrationController { +public class MigrationController {
1-37: Consider implementing migration versioning.Since this migration controller is being introduced to support configuration changes (including the new random teleport settings), consider implementing a versioning system for migrations. This would help track which migrations have been applied and ensure they're not re-run unnecessarily.
Key considerations:
- Add version tracking to configurations
- Store migration history
- Implement version comparison before migration
- Add ability to roll back migrations if needed
Would you like assistance in implementing this versioning system?
eternalcore-core/src/main/java/com/eternalcode/core/util/ReflectUtil.java (1)
75-96: Consider alternatives to reflection for configuration accessWhile reflection can be useful for framework-level operations, using it for configuration access might indicate a design that's too tightly coupled to implementation details. Consider these alternatives:
- Use a proper configuration abstraction layer
- Implement visitor pattern for traversing configuration objects
- Use builder pattern for configuration construction
This would make the code more maintainable and less prone to runtime errors.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (2)
30-45: Consider marking fields as finalThe dependency injection and initialization look good. The rename from
delaytocooldownbetter represents its purpose. Consider marking all fields asfinalsince they're initialized in the constructor and never changed.- private NoticeService noticeService; - private RandomTeleportService randomTeleportService; - private RandomTeleportTaskService randomTeleportTaskService; - private PluginConfiguration config; + private final NoticeService noticeService; + private final RandomTeleportService randomTeleportService; + private final RandomTeleportTaskService randomTeleportTaskService; + private final PluginConfiguration config;
Line range hint
49-73: Consider using consistent teleportation approachThe
executeSelfmethod usesrandomTeleportTaskServicewhileexecuteOtherusesrandomTeleportServicedirectly. This inconsistency might lead to different behavior between self and other teleportation.Consider using the task service consistently across both methods:
- this.randomTeleportService.teleport(player) + this.randomTeleportTaskService.createTeleport(player)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java (1)
9-25: Consider adding Javadoc comments to interface methods for clarityTo improve code readability and maintainability, consider adding Javadoc comments to the methods in the
RandomTeleportSettingsinterface. This will help other developers understand the purpose and usage of each configuration setting.For example:
/** * Gets the radius settings for random teleportation. * * @return the random teleport radius */ RandomTeleportRadius radius();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
eternalcore-api/src/main/java/com/eternalcode/core/delay/Delay.java(2 hunks)eternalcore-api/src/main/java/com/eternalcode/core/delay/DelaySettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/ConfigurationManager.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/Migration.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/MigrationController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/MigrationService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java(6 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResolveWorldUtil.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettings.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/util/ReflectUtil.java(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- eternalcore-api/src/main/java/com/eternalcode/core/delay/DelaySettings.java
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/Migration.java
🚧 Files skipped from review as they are similar to previous changes (3)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportResolveWorldUtil.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportTaskService.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java
🔇 Additional comments (8)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/ConfigurationManager.java (1)
82-84: Verify the integration with RandomTeleport functionality.
Since this change is part of the random teleport feature expansion, let's verify how this method is being used.
eternalcore-api/src/main/java/com/eternalcode/core/delay/Delay.java (1)
14-14: LGTM! Good use of Supplier pattern
The change from DelaySettings to Supplier<Duration> improves flexibility and follows the Single Responsibility Principle.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportCommand.java (3)
3-6: LGTM! Good use of static imports and constants
The refactoring to use static imports from dedicated constant classes improves code organization and maintainability.
Also applies to: 22-22
Line range hint 104-144: LGTM! Well-structured helper methods
The helper methods are well-organized and provide clear separation of concerns:
- Success/failure handling is properly separated
- Admin notification is handled separately
- Cooldown checking includes proper permission bypass
30-30: Verify test coverage for RandomTeleportTaskService
The new RandomTeleportTaskService is a critical component for handling teleportation. Let's verify its test coverage.
Also applies to: 38-38, 63-63
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java (2)
58-60:
Adjust random Y-coordinate in the Nether to avoid bedrock layers
In the Nether, setting randomY using this.random.nextInt(DEFAULT_NETHER_HEIGHT) can result in Y-coordinates within the bedrock layers at the bottom (Y=0 to Y=4) or near the roof (Y=121 to Y=127), which are unsuitable for player teleportation. To avoid these regions, consider restricting randomY to a safer range.
Apply this diff to adjust the Y-coordinate range:
if (world.getEnvironment() == World.Environment.NETHER) {
- randomY = this.random.nextInt(DEFAULT_NETHER_HEIGHT);
+ int netherMinY = 5;
+ int netherMaxY = 120;
+ randomY = netherMinY + this.random.nextInt(netherMaxY - netherMinY + 1);
}This ensures that players are teleported to safe Y-levels within the Nether, avoiding hazardous bedrock areas.
39-74: 🛠️ Refactor suggestion
Refactor recursive method to iterative approach to prevent potential stack overflow
The getSafeRandomLocation method uses recursion when retrying to find a safe location by calling itself with attemptCount - 1. If attemptCount is large, this could lead to a StackOverflowError. To enhance robustness, consider refactoring this method to use an iterative loop instead of recursion.
Here's an example of how you might refactor the method:
public CompletableFuture<Location> getSafeRandomLocation(World world, RandomTeleportRadius radius, int attemptCount) {
- if (attemptCount < 0) {
- return CompletableFuture.failedFuture(new RuntimeException("Cannot find safe location"));
- }
-
- // Existing logic to generate location...
-
- if (this.isSafeLocation(chunk, generatedLocation)) {
- return CompletableFuture.completedFuture(generatedLocation);
- }
-
- return this.getSafeRandomLocation(world, radius, attemptCount - 1);
+ for (int i = 0; i < attemptCount; i++) {
+ // Existing logic to generate location...
+
+ if (this.isSafeLocation(chunk, generatedLocation)) {
+ return CompletableFuture.completedFuture(generatedLocation);
+ }
+ }
+ return CompletableFuture.failedFuture(new RuntimeException("Cannot find safe location"));
}This iterative approach prevents potential stack overflows by avoiding recursive method calls.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (1)
71-73: 🛠️ Refactor suggestion
Mark legacy method as @deprecated to encourage migration
Since there is a new method that accepts RandomTeleportRadius, it's advisable to deprecate this legacy method that uses an integer radius. This will guide developers to migrate to the updated API.
Apply this diff to add the deprecation annotation:
+ @Deprecated(since = "1.0", forRemoval = true)
@Override
public CompletableFuture<Location> getSafeRandomLocation(World world, int radius, int attemptCount) {
return this.safeLocationService.getSafeRandomLocation(world, RandomTeleportRadius.of(radius), attemptCount);
}Additionally, consider adding a Javadoc @deprecated tag with guidance on the preferred method:
/**
* @deprecated Use {@link #getSafeRandomLocation(World, RandomTeleportRadius, int)} instead.
*/| @Inject | ||
| MigrationController(MigrationService migrationService, ConfigurationManager configurationManager, Logger logger) { | ||
| this.migrationService = migrationService; | ||
| this.configurationManager = configurationManager; | ||
| this.logger = logger; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks for injected dependencies.
To prevent potential NullPointerExceptions, consider adding null validation in the constructor.
@Inject
MigrationController(MigrationService migrationService, ConfigurationManager configurationManager, Logger logger) {
+ if (migrationService == null || configurationManager == null || logger == null) {
+ throw new IllegalArgumentException("Dependencies cannot be null");
+ }
this.migrationService = migrationService;
this.configurationManager = configurationManager;
this.logger = logger;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Inject | |
| MigrationController(MigrationService migrationService, ConfigurationManager configurationManager, Logger logger) { | |
| this.migrationService = migrationService; | |
| this.configurationManager = configurationManager; | |
| this.logger = logger; | |
| } | |
| @Inject | |
| MigrationController(MigrationService migrationService, ConfigurationManager configurationManager, Logger logger) { | |
| if (migrationService == null || configurationManager == null || logger == null) { | |
| throw new IllegalArgumentException("Dependencies cannot be null"); | |
| } | |
| this.migrationService = migrationService; | |
| this.configurationManager = configurationManager; | |
| this.logger = logger; | |
| } |
| @Subscribe | ||
| void onMigration(EternalInitializeEvent event) { | ||
| for (ReloadableConfig config : configurationManager.getConfigs()) { | ||
| boolean wasMigrated = migrationService.migrate(config); | ||
|
|
||
| if (wasMigrated) { | ||
| configurationManager.save(config); | ||
| logger.info("Configuration " + config.getClass().getSimpleName() + " was migrated and saved."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging implementation.
The current implementation has several areas for improvement:
- Missing error handling for failed migrations
- Inefficient string concatenation in logging
- No progress tracking for multiple configurations
Consider implementing this enhanced version:
@Subscribe
void onMigration(EternalInitializeEvent event) {
+ int totalConfigs = configurationManager.getConfigs().size();
+ int migratedCount = 0;
for (ReloadableConfig config : configurationManager.getConfigs()) {
+ String configName = config.getClass().getSimpleName();
+ try {
boolean wasMigrated = migrationService.migrate(config);
if (wasMigrated) {
configurationManager.save(config);
- logger.info("Configuration " + config.getClass().getSimpleName() + " was migrated and saved.");
+ logger.info(String.format("Configuration '%s' was migrated and saved successfully", configName));
+ migratedCount++;
}
+ } catch (Exception e) {
+ logger.severe(String.format("Failed to migrate configuration '%s': %s", configName, e.getMessage()));
+ }
}
+ logger.info(String.format("Migration completed: %d/%d configurations processed", migratedCount, totalConfigs));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Subscribe | |
| void onMigration(EternalInitializeEvent event) { | |
| for (ReloadableConfig config : configurationManager.getConfigs()) { | |
| boolean wasMigrated = migrationService.migrate(config); | |
| if (wasMigrated) { | |
| configurationManager.save(config); | |
| logger.info("Configuration " + config.getClass().getSimpleName() + " was migrated and saved."); | |
| } | |
| } | |
| } | |
| @Subscribe | |
| void onMigration(EternalInitializeEvent event) { | |
| int totalConfigs = configurationManager.getConfigs().size(); | |
| int migratedCount = 0; | |
| for (ReloadableConfig config : configurationManager.getConfigs()) { | |
| String configName = config.getClass().getSimpleName(); | |
| try { | |
| boolean wasMigrated = migrationService.migrate(config); | |
| if (wasMigrated) { | |
| configurationManager.save(config); | |
| logger.info(String.format("Configuration '%s' was migrated and saved successfully", configName)); | |
| migratedCount++; | |
| } | |
| } catch (Exception e) { | |
| logger.severe(String.format("Failed to migrate configuration '%s': %s", configName, e.getMessage())); | |
| } | |
| } | |
| logger.info(String.format("Migration completed: %d/%d configurations processed", migratedCount, totalConfigs)); | |
| } |
| public static <T> T getFieldValue(Field declaredField, Object object) { | ||
| try { | ||
| declaredField.setAccessible(true); | ||
| return unsafeCast(declaredField.get(object)); | ||
| } | ||
| catch (IllegalAccessException exception) { | ||
| throw new RuntimeException(exception); | ||
| } | ||
| } |
There was a problem hiding this comment.
Address security and type safety concerns in getFieldValue method
The current implementation has several critical issues:
- Permanently modifying field accessibility could have security implications
- Unchecked casting could lead to runtime exceptions
- Generic exception handling masks specific issues
Here's a safer implementation:
+ /**
+ * Gets the value of a field from an object using reflection.
+ *
+ * @param declaredField the field to get value from, must not be null
+ * @param object the object to get field value from, must not be null
+ * @param <T> the expected type of the field value
+ * @return the field value cast to the expected type
+ * @throws NullPointerException if any parameter is null
+ * @throws RuntimeException if the field is inaccessible or if type casting fails
+ */
public static <T> T getFieldValue(Field declaredField, Object object) {
+ Preconditions.checkNotNull(declaredField, "Field cannot be null!");
+ Preconditions.checkNotNull(object, "Object cannot be null!");
+
+ boolean wasAccessible = declaredField.canAccess(object);
try {
declaredField.setAccessible(true);
- return unsafeCast(declaredField.get(object));
+ Object value = declaredField.get(object);
+ return unsafeCast(value);
}
catch (IllegalAccessException exception) {
- throw new RuntimeException(exception);
+ throw new RuntimeException("Failed to access field: " + declaredField.getName(), exception);
+ }
+ finally {
+ declaredField.setAccessible(wasAccessible);
}
}Also, consider adding runtime type checking:
if (value != null && !declaredField.getType().isInstance(value)) {
throw new RuntimeException("Field value type mismatch. Expected: " +
declaredField.getType().getName() + ", but got: " + value.getClass().getName());
}| public static List<Field> getAllSuperFields(Class<?> aClass) { | ||
| List<Field> fields = new ArrayList<>(); | ||
| Class<?> currentClass = aClass; | ||
|
|
||
| while (currentClass != null && currentClass != Object.class) { | ||
| Collections.addAll(fields, currentClass.getDeclaredFields()); | ||
|
|
||
| currentClass = currentClass.getSuperclass(); | ||
| } | ||
|
|
||
| return fields; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation for getAllSuperFields method
The method needs some improvements for robustness and maintainability:
- Add null check for the input parameter
- Add Javadoc explaining the behavior, especially regarding field order and Object class exclusion
- Consider using a more memory-efficient collection if dealing with large class hierarchies
Here's the suggested implementation:
+ /**
+ * Returns all declared fields from the given class and its superclass hierarchy.
+ * Fields from the Object class are excluded. Fields are returned in order from
+ * subclass to superclass.
+ *
+ * @param aClass the class to get fields from, must not be null
+ * @return list of all declared fields
+ * @throws NullPointerException if aClass is null
+ */
public static List<Field> getAllSuperFields(Class<?> aClass) {
+ Preconditions.checkNotNull(aClass, "Class cannot be null!");
List<Field> fields = new ArrayList<>();
Class<?> currentClass = aClass;
while (currentClass != null && currentClass != Object.class) {
Collections.addAll(fields, currentClass.getDeclaredFields());
currentClass = currentClass.getSuperclass();
}
return fields;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static List<Field> getAllSuperFields(Class<?> aClass) { | |
| List<Field> fields = new ArrayList<>(); | |
| Class<?> currentClass = aClass; | |
| while (currentClass != null && currentClass != Object.class) { | |
| Collections.addAll(fields, currentClass.getDeclaredFields()); | |
| currentClass = currentClass.getSuperclass(); | |
| } | |
| return fields; | |
| } | |
| /** | |
| * Returns all declared fields from the given class and its superclass hierarchy. | |
| * Fields from the Object class are excluded. Fields are returned in order from | |
| * subclass to superclass. | |
| * | |
| * @param aClass the class to get fields from, must not be null | |
| * @return list of all declared fields | |
| * @throws NullPointerException if aClass is null | |
| */ | |
| public static List<Field> getAllSuperFields(Class<?> aClass) { | |
| Preconditions.checkNotNull(aClass, "Class cannot be null!"); | |
| List<Field> fields = new ArrayList<>(); | |
| Class<?> currentClass = aClass; | |
| while (currentClass != null && currentClass != Object.class) { | |
| Collections.addAll(fields, currentClass.getDeclaredFields()); | |
| currentClass = currentClass.getSuperclass(); | |
| } | |
| return fields; | |
| } |
| public Delay(Supplier<Duration> delayProvider) { | ||
| this.delaySettings = delayProvider; | ||
|
|
||
| this.delays = CacheBuilder.newBuilder() | ||
| .expireAfterWrite(delaySettings.delay()) | ||
| .expireAfterWrite(delayProvider.get()) | ||
| .build(); |
There was a problem hiding this comment.
Add null checks for the delay provider
The constructor should validate the supplier and its provided duration to prevent runtime errors.
Apply these changes:
public Delay(Supplier<Duration> delayProvider) {
+ Objects.requireNonNull(delayProvider, "delayProvider cannot be null");
this.delaySettings = delayProvider;
+ Duration initialDelay = Objects.requireNonNull(delayProvider.get(),
+ "delay duration cannot be null");
this.delays = CacheBuilder.newBuilder()
- .expireAfterWrite(delayProvider.get())
+ .expireAfterWrite(initialDelay)
.build();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Delay(Supplier<Duration> delayProvider) { | |
| this.delaySettings = delayProvider; | |
| this.delays = CacheBuilder.newBuilder() | |
| .expireAfterWrite(delaySettings.delay()) | |
| .expireAfterWrite(delayProvider.get()) | |
| .build(); | |
| public Delay(Supplier<Duration> delayProvider) { | |
| Objects.requireNonNull(delayProvider, "delayProvider cannot be null"); | |
| this.delaySettings = delayProvider; | |
| Duration initialDelay = Objects.requireNonNull(delayProvider.get(), | |
| "delay duration cannot be null"); | |
| this.delays = CacheBuilder.newBuilder() | |
| .expireAfterWrite(initialDelay) | |
| .build(); |
| int randomX = spawnX + this.random.nextInt(radius.maxX() - radius.minX() + 1) + radius.minX(); | ||
| int randomZ = spawnZ + this.random.nextInt(radius.maxZ() - radius.minZ() + 1) + radius.minZ(); |
There was a problem hiding this comment.
Fix the calculation of random coordinates to ensure correct range
The current calculation of randomX and randomZ may produce coordinates outside the intended radius due to incorrect addition of radius.minX() and radius.minZ() after the random number generation. This could lead to teleport locations beyond the specified boundaries.
Apply this diff to correct the calculation:
-int randomX = spawnX + this.random.nextInt(radius.maxX() - radius.minX() + 1) + radius.minX();
+int randomX = spawnX + radius.minX() + this.random.nextInt(radius.maxX() - radius.minX() + 1);
-int randomZ = spawnZ + this.random.nextInt(radius.maxZ() - radius.minZ() + 1) + radius.minZ();
+int randomZ = spawnZ + radius.minZ() + this.random.nextInt(radius.maxZ() - radius.minZ() + 1);This modification ensures that the random coordinates are correctly calculated within the specified radius relative to the spawn location.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int randomX = spawnX + this.random.nextInt(radius.maxX() - radius.minX() + 1) + radius.minX(); | |
| int randomZ = spawnZ + this.random.nextInt(radius.maxZ() - radius.minZ() + 1) + radius.minZ(); | |
| int randomX = spawnX + radius.minX() + this.random.nextInt(radius.maxX() - radius.minX() + 1); | |
| int randomZ = spawnZ + radius.minZ() + this.random.nextInt(radius.maxZ() - radius.minZ() + 1); |
| if (fieldType.isAnnotationPresent(Contextual.class)) { | ||
| Object fieldValue = ReflectUtil.getFieldValue(declaredField, config); | ||
| isMigrated |= reflectMigrate(fieldValue); | ||
| } |
There was a problem hiding this comment.
Correct the annotation check for @Contextual on fields
Currently, the code checks if the field's type is annotated with @Contextual using fieldType.isAnnotationPresent(Contextual.class). If the intention is to check whether the field itself is annotated with @Contextual, the correct method is declaredField.isAnnotationPresent(Contextual.class).
Apply this fix to ensure the annotation check is performed on the field:
- if (fieldType.isAnnotationPresent(Contextual.class)) {
+ if (declaredField.isAnnotationPresent(Contextual.class)) {
Object fieldValue = ReflectUtil.getFieldValue(declaredField, config);
+ if (fieldValue != null) {
isMigrated |= reflectMigrate(fieldValue);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fieldType.isAnnotationPresent(Contextual.class)) { | |
| Object fieldValue = ReflectUtil.getFieldValue(declaredField, config); | |
| isMigrated |= reflectMigrate(fieldValue); | |
| } | |
| if (declaredField.isAnnotationPresent(Contextual.class)) { | |
| Object fieldValue = ReflectUtil.getFieldValue(declaredField, config); | |
| if (fieldValue != null) { | |
| isMigrated |= reflectMigrate(fieldValue); | |
| } | |
| } |
| if (Migration.class.isAssignableFrom(fieldType)) { | ||
| Migration migration = ReflectUtil.getFieldValue(declaredField, config); | ||
| boolean wasMigrationSuccessful = migration.migrate(); | ||
| isMigrated |= wasMigrationSuccessful; | ||
| } |
There was a problem hiding this comment.
Handle potential NullPointerException when migrating fields
If a field implementing Migration is null, invoking migration.migrate() will result in a NullPointerException. It's important to check if migration is not null before calling the migrate() method to prevent runtime exceptions.
Apply this fix to handle the null check:
if (Migration.class.isAssignableFrom(fieldType)) {
Migration migration = ReflectUtil.getFieldValue(declaredField, config);
+ if (migration != null) {
boolean wasMigrationSuccessful = migration.migrate();
isMigrated |= wasMigrationSuccessful;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Migration.class.isAssignableFrom(fieldType)) { | |
| Migration migration = ReflectUtil.getFieldValue(declaredField, config); | |
| boolean wasMigrationSuccessful = migration.migrate(); | |
| isMigrated |= wasMigrationSuccessful; | |
| } | |
| if (Migration.class.isAssignableFrom(fieldType)) { | |
| Migration migration = ReflectUtil.getFieldValue(declaredField, config); | |
| if (migration != null) { | |
| boolean wasMigrationSuccessful = migration.migrate(); | |
| isMigrated |= wasMigrationSuccessful; | |
| } | |
| } |
| Object fieldValue = ReflectUtil.getFieldValue(declaredField, config); | ||
| isMigrated |= reflectMigrate(fieldValue); | ||
| } |
There was a problem hiding this comment.
Handle potential NullPointerException when migrating contextual fields
When accessing the value of a field annotated with @Contextual, the fieldValue might be null. Calling reflectMigrate(fieldValue) without checking can lead to a NullPointerException. Ensure that fieldValue is not null before recursively calling the migration method.
Apply this fix to handle the null check:
if (declaredField.isAnnotationPresent(Contextual.class)) {
Object fieldValue = ReflectUtil.getFieldValue(declaredField, config);
+ if (fieldValue != null) {
isMigrated |= reflectMigrate(fieldValue);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| private <T> boolean reflectMigrate(T config) { | ||
| boolean isMigrated = false; | ||
|
|
||
| for (Field declaredField : ReflectUtil.getAllSuperFields(config.getClass())) { | ||
| Class<?> fieldType = declaredField.getType(); | ||
|
|
||
| if (Migration.class.isAssignableFrom(fieldType)) { | ||
| Migration migration = ReflectUtil.getFieldValue(declaredField, config); | ||
| boolean wasMigrationSuccessful = migration.migrate(); | ||
| isMigrated |= wasMigrationSuccessful; | ||
| } | ||
|
|
||
| if (fieldType.isAnnotationPresent(Contextual.class)) { | ||
| Object fieldValue = ReflectUtil.getFieldValue(declaredField, config); | ||
| isMigrated |= reflectMigrate(fieldValue); | ||
| } | ||
| } | ||
|
|
||
| return isMigrated; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider exception handling for reflective operations
Reflective operations such as getFieldValue can throw exceptions (e.g., IllegalAccessException, IllegalArgumentException). It's advisable to handle these exceptions to prevent unexpected crashes and to provide meaningful error messages.
Wrap the reflective calls in try-catch blocks and handle exceptions appropriately:
for (Field declaredField : ReflectUtil.getAllSuperFields(config.getClass())) {
Class<?> fieldType = declaredField.getType();
if (Migration.class.isAssignableFrom(fieldType)) {
+ try {
Migration migration = ReflectUtil.getFieldValue(declaredField, config);
if (migration != null) {
boolean wasMigrationSuccessful = migration.migrate();
isMigrated |= wasMigrationSuccessful;
}
+ } catch (Exception e) {
+ // Handle exception or log error
+ }
}
if (declaredField.isAnnotationPresent(Contextual.class)) {
try {
Object fieldValue = ReflectUtil.getFieldValue(declaredField, config);
if (fieldValue != null) {
isMigrated |= reflectMigrate(fieldValue);
}
} catch (Exception e) {
// Handle exception or log error
}
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (1)
52-67: Consider adding more unsafe blocksThe unsafe blocks list could be expanded to include more hazardous blocks:
- Campfire and Soul Campfire (cause fire damage)
- Pointed Dripstone (causes fall damage)
- Deep Water (can cause drowning)
public Set<Material> unsafeBlocks = EnumSet.of( Material.LAVA, Material.WATER, + Material.CAMPFIRE, + Material.SOUL_CAMPFIRE, + Material.POINTED_DRIPSTONE, Material.CACTUS, // ... existing blocks ... );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/MigrationController.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/migration/MigrationController.java
🔇 Additional comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSettingsImpl.java (2)
14-15: 🛠️ Refactor suggestion
Add validation for delay duration
The delay duration should have minimum and maximum bounds to prevent abuse and ensure good user experience.
@Description("# Delay to wait for the random teleportation")
- public Duration delay = Duration.ofSeconds(5);
+ @Description({
+ "# Delay to wait for the random teleportation",
+ "# - Minimum: 1 second (prevents instant teleports)",
+ "# - Maximum: 10 seconds (ensures reasonable UX)",
+ "# - Default: 5 seconds"
+ })
+ public Duration delay = Duration.ofSeconds(5);Likely invalid or redundant comment.
34-35: 🛠️ Refactor suggestion
Add validation for radius configuration
The static radius of 5000 blocks in all directions could impact performance during location searching.
Why added
RandomTeleportRadiusRepresenter?Because some servers have a different border shape than a square.Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation