[C] Refactor large method getEntity to reduce complexity and size.#17
[C] Refactor large method getEntity to reduce complexity and size.#17Athear wants to merge 1 commit intoNicMcPhee:masterfrom
Conversation
This commit replaces the 120-line method that handled transitioning from Minecraft entities to Craftbukkit entites with a 40 line reflective method. The new method should allow simpler handling of new Minecraft entity classes. Additionally, this commit adds tests for entity translation.
|
This is an absolutely excellent pull request write-up; it has the perfect amount of detail split up into easily digestible parts. The problem is well defined, as is the justification. The 'PR Breakdown' section allowed me to understand what was being modified without actually looking at the code, and to understand that they're using reflection to solve the problem. Also, adding their own tests for the code is super awesome! |
|
Non-code: Code: |
|
Really nice job with a tricky pull request, both to document and to implement. Well done! |
4 participants
The Issue:
The getEntity method in CraftEntity is very large and complex, involving around 120 lines of code and a long, nested if-else chain. This can lead to issues when new Minecraft entities are added, requiring precise placement within the chain.
Justification for this PR:
This PR proposes to reduce both length and complexity with reflection. The proposed getEntity method will be less visually intimidating to editors and require less editing in general.
PR Breakdown:
As most Craftbukkit entity names reflect the naming scheme of Minecraft entities, reflection can be used to quickly convert from a Minecraft entity to a Craftbukkit entity without having to traverse a nested if chain. The new getEntity method extracts the Mincraft entity's name and converts it to the Craftbukkit name, then finds and instantiates the desired Craftbukkit entity.
Special Craftbukkit entity names, which do not precisely match their Minecraft counterpart, are taken from a static map. This map is only used for special names to reduce complexity and increase easy of converting new additions from Minecraft updates.
External behavior of getEntity should remain unchanged.
Test Results and Materials:
Test file: Included in PR.
Tests were added to test the translation of Minecraft entities to Craftbukkit entities. The test instantiates as many Minecraft entities as we were able and attempts to generate Craftbukkit entities from them using the getEntity method. Certain entities required more information than we were able to provide for instantiation, and were excluded from the test suite. These have been noted at the beginning of the file.
The tests were written and confirmed against the original getEntity method before work began on the new getEntity method.