{"action":"created","issue":{"url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/issues/20220","repository_url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA","labels_url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/issues/20220/labels{/name}","comments_url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/issues/20220/comments","events_url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/issues/20220/events","html_url":"https://github.com/CleverRaven/Cataclysm-DDA/issues/20220","id":205783653,"number":20220,"title":"Fixing the \"cram every functionality into one of 4 classes\" anti-pattern","user":{"login":"Coolthulhu","id":1878860,"avatar_url":"https://avatars.githubusercontent.com/u/1878860?v=3","gravatar_id":"","url":"https://api.github.com/users/Coolthulhu","html_url":"https://github.com/Coolthulhu","followers_url":"https://api.github.com/users/Coolthulhu/followers","following_url":"https://api.github.com/users/Coolthulhu/following{/other_user}","gists_url":"https://api.github.com/users/Coolthulhu/gists{/gist_id}","starred_url":"https://api.github.com/users/Coolthulhu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Coolthulhu/subscriptions","organizations_url":"https://api.github.com/users/Coolthulhu/orgs","repos_url":"https://api.github.com/users/Coolthulhu/repos","events_url":"https://api.github.com/users/Coolthulhu/events{/privacy}","received_events_url":"https://api.github.com/users/Coolthulhu/received_events","type":"User","site_admin":false},"labels":[{"id":477037040,"url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/labels/Organization","name":"Organization","color":"eeeeee","default":false},{"id":14965562,"url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/labels/Question","name":"Question","color":"990099","default":false},{"id":43875653,"url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/labels/Suggestion%20(Discuss)","name":"Suggestion (Discuss)","color":"FF3399","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2017-02-07T04:31:00Z","updated_at":"2017-02-07T08:01:55Z","closed_at":null,"body":"Many classes are bloated with functionality that could be rewritten using only public methods from said classes.\r\n\r\n`game`, `player`, `map`, `item` - all of those are bloated with functions that interact with multiple unrelated objects at a time, arbitrarily picking one of them as the current `this`, then using mostly or only public methods.\r\nFor example:\r\n* Finding ammo to reload is functionality of both `Character` and `item`, with no clear hierarchy\r\n* `map::collapse_at` implements a whole behavior of collapsing, mostly using only pretty muc only public methods of the class, interacting with `game` (collapsing on critters)\r\n* `map::create_spores` - it creates and infects creatures\r\n* The entire projectile shooting mechanic is a `Creature` method, but could easily be implemented with just a `Creature` argument\r\n\r\nThis looks very much like an anti-pattern, something to avoid. There is no clear responsibility, no encapsulation, bloated headers, multi-thousand line files, no... pattern.\r\n\r\nIs there an obvious course of action? Do we have a plan here?\r\n\r\nI'd do it like this:\r\n* When implementing new function that doesn't have a very clear \"owner\", try to find a neutral ground for it, by writing it as a class-less function inside a namespace\r\n* Groups of functions, such as most functions related to consumption, should likewise be moved to a namespace. Alternatively, a wrapper class or a similar class as `player_morale`\r\n\r\nIs there a good, short guideline that could be a good starting point for designing class interactions? Something that could be linked in PRs with just \"it should look like this: [link], you can move `fun_x` and `fun_y` outside the class because it uses only public methods\".\r\n\r\nEDIT: It looks like https://en.wikipedia.org/wiki/Facade_pattern and https://en.wikipedia.org/wiki/Decorator_pattern could be useful here"},"comment":{"url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/issues/comments/277927406","html_url":"https://github.com/CleverRaven/Cataclysm-DDA/issues/20220#issuecomment-277927406","issue_url":"https://api.github.com/repos/CleverRaven/Cataclysm-DDA/issues/20220","id":277927406,"user":{"login":"Zireael07","id":4525388,"avatar_url":"https://avatars.githubusercontent.com/u/4525388?v=3","gravatar_id":"","url":"https://api.github.com/users/Zireael07","html_url":"https://github.com/Zireael07","followers_url":"https://api.github.com/users/Zireael07/followers","following_url":"https://api.github.com/users/Zireael07/following{/other_user}","gists_url":"https://api.github.com/users/Zireael07/gists{/gist_id}","starred_url":"https://api.github.com/users/Zireael07/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Zireael07/subscriptions","organizations_url":"https://api.github.com/users/Zireael07/orgs","repos_url":"https://api.github.com/users/Zireael07/repos","events_url":"https://api.github.com/users/Zireael07/events{/privacy}","received_events_url":"https://api.github.com/users/Zireael07/received_events","type":"User","site_admin":false},"created_at":"2017-02-07T08:01:55Z","updated_at":"2017-02-07T08:01:55Z","body":"I guess you could enforce a 'no new features' block before 0.D and spend some time cleaning those up before actually shipping 0.D?"}}
{"actor":{"display_login":"Zireael07"}}