Skip to content

org.bukkit.craftbukkit.entity.CraftEntity.getEntity() has been broken up#1

Open
RenjiClark wants to merge 1 commit intoNicMcPhee:masterfrom
RenjiClark:ShortenCraftEntityGetEntity
Open

org.bukkit.craftbukkit.entity.CraftEntity.getEntity() has been broken up#1
RenjiClark wants to merge 1 commit intoNicMcPhee:masterfrom
RenjiClark:ShortenCraftEntityGetEntity

Conversation

@RenjiClark
Copy link
Copy Markdown

into multiple methods in order to shorten it.

into multiple methods in order to shorten it.
@NicMcPhee
Copy link
Copy Markdown
Owner

This looks reasonable and does improve readability, but it still ultimately leaves us with ton of ugly if-then-else chains. Because it doesn't actually change much, it might be acceptable to the Bukkit folks, but it seems that's in significant part because this doesn't actually do all that much?

@PhouLee
Copy link
Copy Markdown

PhouLee commented May 5, 2014

Non-code review:
I believe the pull request was very direct in what it did but the formatting of it does not fit the Craftbukkit's guidelines. You guys could have been a lot more descriptive in your pull request and ultimately the commit messages too. Both the pull request and commit have the exact same title/description. There is also no basis on why you guys shorten getEntity() method but as was probably as Nic said to improve readability. If this pull request is to be seriously consider there will need to be changes made to it and the commit messages.

Code-review:
This refactoring was a good start at tackling the ugly getEntity() method. I believe the CraftBukkit team may consider taking this since it improves readability and may allow them in the future to add on new entities much easier. This refactoring considers the commented facts that order really does matter and left the constructing of the entities in the same order.

@kirk0242
Copy link
Copy Markdown

kirk0242 commented May 6, 2014

Non-code review:
The pull request accurately describe what occurred, but it was extremely brief and it did not conform to the guidelines for pull requests. There maybe could have been more commits and more verbose commit messages.

Code review:
Breaking up the method is probably a good first step in terms of making the code readable and it gives a good jumping off point for the more meaningful refactoring of replacing the nasty nest of logic that happens in this method. The code likes tidy and up to specifications. It would have been great to see substantive changes to the code, but it is a huge undertaking and I understand why you chose to take on only what you did. Great job guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants