Judge's feedback

[ghost]

Placeholder issue for judge's feedback.

[railschallenge-judge01]

config/application.rb

• config.autoload_paths += Dir[Rails.root.join('app', 'models', 'responders')] is not necessary - there is no app/models/responders folder
• You could get rid of config.autoload_paths += Dir[Rails.root.join('app', 'helpers', 'dispatchers')] if you made the module strucure of the dispatcher files match the folders

Migrations

• consider adding indexes to fields you query against (where, order, etc)

Responder model

• the relation to emergencies would be cleaner and require less code if you left the default PK to id rather than code
• the to scope in this context: f = to(type) doesn't provide a lot of meaning as to what the code is doing
• types and sum_capacity could be scopes

  scope :types, -> { uniq.pluck(:type) }
  scope :sum_capacity, -> { pluck(:capacity).reduce(0, :+) }

Emergency Model

• I like the enumeration here: SEVERITY_TYPES.each do |type|, however it does to some degree reduce readability as one has to parse out what the severity type are to fully understand the code
  • Additionally, the enumerator require three lines - without the enumerator the code would require three lines - so the code doesn't really compress
• The use of a before filter is OK here, though in general over-use of ActiveRecord callbacks can lead to hard to maintain/trace code
  • alternatively, you can make a dedicate method that saves and frees the responders
• full_responses() can be a scope
• use of the code field for the PK/FK overcomplicates the solution
• Rails helper files are meant to be called from views to add visual decoration to data, and should never be called from inside a model
  • Instead, just add the look_for_responder() method to the Emergency model
  • Or, if you want to keep the model thin, make it a concern or a PORO in /lib

Controllers

• Don't load up a class variable in the before filter
  • 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
• You probably don't need comment headers for the public controller actions

[alexlag]

First of all, thanks for your input!

I've made some changes according to the list, mostly moving dispatchers around. I still don't see them as concerns, /lib is still needed to be put in autoload_paths, so I put them in models/dispatcher. Is that approach okay?

And I feel somewhat conflicted about other points or just have questions:

• consider adding indexes to fields you query against (where, order, etc)

Now I have all foreign and natural keys covered, plus columns I use in scopes. Does adding index for column makes it faster to reduce over it?

• Those additional scopes.

Scopes seems to be the exact same as the class methods, and I think scopes are worth writing when actually filtering a subset of instances. Am I wrong?

• About keys in relations.

I am stuck in the middle with those: on the one hand system has obvious natural keys(code and name), so there is no need for usual default id, on the other rejecting those yield dirtier code. So in such scenarios it's better to be redundant and keep numeric ids?

• Enumeration in model.

I like SEVERITY_TYPES constant because it makes emergency more flexible - in case of new KittenResponder you only need to change constant and add one migration. I think it should be possible to even avoid hard-coding it - something like Emergency.column_names.select { |x| x.ends_with?('severity')}. That's a bad idea?

• Before filter in controllers.

Moving load of variable to methods would be a nobrainer, if setter didn't have second line throwing exception. I could write in one line with or, but that's offense to rubocop. I can just call setter in each action, would that be better?

[railschallenge-judge02]

A few other comments:

General

• You have some informal documentation. It's not expected, especially for an early submission, but would be helpful where it is merited.

EmergenciesController, RespondersController

• for index and show the render lines are explicit, but not necessary
• the comment on private params methods is unnecessary

Emergency

• Could you please explain your use of Responder.none?

Dispatcher classes

• It's interesting that you chose to code 3 different ones, but use only one.
• NoDispatchError is only defined in one of the classes, meaning that even though it isn't used, it needs to stick around.

[alexlag]

Thank you for your comments!
Fixed some of the things you mentioned

• About Responder.none - I guess it's unnecessary here, especially when I am already returning empty array in dispatchers
• Actually, 2 are used - Naive calls Trivial via super . NaiveGreedy passes tests and probably works faster so I kept it. But now it looks like it works incorrectly, so I removed it.

[ghost]

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

[ghost]

Your score breakdown:

236.5 points total

• 114 for passing tests
• 122.5 for the judge's section

Hope to see you on the next challenge!