{"action":"created","issue":{"url":"https://api.github.com/repos/loot/loot/issues/516","labels_url":"https://api.github.com/repos/loot/loot/issues/516/labels{/name}","comments_url":"https://api.github.com/repos/loot/loot/issues/516/comments","events_url":"https://api.github.com/repos/loot/loot/issues/516/events","html_url":"https://github.com/loot/loot/issues/516","id":121886072,"number":516,"title":"Improve JS code quality","user":{"login":"WrinklyNinja","id":2430738,"avatar_url":"https://avatars.githubusercontent.com/u/2430738?v=3","gravatar_id":"","url":"https://api.github.com/users/WrinklyNinja","html_url":"https://github.com/WrinklyNinja","followers_url":"https://api.github.com/users/WrinklyNinja/followers","following_url":"https://api.github.com/users/WrinklyNinja/following{/other_user}","gists_url":"https://api.github.com/users/WrinklyNinja/gists{/gist_id}","starred_url":"https://api.github.com/users/WrinklyNinja/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/WrinklyNinja/subscriptions","organizations_url":"https://api.github.com/users/WrinklyNinja/orgs","repos_url":"https://api.github.com/users/WrinklyNinja/repos","events_url":"https://api.github.com/users/WrinklyNinja/events{/privacy}","received_events_url":"https://api.github.com/users/WrinklyNinja/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/loot/loot/labels/enhancement","name":"enhancement","color":"84b6eb"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-12-12T22:57:20Z","updated_at":"2015-12-19T12:13:58Z","closed_at":null,"body":"While I'm mostly happy with the state of the C++ code (tests are little dodgy, as is the UI code as it's untested and a couple of classes are very large), I have very little confidence in the UI's JavaScript code. Some things that need to be done:\r\n\r\n* Decouple the JS from the DOM as much as possible. Where a function needs to interact with the DOM, it should go through a JS abstraction layer, unless the function *only* contains DOM interaction and no other logic (eg. the static text translation function). Event handling (dispatching or listening) on `document` is also OK when used for communicating changes. This is to make as much code unit-testable as possible, and to ease maintenance as the DOM changes.\r\n* Implement everything with a consistent structure. Each JavaScript file should be implemented as a UMD module. All exports should be namespaced into a root `loot` object. Any dependencies should be made explicit in the module, or passed as arguments as required to the functions the module exports.\r\n* Refactor the event handlers, `event.js` is just too long.\r\n* Write unit tests. With the DOM decoupled, a test framework (I'm thinking Mocha, since I had a good experience using it with `jed-gettext-parser`) could mock the abstraction layer functions.\r\n* Remove usage of `Object.observe()`. It has been withdrawn as an ES proposal, and will be removed from Chrome in v49 (only two releases away). Replace it with setters and event firing/listening.\r\n\r\nThis work does not include JS in custom elements, and should be completely Polymer-independent (besides calling Polymer element APIs in the abstraction layer). I should target ECMAScript 2015 (a.k.a. ES6), as that's the latest standardised version and Chrome has good support for it. Non-standard functionality should only be used very carefully to prevent the kind of headaches I've had so far.\r\n\r\nFiles improved so far:\r\n\r\n- [ ] events.js\r\n- [x] game.js\r\n- [x] filters.js - Some functionality is missing though, filters.hiddenPluginNo and filters.hiddenMessageNo are not re-implemented, they need to be re-implemented as separate functions that will count the difference between all and visible data.\r\n- [ ] helpers.js\r\n- [ ] init.js\r\n- [x] l10n.js\r\n- [ ] loot.js\r\n- [x] plugin.js\r\n- [x] translator.js"},"comment":{"url":"https://api.github.com/repos/loot/loot/issues/comments/165980918","html_url":"https://github.com/loot/loot/issues/516#issuecomment-165980918","issue_url":"https://api.github.com/repos/loot/loot/issues/516","id":165980918,"user":{"login":"WrinklyNinja","id":2430738,"avatar_url":"https://avatars.githubusercontent.com/u/2430738?v=3","gravatar_id":"","url":"https://api.github.com/users/WrinklyNinja","html_url":"https://github.com/WrinklyNinja","followers_url":"https://api.github.com/users/WrinklyNinja/followers","following_url":"https://api.github.com/users/WrinklyNinja/following{/other_user}","gists_url":"https://api.github.com/users/WrinklyNinja/gists{/gist_id}","starred_url":"https://api.github.com/users/WrinklyNinja/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/WrinklyNinja/subscriptions","organizations_url":"https://api.github.com/users/WrinklyNinja/orgs","repos_url":"https://api.github.com/users/WrinklyNinja/repos","events_url":"https://api.github.com/users/WrinklyNinja/events{/privacy}","received_events_url":"https://api.github.com/users/WrinklyNinja/received_events","type":"User","site_admin":false},"created_at":"2015-12-19T12:13:58Z","updated_at":"2015-12-19T12:13:58Z","body":"Work is progressing well, though I haven't been testing changes in LOOT itself, and I know some things have been broken. Once `loot.js` has been refactored, that will be the last of the code that was already object-oriented, with the rest consisting of free functions.\r\n\r\nThat will then be a good point at which to fix all breakages, as the fixes may inform decisions on how to refactor the remaining code, and it's not good to just let breakages pile up until the end."}}