search

jdmonin / JSettlers2

Java Settlers project home, downloads, and GPLv3 source code. To download the latest version as a JAR, see https://github.com/jdmonin/JSettlers2/releases/latest .
http://nand.net/jsettlers/
GNU General Public License v3.0
160 stars 63 forks source link

Tighten the Class scope #50

Closed colinwerner closed 5 years ago

colinwerner commented 5 years ago

In order to easily learn this code base, it helps to have the scope of each class explicitly defined and be as narrow as possible. So I did the following: 1.) explicitly add /package/ to any class declaration (default scope) 2.) tighten the scope of public classes to package 3.) remove unused import

jdmonin commented 5 years ago

Thank you for another nice pull request!

This one's a very clean patch, in both purpose and content, which makes it much easier to review.

Your understanding of what to hide was pretty accurate. I made about 40% public again in followup commits that give my rationale: End users might run them directly, or developers might use them to extend or modify JSettlers or its bots. This PR also nudged me to remove 2 completely unused data-structure classes from soc.robot.

I've merged and also thanked you in Versions.md; if you want a different attribution there (or none) please let me know :)

colinwerner commented 5 years ago

End users might run them directly, or developers might use them to extend or modify JSettlers or its bots.

May I suggest creating an API or a separate lib in order to make this behaviour explicit?

jdmonin commented 5 years ago

I think declaring them public makes them part of the overall API. I agree that's not the most structured way to do it. At this point I'd rather use a light touch and mention that kind of guideline in readme.developer or maybe in the javadocs, rather than do a bunch of refactoring that might never be used by anyone.

jdmonin commented 5 years ago

I do appreciate the suggestion, so please keep them coming. There just isn't always time to do everything :)