Comparison Controller

[bkingon]

@jierasmus27 with regards to the comparisons controller: https://github.com/jierasmus27/star_wars_battle/blob/master/app/controllers/comparison_controller.rb I have a few comments and thoughts

• How would you go about making it more RESTful?
• You are rescuing the standard error and assigning the variable e when something goes wrong, however, you are just rendering an error page and not even using the assigned e variable. How could we handle the errors better. Maybe even without showing an entire error page?

In terms of the tests:

• You have some factory girl factories added, however there are also structs for params inside of the actual spec files. How could those be refactored to possibly be made more re-usable but also to slim down the spec files a bit?

[jierasmus27]

Hi @bkingon

Thank you very much for the feedback. I have refactored the ComparisonController to be more RESTful substituting the list method for an index and the two compare methods down to only one compare method which would be 'compare/:entity' with the primary and secondary id's passed as params.

Additionally I have cleaned up the controller and model tests by implementing the factorygirl factories which has yielded a much cleaner solution.

I will next focus on general error handling and specifically the error in the API Gem which as you point out was basically a catch-all, which was left as is mainly due to time constraints.

[jierasmus27]

Hi @bkingon

For the SwapiRb gem, I have added custom errors for the 3 main causes of errors (getting an error connecting to the destination API, getting an unexpected response code and issues parsing the response body to JSON) so the consuming app could log and handle errors better.

For the main app, I have made implemented a change where the character or starship comparison functionality would be disabled if the corresponding api calls were unsuccessful, with the errors handled individually in the models and flash messages instead of an entire error page. Therefore if one of the API calls for index were successful, the functionality would still be available with an appropriate message displayed to indicate this.

[bkingon]

Hi @jierasmus27

Looks much cleaner already :) I am not looking for you to make any further changes, but this article is nice idea around very clean and RESTful controllers, with sub controllers. May be worth a ready if you have not already come across it: http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/

I like how much easier it is to read the test now as well. My only comment there would be that you should be a bit careful to remove unused files if you make use of generators.

Lastly, I would just say that you could clean up your gem file, and remove gems that are not necessary. Seen as your app does not appear to require a database, you would not need the sqlite3 gem, however, removing that now would require some configuration updates.

Otherwise it is looking great, and a fun app to play with :smile:

[jierasmus27]

@bkingon thanks for the feedback and the article, as you say it's a very nice improvement to keep the controllers nice and clean.