{"action":"created","issue":{"url":"https://api.github.com/repos/phetsims/unit-rates/issues/120","repository_url":"https://api.github.com/repos/phetsims/unit-rates","labels_url":"https://api.github.com/repos/phetsims/unit-rates/issues/120/labels{/name}","comments_url":"https://api.github.com/repos/phetsims/unit-rates/issues/120/comments","events_url":"https://api.github.com/repos/phetsims/unit-rates/issues/120/events","html_url":"https://github.com/phetsims/unit-rates/issues/120","id":181025359,"number":120,"title":"fundamental architectural issues","user":{"login":"pixelzoom","id":3046552,"avatar_url":"https://avatars.githubusercontent.com/u/3046552?v=3","gravatar_id":"","url":"https://api.github.com/users/pixelzoom","html_url":"https://github.com/pixelzoom","followers_url":"https://api.github.com/users/pixelzoom/followers","following_url":"https://api.github.com/users/pixelzoom/following{/other_user}","gists_url":"https://api.github.com/users/pixelzoom/gists{/gist_id}","starred_url":"https://api.github.com/users/pixelzoom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pixelzoom/subscriptions","organizations_url":"https://api.github.com/users/pixelzoom/orgs","repos_url":"https://api.github.com/users/pixelzoom/repos","events_url":"https://api.github.com/users/pixelzoom/events{/privacy}","received_events_url":"https://api.github.com/users/pixelzoom/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/phetsims/unit-rates/labels/priority:1-top","name":"priority:1-top","color":"FF007F"}],"state":"open","locked":false,"assignee":{"login":"pixelzoom","id":3046552,"avatar_url":"https://avatars.githubusercontent.com/u/3046552?v=3","gravatar_id":"","url":"https://api.github.com/users/pixelzoom","html_url":"https://github.com/pixelzoom","followers_url":"https://api.github.com/users/pixelzoom/followers","following_url":"https://api.github.com/users/pixelzoom/following{/other_user}","gists_url":"https://api.github.com/users/pixelzoom/gists{/gist_id}","starred_url":"https://api.github.com/users/pixelzoom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pixelzoom/subscriptions","organizations_url":"https://api.github.com/users/pixelzoom/orgs","repos_url":"https://api.github.com/users/pixelzoom/repos","events_url":"https://api.github.com/users/pixelzoom/events{/privacy}","received_events_url":"https://api.github.com/users/pixelzoom/received_events","type":"User","site_admin":false},"assignees":[{"login":"pixelzoom","id":3046552,"avatar_url":"https://avatars.githubusercontent.com/u/3046552?v=3","gravatar_id":"","url":"https://api.github.com/users/pixelzoom","html_url":"https://github.com/pixelzoom","followers_url":"https://api.github.com/users/pixelzoom/followers","following_url":"https://api.github.com/users/pixelzoom/following{/other_user}","gists_url":"https://api.github.com/users/pixelzoom/gists{/gist_id}","starred_url":"https://api.github.com/users/pixelzoom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pixelzoom/subscriptions","organizations_url":"https://api.github.com/users/pixelzoom/orgs","repos_url":"https://api.github.com/users/pixelzoom/repos","events_url":"https://api.github.com/users/pixelzoom/events{/privacy}","received_events_url":"https://api.github.com/users/pixelzoom/received_events","type":"User","site_admin":false},{"login":"ariel-phet","id":4051174,"avatar_url":"https://avatars.githubusercontent.com/u/4051174?v=3","gravatar_id":"","url":"https://api.github.com/users/ariel-phet","html_url":"https://github.com/ariel-phet","followers_url":"https://api.github.com/users/ariel-phet/followers","following_url":"https://api.github.com/users/ariel-phet/following{/other_user}","gists_url":"https://api.github.com/users/ariel-phet/gists{/gist_id}","starred_url":"https://api.github.com/users/ariel-phet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ariel-phet/subscriptions","organizations_url":"https://api.github.com/users/ariel-phet/orgs","repos_url":"https://api.github.com/users/ariel-phet/repos","events_url":"https://api.github.com/users/ariel-phet/events{/privacy}","received_events_url":"https://api.github.com/users/ariel-phet/received_events","type":"User","site_admin":false},{"login":"arouinfar","id":8419308,"avatar_url":"https://avatars.githubusercontent.com/u/8419308?v=3","gravatar_id":"","url":"https://api.github.com/users/arouinfar","html_url":"https://github.com/arouinfar","followers_url":"https://api.github.com/users/arouinfar/followers","following_url":"https://api.github.com/users/arouinfar/following{/other_user}","gists_url":"https://api.github.com/users/arouinfar/gists{/gist_id}","starred_url":"https://api.github.com/users/arouinfar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arouinfar/subscriptions","organizations_url":"https://api.github.com/users/arouinfar/orgs","repos_url":"https://api.github.com/users/arouinfar/repos","events_url":"https://api.github.com/users/arouinfar/events{/privacy}","received_events_url":"https://api.github.com/users/arouinfar/received_events","type":"User","site_admin":false}],"milestone":null,"comments":1,"created_at":"2016-10-04T22:51:48Z","updated_at":"2016-10-05T15:16:27Z","closed_at":null,"body":"As I dig deep into this sim, I'm seeing several fundamental architectural issues.  And I believe they are symptomatic of not having the design sufficiently completed before beginning implementation - the developer obviously had to adapt to evolving requirements.\r\n\r\nThese issues are all somewhat related, so I'm going to try to summarize them all here.\r\n\r\n(1) Logic that involves testing the item type\r\n\r\n I see large `switch` and `if-then-else` statements (typically on some property related to an item type) that are indicative of fundamental problems with how the model is structured.   For example, `NumberLine` should be agnostic of what items its related to. Instead, I see this at line 83, where NumberLine has logic for every item type:\r\n\r\n```js\r\n      switch( itemType ) {\r\n        case ItemData.APPLES.type:\r\n          maxUnits = ItemData.APPLES.maxUnit;\r\n          break;\r\n        case ItemData.LEMONS.type:\r\n          maxUnits = ItemData.LEMONS.maxUnit;\r\n          break;\r\n        case ItemData.ORANGES.type:\r\n          maxUnits = ItemData.ORANGES.maxUnit;\r\n          break;\r\n        case ItemData.PEARS.type:\r\n          maxUnits = ItemData.PEARS.maxUnit;\r\n          break;\r\n        case ItemData.CARROTS.type:\r\n          maxUnits = ItemData.CARROTS.maxUnit;\r\n          break;\r\n        case ItemData.CUCUMBERS.type:\r\n          maxUnits = ItemData.CUCUMBERS.maxUnit;\r\n          break;\r\n        case ItemData.POTATOES.type:\r\n          maxUnits = ItemData.POTATOES.maxUnit;\r\n          break;\r\n        case ItemData.TOMATOES.type:\r\n          maxUnits = ItemData.TOMATOES.maxUnit;\r\n          break;\r\n        case ItemData.PURPLE_CANDY.type:\r\n          maxUnits = ItemData.PURPLE_CANDY.maxUnit;\r\n          break;\r\n        case ItemData.RED_CANDY.type:\r\n          maxUnits = ItemData.RED_CANDY.maxUnit;\r\n          break;\r\n        case ItemData.GREEN_CANDY.type:\r\n          maxUnits = ItemData.GREEN_CANDY.maxUnit;\r\n          break;\r\n        case ItemData.BLUE_CANDY.type:\r\n          maxUnits = ItemData.BLUE_CANDY.maxUnit;\r\n          break;\r\n        default:\r\n          assert && assert( false, 'Cannot get max unit of unrecognized type' );\r\n      }\r\n```\r\n\r\nThis obviously isn't extensive - it's not going to work for new item types, or for race cars. And similar `switch` or `if-then-else` statements appear in (too) many other places:\r\n\r\n> Shelf, line 63\r\nItemFactoryNode, line 71\r\nChallenges, lines 182, 300, 378\r\nItemComboBox, lines 62, 126\r\nShoppingScreenView, lines 130, 165\r\nItemRateNode, line 229\r\n\r\nAdding a new item type would involve visiting all of the above files - that's a serious problem. Adding a new item type should be a matter of completing 1 specification of that item type, and adding it in 1 place.\r\n\r\n(2) Item type information is not encapsulated. \r\n\r\nInformation about item types is spread out across multiple data structures and program logic. `ItemData` contains some of the description for each item type. `ChallengeData` contains some more of it.  And program logic (as identified in (1) above) contains functionality that should be encapsulated with each item type.  This makes it difficult to add a new item type, or get a clear picture of everything relevant to an item type. \r\n\r\n(3) Responsibilities are misplaced.\r\n\r\nItem types are supposed to retain their state, and when we switch between item types, we see (for example) the state of the number line for that type.  This would suggest that each item type should have its own associated number line.  Instead, there is a single NumberLine instance that attempts to keep track of the relates state for each item type.  This is misplaced responsibility. An item type should be responsible for maintaining its state.\r\n\r\nThere are also many view Nodes that implement `reset`. Typically, the model and a set of view-specific Properties are all that needs to be reset, and the view updates accordingly. So this is an indication that these Nodes are likely responsible for things that they shouldn't be.\r\n\r\n(4) Unnecessary mutability\r\n\r\nInstead of 1 NumberLine per item type, there is 1 NumberLine instance that mutates based on the item type.  Most of the view components are similar; there is 1 item combo box that changes what it displays based on what item type is selected.   This is complicates and difficult to get right.  Each item type should have its own set of model and view components, and we see the ones that correspond to the item type that is selected."},"comment":{"url":"https://api.github.com/repos/phetsims/unit-rates/issues/comments/251705444","html_url":"https://github.com/phetsims/unit-rates/issues/120#issuecomment-251705444","issue_url":"https://api.github.com/repos/phetsims/unit-rates/issues/120","id":251705444,"user":{"login":"ariel-phet","id":4051174,"avatar_url":"https://avatars.githubusercontent.com/u/4051174?v=3","gravatar_id":"","url":"https://api.github.com/users/ariel-phet","html_url":"https://github.com/ariel-phet","followers_url":"https://api.github.com/users/ariel-phet/followers","following_url":"https://api.github.com/users/ariel-phet/following{/other_user}","gists_url":"https://api.github.com/users/ariel-phet/gists{/gist_id}","starred_url":"https://api.github.com/users/ariel-phet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ariel-phet/subscriptions","organizations_url":"https://api.github.com/users/ariel-phet/orgs","repos_url":"https://api.github.com/users/ariel-phet/repos","events_url":"https://api.github.com/users/ariel-phet/events{/privacy}","received_events_url":"https://api.github.com/users/ariel-phet/received_events","type":"User","site_admin":false},"created_at":"2016-10-05T15:16:27Z","updated_at":"2016-10-05T15:16:27Z","body":"Discussed over email. Current plan is to have @pixelzoom do about ~20 hours of work in addressing these issues, and then re-discuss. \r\n\r\n"}}
{"actor":{"display_login":"ariel-phet"}}