Judge's feedback

[ghost]

Placeholder issue for judge's feedback.

[railschallenge-judge01]

Overall, this is pretty solid looking code. Here are a handful of thoughts:

Models

• Could use some comment headers at the top of the methods
• You don't need to_param and to_params, they are not called anywhere
• Consider including db-level null validations for addition data integrity
• responders.rb
  • Responders.assign_responders() - use of to_a is fine here, but with a large set of responders this would not scale
  • Nice use of scope chaining
• emergency.rb
  • Emergency responders() method - since there is a 1:N relationship between responses emergencies, use of the method name responses could be a bit confusing. In this context, a responses methods would generally be expected to return a collection of response objects, not names
  • The use of the after_update callback is OK here, though in general ActiveRecord callbacks can lead to hard to maintain/trace code

Controllers

• Loading class variables up in a before filter isn't the best practice
  • it's laudable to reduce code redundancy, but...
  • In a large-scale app, this approach can lead to convoluted workflows and hard-to-trace bugs

[railschallenge-judge02]

A few more thoughts

General

• You have no documentation. This is optional and not expected for an early submission, but is always welcome in places where explanation is merited.

ErrorsController

• Does this single action merit a separate controller?
• render :show is more idiomatic than single quotes

Emergency

• Some repetition in the validations can be removed
• not sure if full_responses merits a scope for a single use

Responder

• is to_params supposed to be to_param?

[kirkokada]

Thanks for the feedback!

I have just finished implementing all of the suggestions and added comment headers to the models.

One question:

Regarding the full_responses scope in emergency.rb, I moved the call to where(full_response: true) into the emergencies controller. I was under the impression that database queries should be consolidated into the model as much as possible, which was why I made the scope. For simple cases like this one that are not repeated elsewhere, is it kosher to call database queries from the controller?

Also, if someone could point me to a style guide for method comments I'd be much obliged.

[ghost]

Verified score of 114 points on the automated scoring, based on the current master branch.

[ghost]

Your individual score breakdown:

254 points total:

• 114 for passing tests
• 140 for the judges' section