Judge's feedback

[ghost]

Placeholder issue for judges to leave feedback

[railschallenge-judge01]

Migrations

• You could add null validations at the db level for additional data integrity

Responder Model

• I'm all for moving magic numbers to constant, but I'm not seeing a lot of value in VALID_CAPACITIES
  • it's used's just once and 1..5 is pretty understandable when inline in a validation

Emergency Model

• Codes can change over time, probably would be better to leave the PK as the id
• full_response_count could be a scope
• The use of the after_update callback is OK here, though in general ActiveRecord callbacks can lead to hard to maintain/trace code

Controllers

• You don't need to_json on your @responder.errors calls
  • Your render_unprocessable_entity() method implicity converts them to json
• The responder_params and emergency_params code is a bit meta, in that it contextually pulls the constants from the top of the file
  • why not go a step further and DRY the param methods up by moving them to a single common method in the base api controller?
• 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

handle_capacity_request is a great example of this:
We know the capacity request comes in on the index action, so the developer comes along and looks at the index action and only sees code to return all responders. The capacity request has completely subverted the original intent of the action. One would have to know to look for before actions to find the actual behavior. In this case it would be better to code a condition into the index action to handle each case

• Since you created a base API controller, id you felt adventurous, you could get a little meta with the page not found actions.

base_api_controller.rb

  def self.page_not_found_actions(actions)
    actions.each do |action|
      define_method action do
        render_not_found
      end
    end
  end

base_api_controller.rb

  def self.page_not_found_actions(actions)
    actions.each do |action|
      define_method action do
        render_not_found
      end
    end
  end

responders_controller.rb

  class RespondersController < BaseApiController
    CREATE_PERMITTED_PARAMS = %w(type name capacity)
    UPDATE_PERMITTED_PARAMS = %w(on_duty)

    before_action :set_responder, only: [:show, :update]
    before_action :handle_capacity_request, only: :index

    page_not_found_actions %w{ new edit destroy }
    ...
  end

There are plenty of ways to handle the page not found problem. Another way would be to set up a catch-all route.

[tigershen23]

@railschallenge-judge01 @railschallenge-judge04 Thanks for the feedback! Going to address some of it; the rest, I'm taking as :+1:.

Emergency Model

Codes can change over time, probably would be better to leave the PK as the id

• This PK is only on the association w/ Responder, which itself is finite, so I'm comfortable using code for convenience unless there are any other big reasons why.

full_response_count could be a scope

• Ah, I thought so too, but scopes need to return a chainable object (able to chain more scopes onto it), and full_responses_count returns an integer.

The use of the after_update callback is OK here, though in general ActiveRecord callbacks can lead to hard to maintain/trace code

• Yup, I'm generally against them as well, but in this case I thought it was harmless enough to not warrant a separate domain object or something else.

Controllers

Loading class variables up in a before filter isn't the best practice

• Agreed. Does this apply to the set_emergency-type methods as well? I was under the impression those are kind of Rails practice.

why not go a step further and DRY the param methods up by moving them to a single common method in the base api controller?

• I think one *_params method per controller is generally considered a good situation. But I'm intrigued; you're suggesting to make it even more meta? :stuck_out_tongue: Could you provide an example for this? The differences are the method name and the required param (responder vs. emergency) - how would I DRY this up?

Since you created a base API controller, id you felt adventurous, you could get a little meta with the page not found actions.

• Adventure is out there! I like your suggestion, gonna take it.

I'm sure you have lots of other reviews to get to, but a re-review would be awesome! Did some decent refactoring.

[railschallenge-judge02]

Some more thoughts. I've attempted to indicate places where I'm giving a personal opinion that may not require any correction, but where discussion would be appreciated.

General

• No views! Your use of activemodel_serializers is a neat choice.
• You have some documentation. This is optional and not expected for an early submission, but is always welcome in places where explanation is merited.
• (subjective) I have a different opinion on meta-programming, which is that it sacrifices readability. It's useful when needed, but there's a balance.
• While the separation of model logic into your Domain module namespace is understandable, the name isn't. (subjective) In addition, there are more files here than needed for this app e.g. Dispatcher has a single method. To me, it feels like you've anticipated the growth of the app before it has grown.
• InstanceUtilities and ClassUtilities are convincing me that you're following a coding style that rigorously removes logic from models. Could you discuss and outline the practical benefit in this case? (subjective) Jumping between files makes the code less readable, which is a different use case (judging) than the client's (using). But a future developer (including yourself, the "senior") will have some overhead just orienting to the location of the separated logic.

