{"action":"created","issue":{"url":"https://api.github.com/repos/AUCSC/sac/issues/47","repository_url":"https://api.github.com/repos/AUCSC/sac","labels_url":"https://api.github.com/repos/AUCSC/sac/issues/47/labels{/name}","comments_url":"https://api.github.com/repos/AUCSC/sac/issues/47/comments","events_url":"https://api.github.com/repos/AUCSC/sac/issues/47/events","html_url":"https://github.com/AUCSC/sac/pull/47","id":142684854,"number":47,"title":"Notes on PresentationController.php","user":{"login":"quasipickle","id":1377105,"avatar_url":"https://avatars.githubusercontent.com/u/1377105?v=3","gravatar_id":"","url":"https://api.github.com/users/quasipickle","html_url":"https://github.com/quasipickle","followers_url":"https://api.github.com/users/quasipickle/followers","following_url":"https://api.github.com/users/quasipickle/following{/other_user}","gists_url":"https://api.github.com/users/quasipickle/gists{/gist_id}","starred_url":"https://api.github.com/users/quasipickle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/quasipickle/subscriptions","organizations_url":"https://api.github.com/users/quasipickle/orgs","repos_url":"https://api.github.com/users/quasipickle/repos","events_url":"https://api.github.com/users/quasipickle/events{/privacy}","received_events_url":"https://api.github.com/users/quasipickle/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-03-22T15:26:35Z","updated_at":"2016-03-23T23:55:29Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/AUCSC/sac/pulls/47","html_url":"https://github.com/AUCSC/sac/pull/47","diff_url":"https://github.com/AUCSC/sac/pull/47.diff","patch_url":"https://github.com/AUCSC/sac/pull/47.patch"},"body":"You're calling `findOrFail()`, but not handling the situation of it failing.  Those calls should be wrapped in a try...catch() and methods should be restructured to handle a failure.  Same applies to elsewhere in this class that uses `findOrFail()`.\r\n\r\nYou also need to handle the situation of a save() or delete() or other database action failing.  Just a simple `flash()->error()` would suffice.\r\n\r\n**index()**\r\nCalling `array_unshift()` like that is fine as long as the PresentationType records start at ID 1, and are consecutive. What happens when/if a PresentationType gets deleted, then another one added? The auto-increment index doesn't get reset on deletion, so the new PresentationType record will get the next auto-increment value. In this situation, you could have PresentationType records with ids 1-9, and 11. The position in the array for the last record will no longer match the ID.\r\n\r\nI suggest not using the array index as a substitute for the ID, and just pull the ID out of the PresentationType object in your view.\r\n\r\n**create()**\r\n`$presentation->type` and `$presentation->course` are both initialized to impossible values - which is good.  However, they use different impossible values.  While that will work, it may be a little tricky elsewhere if you're testing for a value and are unsure if you should check against NULL or against -1.  I think it's best to use only NULL for fields that don't have a valid value.\r\n\r\n\r\n**prepare_form()**\r\nYou're assigning `Auth::user()` to `$user`, but not using `$user` anywhere.\r\n\r\n**setOwner() is problematic for a few reasons.**\r\n1) You're setting the owner, which is a user id, but you're passing it a Presentation object.  This method then has to pull apart a Presentation object to get at what it needs.  A better plan would be to just have `setOwner()` accept a user id directly.  This will allow you to use this method even in situations where you don't already have a Presentation object\r\n\r\n2) Not only are you setting the owner id, you're also setting 1 of 2 name fields.  This is called a \"side-effect\" and is a bugger when trying to debug.  \"`setOwner()`\" implies the owner will be set - it's not obvious that the name will also be set.  You should create a separate method for setting the name.  Yes, it will create another method call, but there won't be any hidden consequences to the method calls.\r\n\r\n3) The fact that it's a method of the Presentation class, but operates on a passed Presentation object is weird.  Why not just make it act on the current Presentation object?  So rather than calling `$presentation = $this->setOwner($presentation)`, you could call `$presentation->setOwner($owner_id)`  You would have to make `setOwner()` public, but that's no big deal.\r\n\r\n4) This is a stylistic issue, but you've named this method with camel-case, but the other multi-word methods are words separated with underscores.  Best practice is to pick one or the other."},"comment":{"url":"https://api.github.com/repos/AUCSC/sac/issues/comments/200587030","html_url":"https://github.com/AUCSC/sac/pull/47#issuecomment-200587030","issue_url":"https://api.github.com/repos/AUCSC/sac/issues/47","id":200587030,"user":{"login":"ParleyMartins","id":6959616,"avatar_url":"https://avatars.githubusercontent.com/u/6959616?v=3","gravatar_id":"","url":"https://api.github.com/users/ParleyMartins","html_url":"https://github.com/ParleyMartins","followers_url":"https://api.github.com/users/ParleyMartins/followers","following_url":"https://api.github.com/users/ParleyMartins/following{/other_user}","gists_url":"https://api.github.com/users/ParleyMartins/gists{/gist_id}","starred_url":"https://api.github.com/users/ParleyMartins/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ParleyMartins/subscriptions","organizations_url":"https://api.github.com/users/ParleyMartins/orgs","repos_url":"https://api.github.com/users/ParleyMartins/repos","events_url":"https://api.github.com/users/ParleyMartins/events{/privacy}","received_events_url":"https://api.github.com/users/ParleyMartins/received_events","type":"User","site_admin":false},"created_at":"2016-03-23T23:55:29Z","updated_at":"2016-03-23T23:55:29Z","body":">You're calling findOrFail(), but not handling the situation of it failing. Those calls should be wrapped in a try...catch() and methods should be restructured to handle a failure. Same applies to elsewhere in this class that uses findOrFail().\r\n\r\nfindOrFail is Laravel helper that redirects the user to a 404 page when the model is not used. We do have to update our 404 page, because now it's a default Laravel one.\r\n\r\n>index()\r\n>Calling array_unshift() like that ...\r\n\r\nWe fixed that already (your code is a little bit outdated, sorry). We are accessing the presentation types by their relationship with presentation. That was a workaround (wrong, I know) to make it work while I didn't know how to access the type from a given presentation.\r\n\r\n>I suggest not using the array index as a substitute for the ID, and just pull the ID out of the PresentationType object in your view.\r\n\r\nWe will check that.\r\n\r\n>create()\r\n>$presentation->type and $presentation->course are both initialized to impossible values - which is good. However, they use different impossible values. While that will work, it may be a little tricky elsewhere if you're testing for a value and are unsure if you should check against NULL or against -1. I think it's best to use only NULL for fields that don't have a valid value.\r\n\r\nGood advice, I did this in the [new pull request](https://github.com/AUCSC/sac/pull/51)\r\n\r\n>prepare_form()\r\n>You're assigning Auth::user() to $user, but not using $user anywhere.\r\n\r\nThis is also fixed [here](https://github.com/AUCSC/sac/pull/51)\r\n\r\n>setOwner() is problematic for a few reasons.\r\n\r\nI won't even cite the rest because I totally agree with with you. It was a horrible workaround that I did while I couldn't think of something better. While working on #51 I realized that I was also changing the owner everytime I called that method. So, I removed it and the only place the owner is set is on the `store()` as it should be.\r\n"}}