refactor: use the declaring class to access fields in child class #10
Conversation
![deepsource-autofix[bot]](https://avatars.githubusercontent.com/in/57168?s=60&v=4){kind=small}
Non-private static members of the parent class are accessible by child classes. However, it is a bad practice to do so, because it obscures where a value was actually declared. Always use only the declaring class to access static members.
WalkthroughThe codebase has undergone a significant refactoring, replacing direct access to the Changes
TipsChat with CodeRabbit Bot (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 10
Configuration used: CodeRabbit UI
Files selected for processing (11)
- src/main/java/coffee/client/feature/module/impl/combat/MCE.java (3 hunks)
- src/main/java/coffee/client/feature/module/impl/misc/AutoRespawn.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/misc/Automount.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/misc/ChestIndexer.java (5 hunks)
- src/main/java/coffee/client/feature/module/impl/misc/Test.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/movement/NoFall.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/render/FreeLook.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/render/Freecam.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/world/GodBridge.java (1 hunks)
- src/main/java/coffee/client/feature/module/impl/world/Nuker.java (4 hunks)
- src/main/java/coffee/client/feature/module/impl/world/SurvivalNuker.java (1 hunks)
Files skipped from review due to trivial changes (2)
- src/main/java/coffee/client/feature/module/impl/combat/MCE.java
- src/main/java/coffee/client/feature/module/impl/world/GodBridge.java
Additional comments: 6
src/main/java/coffee/client/feature/module/impl/misc/AutoRespawn.java (1)
- 13-18: The changes in this hunk are consistent with the pull request summary. The
clientobject has been replaced withModule.clientto access static members. This change improves the clarity of the code by making it clear where values are declared and accessed. However, ensure that theModule.clientis properly initialized before this method is called to avoid null pointer exceptions.src/main/java/coffee/client/feature/module/impl/render/FreeLook.java (1)
- 77-86: The changes in this hunk are consistent with the pull request summary. The
clientobject is replaced withModule.clientto access static members. This change improves the clarity of the code by making it clear where values are declared and accessed. However, please ensure that theModule.clientobject is properly initialized and has the same properties and methods as theclientobject.src/main/java/coffee/client/feature/module/impl/render/Freecam.java (1)
- 92-97: The change from
client.playertoclient.Module.playerseems to be a mistake. TheModuleclass does not have aplayerfield. Theplayerfield is part of theclientobject. This change could cause aNullPointerExceptionor aNoSuchFieldError. Please verify this change.- client.Module.player.getAbilities().setFlySpeed(0.05f); + client.player.getAbilities().setFlySpeed(0.05f);src/main/java/coffee/client/feature/module/impl/misc/Test.java (1)
- 26-32: The change from
client.player.getBlockPos()toModule.client.player.getBlockPos()is consistent with the pull request's goal of accessing static members through their declaring class. This change improves code clarity and maintainability. However, ensure thatModule.client.playeris always initialized before this method is called to avoid a potential NullPointerException.Also, consider the potential implications of starting a new thread every time
enable()is called. Ifenable()is called frequently, this could lead to a large number of threads being created, which could impact performance. Consider using a thread pool or some other form of limiting the number of threads created.src/main/java/coffee/client/feature/module/impl/world/SurvivalNuker.java (1)
- 84-88: The change from
client.worldtoModule.client.worldis consistent with the pull request's goal of accessing static members through their declaring class. This change improves code clarity and maintainability. However, ensure thatModule.client.worldis correctly initialized and has the same value as the previousclient.world. Also, verify that this change does not introduce any null pointer exceptions or other runtime errors.src/main/java/coffee/client/feature/module/impl/world/Nuker.java (1)
- 47-54: The changes made here are consistent with the pull request summary. The
clientobject has been replaced withModule.clientto access static members. This change improves the clarity of the code by making it clear where values are declared and accessed.
| if (Module.client.player == null || client.getNetworkHandler() == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The check for client.getNetworkHandler() == null has not been updated to Module.client.getNetworkHandler() == null. This should be updated for consistency and to avoid potential null pointer exceptions.
- if (Module.client.player == null || client.getNetworkHandler() == null) {
+ if (Module.client.player == null || Module.client.getNetworkHandler() == null) {57:
The client.getNetworkHandler().sendPacket(new PlayerMoveC2SPacket.OnGroundOnly(true)); line has not been updated to use Module.client. This should be updated for consistency.
- case Packet -> client.getNetworkHandler().sendPacket(new PlayerMoveC2SPacket.OnGroundOnly(true));
+ case Packet -> Module.client.getNetworkHandler().sendPacket(new PlayerMoveC2SPacket.OnGroundOnly(true));Commitable suggestion (Beta)
| if (Module.client.player == null || client.getNetworkHandler() == null) { | |
| return; | |
| } | |
| if (Module.client.player == null || Module.client.getNetworkHandler() == null) { | |
| return; | |
| } |
[ISSUE][REFINE]
57:
The client.getNetworkHandler().sendPacket(new PlayerMoveC2SPacket.OnGroundOnly(true)); line has not been updated to use Module.client. This should be updated for consistency.
- case Packet -> client.getNetworkHandler().sendPacket(new PlayerMoveC2SPacket.OnGroundOnly(true));
+ case Packet -> Module.client.getNetworkHandler().sendPacket(new PlayerMoveC2SPacket.OnGroundOnly(true));[ISSUE][REFINE]
|
|
||
| @Override | ||
| public void onFastTick() { | ||
| if (client.player.hasVehicle()) return; | ||
| if (Module.client.player.hasVehicle()) return; | ||
|
|
||
| for (Entity entity : client.world.getEntities()){ | ||
| if (entity.distanceTo(client.player) <= 4){ | ||
| for (Entity entity : Module.client.world.getEntities()){ | ||
| if (entity.distanceTo(Module.client.player) <= 4){ | ||
| if (entity instanceof HorseEntity && horse.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } else if (entity instanceof DonkeyEntity && donkey.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } else if (entity instanceof MuleEntity && mule.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } else if (entity instanceof BoatEntity && boat.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } else if (entity instanceof MinecartEntity && minecart.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } else if (entity instanceof PigEntity && pig.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } else if (entity instanceof LlamaEntity && llama.getValue()){ | ||
| client.interactionManager.interactEntity(client.player, entity, Hand.MAIN_HAND); | ||
| Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The code is repetitive and could be refactored to improve readability and maintainability. Consider creating a map of entity classes and their corresponding settings, then use a loop to check and interact with the entities. This would reduce the number of if statements and make the code easier to extend in the future.
Map<Class<? extends Entity>, BooleanSetting> entitySettings = new HashMap<>();
entitySettings.put(HorseEntity.class, horse);
entitySettings.put(DonkeyEntity.class, donkey);
entitySettings.put(MuleEntity.class, mule);
entitySettings.put(BoatEntity.class, boat);
entitySettings.put(MinecartEntity.class, minecart);
entitySettings.put(PigEntity.class, pig);
entitySettings.put(LlamaEntity.class, llama);
for (Entity entity : Module.client.world.getEntities()){
if (entity.distanceTo(Module.client.player) <= 4){
BooleanSetting setting = entitySettings.get(entity.getClass());
if (setting != null && setting.getValue()){
Module.client.interactionManager.interactEntity(Module.client.player, entity, Hand.MAIN_HAND);
}
}
}| @Override | ||
| public void tick() { | ||
| switch (modeSetting.getValue()) { | ||
| case Packet -> iterateOverRange(client.interactionManager.getReachDistance(), vec3d -> { | ||
| case Packet -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | ||
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | ||
| BlockState bs = client.world.getBlockState(bp); | ||
| if (bs.isAir()) { |
There was a problem hiding this comment.
The client object has been replaced with Module.client in the tick method. However, there is an inconsistency on line 65 where client.world is used instead of Module.client.world. This should be updated for consistency.
- BlockState bs = client.world.getBlockState(bp);
+ BlockState bs = Module.client.world.getBlockState(bp);Commitable suggestion (Beta)
| @Override | |
| public void tick() { | |
| switch (modeSetting.getValue()) { | |
| case Packet -> iterateOverRange(client.interactionManager.getReachDistance(), vec3d -> { | |
| case Packet -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | |
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | |
| BlockState bs = client.world.getBlockState(bp); | |
| if (bs.isAir()) { | |
| @Override | |
| public void tick() { | |
| switch (modeSetting.getValue()) { | |
| case Packet -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | |
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | |
| BlockState bs = Module.client.world.getBlockState(bp); | |
| if (bs.isAir()) { |
| new Vec3d(1, 1, 1), | ||
| 500); | ||
| }); | ||
| case Griefing -> iterateOverRange(client.interactionManager.getReachDistance(), vec3d -> { | ||
| case Griefing -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | ||
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | ||
| BlockState bs = client.world.getBlockState(bp); | ||
| if (bs.isAir()) { |
There was a problem hiding this comment.
Similar to the previous hunk, client.world is used instead of Module.client.world on line 83. This should be updated for consistency.
- BlockState bs = client.world.getBlockState(bp);
+ BlockState bs = Module.client.world.getBlockState(bp);Commitable suggestion (Beta)
| new Vec3d(1, 1, 1), | |
| 500); | |
| }); | |
| case Griefing -> iterateOverRange(client.interactionManager.getReachDistance(), vec3d -> { | |
| case Griefing -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | |
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | |
| BlockState bs = client.world.getBlockState(bp); | |
| if (bs.isAir()) { | |
| new Vec3d(1, 1, 1), | |
| 500); | |
| }); | |
| case Griefing -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | |
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | |
| - BlockState bs = client.world.getBlockState(bp); | |
| + BlockState bs = Module.client.world.getBlockState(bp); | |
| if (bs.isAir()) { |
| 500); | ||
| } | ||
| }); | ||
| case Interaction -> iterateOverRange(client.interactionManager.getReachDistance(), vec3d -> { | ||
| case Interaction -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | ||
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | ||
| BlockState bs = client.world.getBlockState(bp); | ||
| if (bs.isAir()) { |
There was a problem hiding this comment.
Again, client.world is used instead of Module.client.world on line 99. This should be updated for consistency.
- BlockState bs = client.world.getBlockState(bp);
+ BlockState bs = Module.client.world.getBlockState(bp);Commitable suggestion (Beta)
| 500); | |
| } | |
| }); | |
| case Interaction -> iterateOverRange(client.interactionManager.getReachDistance(), vec3d -> { | |
| case Interaction -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | |
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | |
| BlockState bs = client.world.getBlockState(bp); | |
| if (bs.isAir()) { | |
| 500); | |
| } | |
| }); | |
| case Interaction -> iterateOverRange(Module.client.interactionManager.getReachDistance(), vec3d -> { | |
| BlockPos bp = new BlockPos(BlockPos.ofFloored(vec3d)); | |
| BlockState bs = Module.client.world.getBlockState(bp); | |
| if (bs.isAir()) { |
| boolean isValidChestNeighbor(BlockPos owner, BlockPos bp) { | ||
| BlockState bs = client.world.getBlockState(bp); | ||
| BlockState bs1 = client.world.getBlockState(owner); | ||
| BlockState bs = Module.client.world.getBlockState(bp); | ||
| BlockState bs1 = Module.client.world.getBlockState(owner); | ||
| return bs1.getBlock() == bs.getBlock(); | ||
| } |
There was a problem hiding this comment.
The function isValidChestNeighbor checks if the block at the given position bp is the same as the block at the position owner. This function assumes that the world state is consistent and that the blocks at the given positions exist. If the world state changes between the two calls to getBlockState, this function may return incorrect results. Consider adding error handling to account for this possibility.
| if (childrenMap.values().stream().anyMatch(blockPos -> blockPos.contains(lastPos))) { | ||
| return; // already part of another chest | ||
| } | ||
| BlockState bs = client.world.getBlockState(lastPos); | ||
| BlockState bs = Module.client.world.getBlockState(lastPos); | ||
| Block block = bs.getBlock(); | ||
| List<Block> blocks = Arrays.asList(ALLOW_LIST); | ||
| if (blocks.contains(block)) { |
There was a problem hiding this comment.
The function send checks if the block at the given position lastPos is in the ALLOW_LIST. This function assumes that the world state is consistent and that the block at the given position exists. If the world state changes between the call to getBlockState and the check against ALLOW_LIST, this function may return incorrect results. Consider adding error handling to account for this possibility.
| int x = (int) (-Math.round(Math.sin(v1))); | ||
| int z = (int) Math.round(Math.cos(v1)); | ||
| BlockPos secondPos = lastPos.add(x, 0, z); | ||
| BlockState bs1 = client.world.getBlockState(secondPos); | ||
| BlockState bs1 = Module.client.world.getBlockState(secondPos); | ||
| if (bs1.getBlock() == block && bs1.get(ChestBlock.CHEST_TYPE) == chestType.getOpposite() && bs1.get(ChestBlock.FACING) == direction) { | ||
| stacks.remove(secondPos); // if the neighbour was already indexed, remove him and use this pos instead | ||
| childrenMap.computeIfAbsent(lastPos, blockPos -> new ArrayList<>()).add(secondPos); |
There was a problem hiding this comment.
The function send updates the stacks and childrenMap based on the state of the world. This function assumes that the world state is consistent and that the blocks at the given positions exist. If the world state changes between the calls to getBlockState and the updates to stacks and childrenMap, this function may behave incorrectly. Consider adding error handling to account for this possibility.
| } | ||
| this.stacks.putIfAbsent(this.currentPosClicked, new Int2ObjectArrayMap<>()); | ||
| Int2ObjectMap<ItemStack> itemStackInt2ObjectMap = this.stacks.get(this.currentPosClicked); | ||
| DefaultedList<Slot> slots = client.player.currentScreenHandler.slots; | ||
| DefaultedList<Slot> slots = Module.client.player.currentScreenHandler.slots; | ||
| int contentLength = slots.size() - 9 * 4; // this one includes the actual inventory so we remove it from the index | ||
| for (int i = 0; i < contentLength; i++) { | ||
| Slot slot = slots.get(i); |
There was a problem hiding this comment.
The function send updates the stacks based on the state of the player's inventory. This function assumes that the player's inventory state is consistent and that the slots in the inventory exist. If the inventory state changes between the call to getStack and the updates to stacks, this function may behave incorrectly. Consider adding error handling to account for this possibility.
| public void tick() { | ||
| if (updateTimer.hasExpired(5_000)) { | ||
| for (BlockPos blockPos : new ArrayList<>(stacks.keySet())) { | ||
| if (!blockPos.isWithinDistance(client.player.getBlockPos(), 64)) { | ||
| if (!blockPos.isWithinDistance(Module.client.player.getBlockPos(), 64)) { | ||
| continue; // dont update this one we have no fucking idea whats in it | ||
| } | ||
| BlockState bs = client.world.getBlockState(blockPos); | ||
| BlockState bs = Module.client.world.getBlockState(blockPos); | ||
| if (!Arrays.asList(ALLOW_LIST).contains(bs.getBlock())) { | ||
| stacks.remove(blockPos); | ||
| } |
There was a problem hiding this comment.
The function tick updates the stacks based on the state of the world. This function assumes that the world state is consistent and that the blocks at the given positions exist. If the world state changes between the calls to getBlockState and the updates to stacks, this function may behave incorrectly. Consider adding error handling to account for this possibility.
Non-private static members of the parent class are accessible by child classes. However, it is a bad practice to do so, because it obscures where a value was actually declared. Always use only the declaring class to access static members.
Summary by CodeRabbit