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

move MessageTreater out of SOCPlayerClient #53

Closed colinwerner closed 5 years ago

colinwerner commented 5 years ago

1.) rename MessageTreater to MessageHandler 2.) move MessageHandler to its own class 3.) added a couple comments about follow-up commits 4.) added two getters to facilitate the move to its own class

To test I beat up a bunch of bots with unrelenting ferocity.

I also ran a bunch of bots (as per jdmonin). java -jar JSettlersServer-2.0.00.jar -Djsettlers.bots.botgames.total=9 -Djsettlers.bots.botgames.parallel=5 -Djsettlers.bots.fast_pause_percent=1

jdmonin commented 5 years ago

Thank you again for a very nice contribution! I like the approach you took; it's a nice balance between separation and pragmatic access.

Since this is a substantial contribution, I'll be a little more picky in this code review. The points I'm raising are minor blemishes, so I'm sure you won't have trouble taking care of them. :)

So, a few really small things. If you can please add a follow-up commit for them to your branch, that should take care of it.

Thanks again for putting this together!

-Jeremy

colinwerner commented 5 years ago

There is one instance where the entire map is required (I believe for broadcast message). So I left it as a getter -- it can be simplified later?

  • Instead of client.getClientListeners().get(gameName), it might be cleaner to add a getter to do client.getClientListener(gameName)

I could add both?

jdmonin commented 5 years ago

Adding both sounds good :)

colinwerner commented 5 years ago

I believe I have updated everything (and some more). Please let me know...