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

Step 1 completed #24

Closed alesky78 closed 7 years ago

alesky78 commented 7 years ago

1 step is completed

this is the list of the change:

alesky78 commented 7 years ago

Jeremy

I noted now that i didn't work in UTF-8 so all the 6 file commited are in cp1252

jdmonin commented 7 years ago

Hi Alessandro,

You work quickly, thank you!

Based on imports and calls, it looks like maybe soc.server.SOCInboundMessageQueue should be InboundMessageQueue in the genericServer package to maintain the generic/SOC separation.

My main concern though is that besides the expected moves and renames which do the refactoring, this pull's SOCInboundMessageQueue also contains a big implementation change. The Server and Treater threads have always synchronized and blocked on the inQueue vector. In this pull that isn't done anymore, instead there is a polling and sleeping mechanism with ConcurrentLinkedQueue. I'm open to talking about that kind of change but it's too much at once for this kind of refactoring: Please restore the proven Vector synchronization.

For "paperwork", in the files you've changed or added please don't forget to credit yourself with an attribution comment, something like:

  * Portions of this file Copyright (C) 2016 Alessandro D'Ottavio

Also please remove the new @author tag from NetStringServerSocket: This class was moved, not newly designed and created.

Don't worry in this pull about utf-8 versus cp1252: All of these classes contain only ascii characters, so they're identical in those two encodings.

Thank you for your patience with these details! -Jeremy

alesky78 commented 7 years ago

ok no problem with all the change, that is part of the collaboration. I immagine also that this is probably the the strategy that you use to keep tracking of the changes in the software. My change over the syncornization is hide inside the commits that i did. you are absolutely right...

well i this point to revert the syncornization... i will:

this should revert exactly the same logic as before

anyway if you agree wiht the refactoring over the syncornize logic... I can open another ticket and we can discuss there this kinds of topic.

jdmonin commented 7 years ago

Thanks again, those look great so far. Once the inQueue revert is ready, I'll be happy to merge.

alesky78 commented 7 years ago

Hi Jeremy

i reverted the code, now the sync logic is the same as before... i tested the changes in both pratics and real network mode

jdmonin commented 7 years ago

Hi Alessandro,

Thanks again for following up so quickly with those details!

This is now merged. I have some followup commits with minor adjustments to keep some parts that were overlooked and to fit the project coding style.

Thank you again! -Jeremy