{"action":"created","issue":{"url":"https://api.github.com/repos/jdmonin/JSettlers2/issues/23","repository_url":"https://api.github.com/repos/jdmonin/JSettlers2","labels_url":"https://api.github.com/repos/jdmonin/JSettlers2/issues/23/labels{/name}","comments_url":"https://api.github.com/repos/jdmonin/JSettlers2/issues/23/comments","events_url":"https://api.github.com/repos/jdmonin/JSettlers2/issues/23/events","html_url":"https://github.com/jdmonin/JSettlers2/issues/23","id":188011032,"number":23,"title":"refactoring server side class to highlights responsabilities","user":{"login":"alesky78","id":11068659,"avatar_url":"https://avatars.githubusercontent.com/u/11068659?v=3","gravatar_id":"","url":"https://api.github.com/users/alesky78","html_url":"https://github.com/alesky78","followers_url":"https://api.github.com/users/alesky78/followers","following_url":"https://api.github.com/users/alesky78/following{/other_user}","gists_url":"https://api.github.com/users/alesky78/gists{/gist_id}","starred_url":"https://api.github.com/users/alesky78/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alesky78/subscriptions","organizations_url":"https://api.github.com/users/alesky78/orgs","repos_url":"https://api.github.com/users/alesky78/repos","events_url":"https://api.github.com/users/alesky78/events{/privacy}","received_events_url":"https://api.github.com/users/alesky78/received_events","type":"User","site_admin":false},"labels":[{"id":15126538,"url":"https://api.github.com/repos/jdmonin/JSettlers2/labels/enhancement","name":"enhancement","color":"84b6eb","default":true}],"state":"open","locked":false,"assignee":{"login":"jdmonin","id":2141472,"avatar_url":"https://avatars.githubusercontent.com/u/2141472?v=3","gravatar_id":"","url":"https://api.github.com/users/jdmonin","html_url":"https://github.com/jdmonin","followers_url":"https://api.github.com/users/jdmonin/followers","following_url":"https://api.github.com/users/jdmonin/following{/other_user}","gists_url":"https://api.github.com/users/jdmonin/gists{/gist_id}","starred_url":"https://api.github.com/users/jdmonin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdmonin/subscriptions","organizations_url":"https://api.github.com/users/jdmonin/orgs","repos_url":"https://api.github.com/users/jdmonin/repos","events_url":"https://api.github.com/users/jdmonin/events{/privacy}","received_events_url":"https://api.github.com/users/jdmonin/received_events","type":"User","site_admin":false},"assignees":[{"login":"jdmonin","id":2141472,"avatar_url":"https://avatars.githubusercontent.com/u/2141472?v=3","gravatar_id":"","url":"https://api.github.com/users/jdmonin","html_url":"https://github.com/jdmonin","followers_url":"https://api.github.com/users/jdmonin/followers","following_url":"https://api.github.com/users/jdmonin/following{/other_user}","gists_url":"https://api.github.com/users/jdmonin/gists{/gist_id}","starred_url":"https://api.github.com/users/jdmonin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdmonin/subscriptions","organizations_url":"https://api.github.com/users/jdmonin/orgs","repos_url":"https://api.github.com/users/jdmonin/repos","events_url":"https://api.github.com/users/jdmonin/events{/privacy}","received_events_url":"https://api.github.com/users/jdmonin/received_events","type":"User","site_admin":false}],"milestone":null,"comments":5,"created_at":"2016-11-08T14:49:23Z","updated_at":"2016-11-11T12:09:47Z","closed_at":null,"body":"hello Jeremy \r\n\r\nI would like to propose to you same little reorganizations in the code...\r\nMy proposal came out after I have studied the code of the server side implementation. \r\nIn particular I have analysed the packages:\r\nsoc.server \r\nsoc.server.genericServer\r\nsoc.messages\r\n\r\nstrong point that I recognized in JSettlers2 code, is that it is full of javadoc documentation. \r\nThis help me in this days to drive my study on the server architecture.\r\nObviously the javadoc cannot cover completely the understanding, and you have to navigate the code and the classes.\r\nIn fact another aspect that I find extremely important when you develop code is that classes with roles/names well defined and relationship well identified \r\nare extremely important point to interpretate/understand architecture of software already implemented.\r\n  \r\nAnd here there is the biggest complexity that I found.\r\nI 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\r\nand the result is an hidden architecture that force too much the reading of the code to understand it...\r\nI mean there are same classes that are overloaded of responsibilities, and this hide the server architecture.\r\nI 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 \r\nunderstanding for people like me that want to read the code and understand it and in case support the project.\r\nI saw, for example, that you already start to do it in the version 2.0 with the class soc.server.SOCGameHandler for example...\r\n\r\nso in conclusion this is my simple proposal\r\n\r\nclass SOCImboundMessageQueue\r\nthe responsibility of this class is to \r\n1- store the  messages received from the client by the Connection and the LocalStringConnection objects  that today is done in the inQueue Server class property\r\n2- remove the relationship between Connection and the LocalStringConnection  whit the Server Object that today is done by the method\r\npublic void treat(String s, StringConnection c) in Server object.. the two connection will call  treat over the SOCImboundMessageQueue\r\n3- 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\r\nthis will remove the logic for the Server and SocServer classes\r\n\r\nclass SOCServerMessageHandler\r\nlike the SOCGameMessageHandler this class has the responsibility to maintain and  implement the messages management\r\nthat still is in the SocServer dispatched by the method  public void processCommand(String s, StringConnection c)\r\nthat mean that all the method handleSERVERPING... handleVERSION... handleAUTHREQUEST.... handleJOIN...\r\nshould be moved here\r\n\r\nclass SOCGameMessageHandler\r\nis just the renaming of the SOCGameHandler to maintain the same name convention of the SOCServerMessageHandler\r\n\r\nclass SOCMessageHandlerDispatcher\r\nthe responsibility of this class is to \r\n1- receive the event from the SOCImboundMessageQueue (see the point 2 class Treater) and dispatch to the correct message Handler\r\nSOCServerMessageHandler or SOCGameMessageHandler\r\n\r\nclass NetStringServerSocket\r\nremove has inner class in Server\r\nand make as public class in the package soc.server.genericServer of for the rest of the hierarchy LocalStringServerSocket and StringServerSocket\r\n\r\nclass NetStringConnection \r\nrename the class Connection in  NetStringConnection to maintain the same name convention whit its server socket counterpart NetStringServerSocket\r\nhas it already exists also for  LocalStringServerSocket and LocalStringConnection\r\n\r\n\r\n\r\nin this way I think that is more simple to identified 3 main blocks in the server architecture\r\n1 - connection management and initialization of the server infrastructure --> SOCServer class\r\n2 - message received and handling --> SOCImboundMessageQueue\r\n3 - server business logic  --> SOCMessageHandlerDispatcher,SOCGameMessageHandler,SOCServerMessageHandler\r\n\r\nhave a more clear definition of the chain of the message management\r\nConnection --> SOCImboundMessageQueue --> SOCMessageHandlerDispatcher\r\n\r\nand finally have two extra classes that in the javadoc that can explain clearly the server messaging model of the software: SOCImboundMessageQueue and SOCMessageHandlerDispatcher\r\n\r\n\r\nplease Jeremy \r\nlet me know your point of view\r\n"},"comment":{"url":"https://api.github.com/repos/jdmonin/JSettlers2/issues/comments/259944548","html_url":"https://github.com/jdmonin/JSettlers2/issues/23#issuecomment-259944548","issue_url":"https://api.github.com/repos/jdmonin/JSettlers2/issues/23","id":259944548,"user":{"login":"jdmonin","id":2141472,"avatar_url":"https://avatars.githubusercontent.com/u/2141472?v=3","gravatar_id":"","url":"https://api.github.com/users/jdmonin","html_url":"https://github.com/jdmonin","followers_url":"https://api.github.com/users/jdmonin/followers","following_url":"https://api.github.com/users/jdmonin/following{/other_user}","gists_url":"https://api.github.com/users/jdmonin/gists{/gist_id}","starred_url":"https://api.github.com/users/jdmonin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jdmonin/subscriptions","organizations_url":"https://api.github.com/users/jdmonin/orgs","repos_url":"https://api.github.com/users/jdmonin/repos","events_url":"https://api.github.com/users/jdmonin/events{/privacy}","received_events_url":"https://api.github.com/users/jdmonin/received_events","type":"User","site_admin":false},"created_at":"2016-11-11T12:09:47Z","updated_at":"2016-11-11T12:09:47Z","body":"Hello Alessandro,\r\n\r\nLet's coordinate the next steps: I want to do SOCServerMessageHandler. I can also do SOCMessageDispatcher and/or SOCGameMessageHandler unless you want one or both of those.\r\n\r\nPlease let me know,\r\n-Jeremy"}}
{"actor":{"display_login":"jdmonin"}}