{"action":"created","issue":{"url":"https://api.github.com/repos/baardbaard/bb-twitterfeed/issues/14","repository_url":"https://api.github.com/repos/baardbaard/bb-twitterfeed","labels_url":"https://api.github.com/repos/baardbaard/bb-twitterfeed/issues/14/labels{/name}","comments_url":"https://api.github.com/repos/baardbaard/bb-twitterfeed/issues/14/comments","events_url":"https://api.github.com/repos/baardbaard/bb-twitterfeed/issues/14/events","html_url":"https://github.com/baardbaard/bb-twitterfeed/issues/14","id":171963693,"number":14,"title":"Code review and feedback","user":{"login":"Rarst","id":737584,"avatar_url":"https://avatars.githubusercontent.com/u/737584?v=3","gravatar_id":"","url":"https://api.github.com/users/Rarst","html_url":"https://github.com/Rarst","followers_url":"https://api.github.com/users/Rarst/followers","following_url":"https://api.github.com/users/Rarst/following{/other_user}","gists_url":"https://api.github.com/users/Rarst/gists{/gist_id}","starred_url":"https://api.github.com/users/Rarst/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Rarst/subscriptions","organizations_url":"https://api.github.com/users/Rarst/orgs","repos_url":"https://api.github.com/users/Rarst/repos","events_url":"https://api.github.com/users/Rarst/events{/privacy}","received_events_url":"https://api.github.com/users/Rarst/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-08-18T18:04:34Z","updated_at":"2016-08-18T19:02:48Z","closed_at":null,"body":"Hey, getting to that code review as you asked. :)\r\n\r\n1. You require class files explicitly. Fine for a plugin small enough, but generally this is something Composer autoload should handle. It can pick up arbitrary classes with classmap.\r\n2. `bbtf()` function seems is out of place. The name is not descriptive and it just passes call through.\r\n3. Singleton is bad pattern. In WP code it's usually remnant of faux–OOP, which uses classes as fake namespacing. In modern code anything singleton does dependency injection will do better (not that it's necessary for plugin this small).\r\n4. Somewhat related to previous point the single main class has too much going on. Things like translations set up are not logically related to Twitter functionality. Again, it's a small plugin _now_, but these things grow fast. Classes should have clear separated responsibility to what they do.\r\n5. Some `$twitter_error` seems to miss `$this->` here and there.\r\n6. `build_tweets_list()` is bulky. if/else should typically be early returns instead and count of nesting should be minimal.\r\n7. HTML templating seems to not have enough escaping and trusting to a third party input (API returns). Personally I like to implement plugin templating with Mustache, but yet again — small enough plugin to bother with proper templating engine probably."},"comment":{"url":"https://api.github.com/repos/baardbaard/bb-twitterfeed/issues/comments/240823457","html_url":"https://github.com/baardbaard/bb-twitterfeed/issues/14#issuecomment-240823457","issue_url":"https://api.github.com/repos/baardbaard/bb-twitterfeed/issues/14","id":240823457,"user":{"login":"Rarst","id":737584,"avatar_url":"https://avatars.githubusercontent.com/u/737584?v=3","gravatar_id":"","url":"https://api.github.com/users/Rarst","html_url":"https://github.com/Rarst","followers_url":"https://api.github.com/users/Rarst/followers","following_url":"https://api.github.com/users/Rarst/following{/other_user}","gists_url":"https://api.github.com/users/Rarst/gists{/gist_id}","starred_url":"https://api.github.com/users/Rarst/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Rarst/subscriptions","organizations_url":"https://api.github.com/users/Rarst/orgs","repos_url":"https://api.github.com/users/Rarst/repos","events_url":"https://api.github.com/users/Rarst/events{/privacy}","received_events_url":"https://api.github.com/users/Rarst/received_events","type":"User","site_admin":false},"created_at":"2016-08-18T19:02:48Z","updated_at":"2016-08-18T19:02:48Z","body":"See Composer docs on autoload in general and classmap specifically: https://getcomposer.org/doc/04-schema.md#classmap"}}
{"actor":{"display_login":"Rarst"}}