search

MCTCP / TerrainControl

Minecraft Terrain Generator for SpigotMC and Forge
https://www.spigotmc.org/threads/terraincontrol.37980/
MIT License
230 stars 163 forks source link

General cleanup of code and descriptions #509

Closed parlough closed 7 years ago

parlough commented 7 years ago

I went through most of the code and documents, cleaning up some basic things. This pull request does not add any functionality, nor does it fix any issues to my knowledge, but just aligns the style used throughout as the Forge version basically required none of these changes.

I do not believe any of these changes should cause issues with new or previous installations. I tested the Bukkit version and it seemed to work the exact same as before.

Feel free to point out mistakes or things I missed and I should be able to get to them quickly.

rutgerkok commented 7 years ago

This looks nice. Thank you!

All the this. changes seem a bit unnecessary for me, as fields are already marked blue in my IDE, so the this. is not really useful for me in most cases. However, if you want to continue working on the code, it makes sense for the code to be more in your style. So my question is:

Is it your intention to continue working on the code, or was this a one-off pull request?

If this is likely your only contribution, then I will still merge all changes except for the this. keyword. If you want to continue working on the code, then I'll merge the this. changes too.

I don't see any performance changes in the code, but maybe I missed them somewhere in the 175 changed files.

The only mistake I can see is in the file common/src/main/java/com/khorn/terraincontrol/BiomeIds.java, where you changed the positions of the braces in the equals method. The { should be put on a new line, as that is what the code already uses. It's just for consitency. Changing the whole project to use { on the same line is certainly possible, but again, this is a change that is only interesting if you want to continue working on the code.

In any case, TerrainControl is not really maintained by me anymore, so the more people willing to make contributions, the better!

parlough commented 7 years ago

I made the this. changes to align all the platforms as the Forge platform with blood's changes is already using it everywhere. Feel free to not use that part of the pull request though.

I am attempting to work on adding Sponge as a platform but as I get further I am seeing it may be hard or at least more challenging than I have the knowledge to complete on my own. So my answer to your question is maybe. If I do end up making changes to common I can add the this. if I see fit then.

I did not make any performance changes actually, not sure why I mentioned that, sorry. I removed that from the pull request description.

My latest commit should fix the bracket issue.

Comment again if you would like any other changes, thanks.

rutgerkok commented 7 years ago

Then I'll just see what happens. It's a bit difficult to split the this. changes from your other work, so I think I will just merge everything.

parlough commented 7 years ago

I don't really have the interest or time to continue pursuing Sponge support for Terrain control, so feel free to merge or close this pull request.