{"action":"opened","issue":{"url":"https://api.github.com/repos/mrjaimisra/ArtSup/issues/40","labels_url":"https://api.github.com/repos/mrjaimisra/ArtSup/issues/40/labels{/name}","comments_url":"https://api.github.com/repos/mrjaimisra/ArtSup/issues/40/comments","events_url":"https://api.github.com/repos/mrjaimisra/ArtSup/issues/40/events","html_url":"https://github.com/mrjaimisra/ArtSup/issues/40","id":119294786,"number":40,"title":"Some Code Review Thoughts","user":{"login":"stevekinney","id":251000,"avatar_url":"https://avatars.githubusercontent.com/u/251000?v=3","gravatar_id":"","url":"https://api.github.com/users/stevekinney","html_url":"https://github.com/stevekinney","followers_url":"https://api.github.com/users/stevekinney/followers","following_url":"https://api.github.com/users/stevekinney/following{/other_user}","gists_url":"https://api.github.com/users/stevekinney/gists{/gist_id}","starred_url":"https://api.github.com/users/stevekinney/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stevekinney/subscriptions","organizations_url":"https://api.github.com/users/stevekinney/orgs","repos_url":"https://api.github.com/users/stevekinney/repos","events_url":"https://api.github.com/users/stevekinney/events{/privacy}","received_events_url":"https://api.github.com/users/stevekinney/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2015-11-28T13:49:20Z","updated_at":"2015-11-28T13:49:20Z","closed_at":null,"body":"Overall, solid work. Here are some notes I took while reviewing the code.\r\n\r\n---\r\n\r\nThere is a little bit of code duplication in the `ConfirmController.rb`, the [show][scc] and [create][ccc] actions could be DRY'd up with a single method.\r\n\r\n[scc]: https://github.com/mrjaimisra/ArtSup/blob/master/app/controllers/confirm_controller.rb#L7-L11\r\n[ccc]: https://github.com/mrjaimisra/ArtSup/blob/master/app/controllers/confirm_controller.rb#L15-L19\r\n\r\nI like [this use of caching][dcc].\r\n\r\n[dcc]: https://github.com/mrjaimisra/ArtSup/blob/master/app/controllers/dashboard_controller.rb#L13-L15\r\n\r\nQuick though: Could you have used a `has_many_through` [here][hmt]?\r\n\r\n[hmt]: https://github.com/mrjaimisra/ArtSup/blob/master/app/models/user.rb#L2-L3\r\n\r\n[This seems like you might end up with some conflicts][hello]. It doesn't seem like something that should be in the production code and might of been something you were using early on. You're probably better off with a random hash.\r\n\r\n[hello]: https://github.com/mrjaimisra/ArtSup/blob/master/app/models/user.rb#L28\r\n\r\nWhy is [this empty jQuery function][empt] still in your code base? Also, why are those empty `*.coffee` files still in there?\r\n\r\n[empt]: https://github.com/mrjaimisra/ArtSup/blob/master/app/assets/javascripts/application.js#L19-L20\r\n\r\nIf you've already wrapped `this` in jQuery and cached the result in `amazonButton`, then [you don't need to keep re-wrapping `$(this)`][$this].\r\n\r\n[$this]: https://github.com/mrjaimisra/ArtSup/blob/master/app/assets/javascripts/application.js#L24-L25\r\n\r\nUsing `<span>` tags with semantic classes probably would have saved you the hassle of having to do [this][split], as well made your code more readable.\r\n  \r\n[split]: https://github.com/mrjaimisra/ArtSup/blob/master/app/assets/javascripts/application.js#L25-L27\r\n\r\nYou shouldn't need to [chain `find` methods in jQuery][findfind]. Consider the following:\r\n\r\n```js\r\n$(this).closest(\".wishlistItem\").find(\".wishlistItemImage img\").attr('src');\r\n```\r\n\r\nThat said, if you just classed the image itself, you probably wouldn't even have to go that far.\r\n\r\nI probably would have gone for a `$.post` [here][post].\r\n\r\n[post]: https://github.com/mrjaimisra/ArtSup/blob/master/app/assets/javascripts/application.js#L42-L54\r\n\r\n\r\n[findfind]: https://github.com/mrjaimisra/ArtSup/blob/master/app/assets/javascripts/application.js#L28\r\n"}}