more cleanup to better understand code

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

157 stars 63 forks source link

more cleanup to better understand code #51

Closed colinwerner closed 5 years ago

colinwerner commented 5 years ago

1.) remove duplicated constant (at the expense of additional imports) 2.) tighten inner class scope (add TODO to consider making them their own classes -- as there are multiple nested classes) 3.) make classes final to avoid extensions (inner classes shouldn't really be extended) 4.) TODO: these inner classes don't really need to be static, another thing to do... 5.) dont need to pass around objects that have refs to each other (ie client and net)

jdmonin commented 5 years ago

Thanks again!

Here's my feedback:

Having fewer of these classes be public is a nice improvement, thanks again for looking at them!
I'm not a fan of having the client or its GUI panels import the server just to avoid a duplicated int constant. Although the server is bundled into the client JAR so people can play locally, someone should be able to build a client JAR without any server files. Just keep ClientNetwork.SOC_PORT_DEFAULT instead.
Some classes nested in SOCPlayerClient are static because they should be top-level classes. I haven't gotten around to doing so, but I'll have time on Monday or Tuesday. If you're still doing work in SOCPlayerClient I'll hold off to avoid create merge conflicts. So please let me know either way :)

colinwerner commented 5 years ago

I agree having the client depend on the server is probably not the best idea; however, the client package already depends on the server package, so as it stands we cannot build the client on its own. So that's why I thought having the default port defined in one location was better (now if the client package doesn't depend on the server package, then it's a different story.

colinwerner commented 5 years ago

We can abandon this and I'll let you move the classes around...

jdmonin commented 5 years ago

OK, the nested classes are out except for MessageTreater. Thanks for nudging me along to get that done :)

Some of the names seemed unclear at this point, so GameManager is now GameMessageMaker, GameDisplay is MainDisplay.

I won't have time to move MessageTreater soon, so my moves are done for now.