{"action":"created","issue":{"url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7565","labels_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7565/labels{/name}","comments_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7565/comments","events_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7565/events","html_url":"https://github.com/OpenRA/OpenRA/pull/7565","id":59342325,"number":7565,"title":"Refactored MobileInfo, Mobile and Move","user":{"login":"Rydra","id":5826108,"avatar_url":"https://avatars.githubusercontent.com/u/5826108?v=3","gravatar_id":"","url":"https://api.github.com/users/Rydra","html_url":"https://github.com/Rydra","followers_url":"https://api.github.com/users/Rydra/followers","following_url":"https://api.github.com/users/Rydra/following{/other_user}","gists_url":"https://api.github.com/users/Rydra/gists{/gist_id}","starred_url":"https://api.github.com/users/Rydra/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Rydra/subscriptions","organizations_url":"https://api.github.com/users/Rydra/orgs","repos_url":"https://api.github.com/users/Rydra/repos","events_url":"https://api.github.com/users/Rydra/events{/privacy}","received_events_url":"https://api.github.com/users/Rydra/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":37,"created_at":"2015-02-28T13:48:03Z","updated_at":"2015-03-30T04:04:11Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/OpenRA/OpenRA/pulls/7565","html_url":"https://github.com/OpenRA/OpenRA/pull/7565","diff_url":"https://github.com/OpenRA/OpenRA/pull/7565.diff","patch_url":"https://github.com/OpenRA/OpenRA/pull/7565.patch"},"body":"Refactored MobileInfo, Mobile and Move to make them testable in isolation, easier to understand what they do by commenting and restructuring the code making sure to not cause undesired side-effects.\r\n\r\nThe fact of having to break dependencies on a highly coupled system forced me to modify and create several interfaces, that at the same time forced me to change several classes directly and indirectly related to Move, Mobile and MobileInfo.\r\n\r\nAdditionally, I added several unit tests to try how testable they've become.\r\n\r\nI've been comenting throughoutfully the code, I've been manipulating, yet to sum up:\r\n\r\n- By no means this refactor is complete. It should be completed through the span of several PR. If you take a look at the MoveTests, you'll see that I had to mock a load of dependencies in order to make the tests pass successfully. This is a clear smell of Demeter violation. The construction of Move instances should be delegated to some kind of Move factory that can resolve all the move dependencies for him. This, utimately, will make Move easier to undertand.\r\n\r\n- There is no clear barrier and meaning between Mobile and MobileInfo. There are lot of functions that Mobile could deal by himself and yet are delegated to MobileInfo...\r\n\r\n- The crush, block, enterCell, finishmove notifications... are implemented in a way that couples actors to each other. We should think in a way to make this work on a subscribe/publish like the Observer Pattern: http://dotnetcodr.com/2013/08/01/design-patterns-and-practices-in-net-the-observer-pattern/ . A message bus implementation of this would be really smart for the future, and would skyroof the performance.\r\n"},"comment":{"url":"https://api.github.com/repos/OpenRA/OpenRA/issues/comments/87535384","html_url":"https://github.com/OpenRA/OpenRA/pull/7565#issuecomment-87535384","issue_url":"https://api.github.com/repos/OpenRA/OpenRA/issues/7565","id":87535384,"user":{"login":"huwpascoe","id":1812087,"avatar_url":"https://avatars.githubusercontent.com/u/1812087?v=3","gravatar_id":"","url":"https://api.github.com/users/huwpascoe","html_url":"https://github.com/huwpascoe","followers_url":"https://api.github.com/users/huwpascoe/followers","following_url":"https://api.github.com/users/huwpascoe/following{/other_user}","gists_url":"https://api.github.com/users/huwpascoe/gists{/gist_id}","starred_url":"https://api.github.com/users/huwpascoe/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/huwpascoe/subscriptions","organizations_url":"https://api.github.com/users/huwpascoe/orgs","repos_url":"https://api.github.com/users/huwpascoe/repos","events_url":"https://api.github.com/users/huwpascoe/events{/privacy}","received_events_url":"https://api.github.com/users/huwpascoe/received_events","type":"User","site_admin":false},"created_at":"2015-03-30T04:04:11Z","updated_at":"2015-03-30T04:04:11Z","body":"\"but adding unnecessary abstraction is a giant code smell for video game design and any other high speed or low resource environment.\"\r\n\r\nHmm... you have a point there. The extra MOV instructions required to offset the IVTable are orders of magnitude more CPU intensive than the massive tangled web of system.reflection calls, object factories and linq extensions that make up the core of the game, which is totally [not still broken](https://github.com/OpenRA/OpenRA/issues/6818) anyway."}}