jdmonin
commented
7 years ago Hello Alessandro,
Thank you for spending time on studying the server and giving thought to refactoring to make things clearer. I completely agree that consistent naming and clearly defined responsibilities help keep the structure more elegant and understandable. I'm glad the javadocs helped out; detailed and up-to-date documentation is an important goal for me.
I like the refactoring you've proposed. I would do some parts slightly differently. Here is the action plan I'd like to do:
- For consistency rename genericServer.Connection to NetStringConnection
- Move Server.NetStringServerSocket to be a top-level class
- New class SOCInboundMessageQueue from Server.Treater, Server.inQueue and related methods
- New class SOCMessageDispatcher from SOCServer.processCommand and related methods [simplified name from SOCMessageHandlerDispatcher]
- New class SOCServerMessageHandler for non-game-specific inbound message handlers (handleVERSION, etc) and related business logic
- Since SOCGameHandler handles not only messages but also game lifecycle business logic (startGame, endGameTurnIfInactive, etc) I'd like to keep that class name. So:
- New class SOCGameMessageHandler from SOCGameHandler's handle* methods
Do you want me to do all these steps, or would you like to do some of them? If you're taking on some of it, about when would you like to make your changes and submit them? That way you and I aren't working on the same classes at the same time. :)
You've put a lot of thought into this and brought good suggestions, and I'm grateful! Please let me know what you think about the revised plan and timing.
Many thanks, -Jeremy
alesky78
hello Jeremy
I would like to propose to you same little reorganizations in the code... My proposal came out after I have studied the code of the server side implementation. In particular I have analysed the packages: soc.server soc.server.genericServer soc.messages
strong point that I recognized in JSettlers2 code, is that it is full of javadoc documentation. This help me in this days to drive my study on the server architecture. Obviously the javadoc cannot cover completely the understanding, and you have to navigate the code and the classes. In fact another aspect that I find extremely important when you develop code is that classes with roles/names well defined and relationship well identified are extremely important point to interpretate/understand architecture of software already implemented.
And here there is the biggest complexity that I found. I agree with the centralization of the code, but in JSettlers2 I found that same time the centralization applied overwrite the concept of responsibility in few classes and the result is an hidden architecture that force too much the reading of the code to understand it... I mean there are same classes that are overloaded of responsibilities, and this hide the server architecture. I think that if in the soc.server there was 3/4 classes more to take the responsibilities of same aspects of the server processing, the system could benefit of a more simple understanding for people like me that want to read the code and understand it and in case support the project. I saw, for example, that you already start to do it in the version 2.0 with the class soc.server.SOCGameHandler for example...
so in conclusion this is my simple proposal
class SOCImboundMessageQueue the responsibility of this class is to 1- store the messages received from the client by the Connection and the LocalStringConnection objects that today is done in the inQueue Server class property 2- remove the relationship between Connection and the LocalStringConnection whit the Server Object that today is done by the method public void treat(String s, StringConnection c) in Server object.. the two connection will call treat over the SOCImboundMessageQueue 3- manage the message received and dispatch to the correct handler for the execution, that today is done by the inner class Treater in the Server class, move the class in SOCImboundMessageQueue this will remove the logic for the Server and SocServer classes
class SOCServerMessageHandler like the SOCGameMessageHandler this class has the responsibility to maintain and implement the messages management that still is in the SocServer dispatched by the method public void processCommand(String s, StringConnection c) that mean that all the method handleSERVERPING... handleVERSION... handleAUTHREQUEST.... handleJOIN... should be moved here
class SOCGameMessageHandler is just the renaming of the SOCGameHandler to maintain the same name convention of the SOCServerMessageHandler
class SOCMessageHandlerDispatcher the responsibility of this class is to 1- receive the event from the SOCImboundMessageQueue (see the point 2 class Treater) and dispatch to the correct message Handler SOCServerMessageHandler or SOCGameMessageHandler
class NetStringServerSocket remove has inner class in Server and make as public class in the package soc.server.genericServer of for the rest of the hierarchy LocalStringServerSocket and StringServerSocket
class NetStringConnection rename the class Connection in NetStringConnection to maintain the same name convention whit its server socket counterpart NetStringServerSocket has it already exists also for LocalStringServerSocket and LocalStringConnection
in this way I think that is more simple to identified 3 main blocks in the server architecture 1 - connection management and initialization of the server infrastructure --> SOCServer class 2 - message received and handling --> SOCImboundMessageQueue 3 - server business logic --> SOCMessageHandlerDispatcher,SOCGameMessageHandler,SOCServerMessageHandler
have a more clear definition of the chain of the message management Connection --> SOCImboundMessageQueue --> SOCMessageHandlerDispatcher
and finally have two extra classes that in the javadoc that can explain clearly the server messaging model of the software: SOCImboundMessageQueue and SOCMessageHandlerDispatcher
please Jeremy let me know your point of view