BaseApiController

• This logic could be made part of the ApplicationController

EmergenciesController, RespondersController

• Why are the permitted params in constants? They're only used once, so using self.class.const_get and adding a line might merit some justification.

ApplicationHelper

• This is unused, and can be removed.

Emergency

• Some repetition in the validations can be removed
• The use of a proc in a callback is not idiomatic - the logic can be moved to the callback method

Responder

• each_type is a less readable than just iterating through each type

[railschallenge-judge01]

Agreed with Judge 2. Meta programming generally sacrifices readability. I only suggested it here because you already had established a common base api controller and could use meta code to create a DSL-like construct to define not-found routes in the child controllers.

Still, that suggestion was highly subjective. As mentioned, all of that base api logic could also just live in the application controller.

[tigershen23]

@railschallenge-judge01 @railschallenge-judge02 Hi guys, thanks again for the feedback.

I have a different opinion on meta-programming, which is that it sacrifices readability. It's useful when needed, but there's a balance.

Soo where does that leave you with my implementation? I think it's pretty darn useful in the case of making the subclasses and serializers for each different responder type, especially because behavior is the same between them all. I'm interested to hear your thoughts on this. To add a new Responder type, it's trivial to just add a word to Responder::VALID_TYPES, and I think that's a big win.

While the separation of model logic into your Domain module namespace is understandable, the name isn't. (subjective) In addition, there are more files here than needed for this app e.g. Dispatcher has a single method. To me, it feels like you've anticipated the growth of the app before it has grown.

Interesting. I started out with a big Dispatcher class, and as that became more fully formed I saw some clear domain lines forming. I took these and abstracted them into their own classes, according to my interpretation of OO design, and I like how it works out. Each class is good at one thing, and none of them is bloated in the least. Dispatcher is a good abstraction for the controller to work with, and it's one method is a pretty clear set of steps for dispatching an emergency - . What is your issue with the Domain namespace? I believe it adequately describes what it encompasses - the domain logic for my application. Re: anticipating app growth, I see it more as protecting against future changes. I think these smaller classes make it much easier to find where to make a certain change + make it easily.

Why are the permitted params in constants? They're only used once, so using self.class.const_get and adding a line might merit some justification.

Yup, going to refactor this to e.g. emergency_create_params and emergency_update_params methods, which to me seems like the way to go here.

each_type is a less readable than just iterating through each type

Why do you say this? I'm willing to change, just want to get more insight.

InstanceUtilities and ClassUtilities are convincing me that you're following a coding style that rigorously removes logic from models. Could you discuss and outline the practical benefit in this case? (subjective) Jumping between files makes the code less readable, which is a different use case (judging) than the client's (using). But a future developer (including yourself, the "senior") will have some overhead just orienting to the location of the separated logic.

Ah, yes. I think ActiveRecord does a great job at modeling data and interfacing with the database, but I think Rails models take on a bit too much responsibility. Thus, my reasoning is that AR models should be used for validations/scopes/etc, but domain logic can go elsewhere. ClassUtilities and InstanceUtilities are my way of abstracting some domain logic out of the models until clearer domain lines show themselves as the application grows. I'd love to hear your thoughts here; it's a methodology I got from coworkers and have enjoyed using.

Cheers, thank you.

[tigershen23]

^ Piggybacking off of this, I think my submission can be viewed favorably in the sense of flexibility and easiness of change, both of which are crucial when starting out and can shape an application's life. Want to add a new responder? Just put one word in Responder::VALID_TYPES (and maybe add a column to Emergency). Want to change Dispatch logic? Instead of searching through a big Dispatcher, you can tunnel down into the correct place to make those changes, and it should be painless. You point out that I've made too many files, but I'm wondering where the negative is there? The most you'd have to jump through is maybe two or three, and when you get where you want it's clear exactly what that class does. Not trying to be confrontational at all; I'm in this to learn, and I'd love for someone to counter me with something that changes my mind. Thoughts?

[ghost]

Hi @tigershen23 - I saw your request for another round of code review. At this time we're still working through the first round with others who have requested a review. If I have time I'll try to provide a third perspective on your repo.

Thanks for your participation in the challenge!

[ghost]

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