refactor!: complete rewrite of DoubleJump plugin#61
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the DoubleJump plugin into a multi-module architecture, updating the project to Java 21 and Gradle 9. It introduces a component-based system using dependency injection and updates core features like jump restrictions, cooldowns, and visual effects. Feedback focuses on correcting metadata in the build scripts that appear to be copied from another project, improving dependency version stability, and making hardcoded values like the ping threshold configurable.
...ore/src/main/java/com/github/imdmk/doublejump/core/feature/jump/rule/player/LaggingRule.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request performs a major architectural overhaul, splitting the project into core and plugin modules while introducing dependency injection and updated library integrations. The refactoring improves modularity but requires critical adjustments to dependency relocation in the build process to prevent classpath conflicts. Further feedback addresses incomplete item matching logic regarding flags and durability, the renaming of a misleading time conversion constant, and the optimization of dependency scopes. Additionally, a risk was identified in the configuration service where automatic field binding could lead to unintentional DI container overwrites.
...p-core/src/main/java/com/github/imdmk/doublejump/core/feature/jump/item/JumpItemMatcher.java
Show resolved
Hide resolved
| for (Field field : config.getClass().getFields()) { | ||
| try { | ||
| Object value = field.get(config); | ||
| if (value != null) { | ||
| resources.on(field.getType()) | ||
| .assignInstance(value); | ||
| } | ||
| } catch (IllegalAccessException exception) { | ||
| throw new IllegalStateException( | ||
| "Failed to read config field: " + field.getName(), | ||
| exception | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop binds all public fields of a configuration class to the DI container. This can be risky, as it might lead to unintentional overwriting of bindings if two different configuration classes have public fields of the same type. For example, if both ConfigA and ConfigB have a public field public String someValue, the one processed last will overwrite the binding for String. It would be safer to use named bindings or qualifiers if your DI framework supports it, or to be very careful about the types of public fields in your configuration classes.
...-core/src/main/java/com/github/imdmk/doublejump/core/shared/time/DurationTicksConverter.java
Show resolved
Hide resolved
1 participant
BREAKING CHANGE: plugin structure, config format and internal APIs have been changed