Judges' feedback

[ghost]

Placeholder issue for judges' feedback.

[ghost]

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

[ghost]

Data model and DB:

• [x] We've seen a couple different approaches for linking responders and emergencies. Some people use an emergency_id column as the foreign key, while others use emergency_code as a foreign key to the emergencies table. It looks like you have both, so I would suggestion picking one and removing the other column that you won't then need.
• Models themselves are kept clean and straightforward.

Dispatching:

• [ ] These lines seem to add extra methods where there don't need to be any. You could rewrite this method in the following way to avoid this extra code:

  def call
    assign_responders!('Fire', emergency.fire_severity) if emergency.fire_severity > 0
    assign_responders!('Police', emergency.police_severity) if emergency.police_severity > 0
    assign_responders!('Medical', emergency.medical_severity) if emergency.medical_severity > 0
    self
  end

This implementation avoids the extra methods, keeps your line length within the rubocop restrictions, and seems just as readable.

ResponderCapacity

• Looks like a reasonable solution. Could also do this with scopes on the models, but I think your solution keeps the capacity logic in one place
• I don't really see the purpose of this method, since it doesn't clarify what's being done, or save on total code. Why not just use respond_with Responder.all ? They are functionally equivalent, and not adding the extra method makes the controller shorter and less complex.

Parameter handling

• Seems like you could DRY up your parameter handling. You have permitted parameter logic in all three of the RespondersController, 'EmergenciesController, and ApplicationController. In all I could 52 or 53 lines of code that are somehow involved in parameter handling, when instead you could either:
• inline parameter handling in each action OR
• keep all logic for parameter handling for a given controller contained within just that controller

...either of which would result is less total code and complexity.

404 handling and routes:

• I don't see the need of having a dedicated ErrorsController. This could instead be a method on the ApplicationController that replies with a static response that all descendent controllers could use.
• Can you explain the decision to use the NotNewId class, and its placement into the config/routes.rb definitions?

active_model_serializers

• Fine use of this. Many other competitors also went this route rather than having dedicated view templates. Appears to clean up your controller code quite a bit.

[sb8244]

Few small things. Thanks for the feedback!

https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/responders_controller.rb#L37 i do this because it makes it very easy to know the concept your working with instead of technical details. This is extremely obvious when scopes are introduced.

The way you are sending new in the tests required that constraint on routes. I may be able to revisit this.

Errors controller is standard. Having an inherited method means that the controllers need to worry about 404 handling. Composition over inheritance in some ways. I highly suggest not inheriting a method like that. In the long run it will produce poorer habits.

Thanks for the rest of the feedback. I agree with it.

Best Steve

On Saturday, April 25, 2015, railschallenge judge #04 < notifications@github.com> wrote:

Data model and DB:

• We've seen a couple different approaches for linking responders and emergencies. Some people use an emergency_id column as the foreign key, while others use emergency_code as a foreign key to the emergencies table. It looks like you have both https://github.com/sb8244/railschallenge-city-watch/blob/master/db/schema.rb#L27, so I would suggestion picking one and removing the other column that you won't then need.
• Models themselves are kept clean and straightforward.

Dispatching:

• These lines https://github.com/sb8244/railschallenge-city-watch/blob/master/app/models/emergency_dispatch.rb#L48-L58 seem to add extra methods where there don't need to be any. You could rewrite this method https://github.com/sb8244/railschallenge-city-watch/blob/master/app/models/emergency_dispatch.rb#L6 in the following way to avoid this extra code:

  def call assign_responders!('Fire', emergency.fire_severity) if emergency.fire_severity > 0 assign_responders!('Police', emergency.police_severity) if emergency.police_severity > 0 assign_responders!('Medical', emergency.medical_severity) if emergency.medical_severity > 0

  self end

This implementation avoids the extra methods, keeps your line length within the rubocop restrictions, and seems just as readable.

ResponderCapacity

• Looks like a reasonable solution. Could also do this with scopes on the models, but I think your solution keeps the capacity logic in one place
• I don't really see the purpose of this method https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/responders_controller.rb#L37, since it doesn't clarify what's being done, or save on total code. Why not just use respond_with Responder.all ? They are functionally equivalent, and not adding the extra method makes the controller shorter and less complex.

Parameter handling

-

Seems like you could DRY up your parameter handling. You have permitted parameter logic in all three of the RespondersController https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/responders_controller.rb#L25-L35, 'EmergenciesController https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/emergencies_controller.rb#L29-L35, and ApplicationController https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/application_controller.rb#L9-L43. In all I could 52 or 53 lines of code that are somehow involved in parameter handling, when instead you could either:

inline parameter handling in each action OR

• keep all logic for parameter handling for a given controller contained within just that controller

...either of which would result is less total code and complexity.

404 handling and routes:

• I don't see the need of having a dedicated ErrorsController. This could instead be a method on the ApplicationController that replies with a static response that all descendent controllers could use.
• Can you explain the decision to use the NotNewId class, and its placement into the config/routes.rb https://github.com/sb8244/railschallenge-city-watch/blob/master/config/routes.rb#L8 definitions?

active_model_serializers

• Fine use of this. Many other competitors also went this route rather than having dedicated view templates. Appears to clean up your controller code quite a bit.

— Reply to this email directly or view it on GitHub https://github.com/sb8244/railschallenge-city-watch/issues/1#issuecomment-96264084 .

Steve Bussey •SalesLoft

•Software Engineer

e steve.bussey@salesloft.com w salesloft.com http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

a 3423 Piedmont Rd NE, Atlanta, GA 30305

[image: Facebook] https://www.facebook.com/SalesLoft [image: Twitter] https://twitter.com/SalesLoft [image: Linkedin] https://www.linkedin.com/company/salesloft [image: wordpress] http://blog.salesloft.com/blog/ [image: http://salesloft.com/prospector/#video] http://salesloft.com/prospector/#video [image: Software Engineer Logo] http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

[sb8244]

Ah yes thought about the routes. Because there is no new route, rails matched it to show instead of 404. I would rather 404 on the routing level rather than the controller, so the constraint is required.

Question about your approach to params. Why include tests that errors on non white list instead of just letting rails do is thing with ac parameters? That is why there is a lot of code around it. The general idea is to put that in the application controller and then implementations only need to specify their parameters.

On Saturday, April 25, 2015, Steve Bussey steve.bussey@salesloft.com wrote:

Few small things. Thanks for the feedback!

https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/responders_controller.rb#L37 i do this because it makes it very easy to know the concept your working with instead of technical details. This is extremely obvious when scopes are introduced.

The way you are sending new in the tests required that constraint on routes. I may be able to revisit this.

Errors controller is standard. Having an inherited method means that the controllers need to worry about 404 handling. Composition over inheritance in some ways. I highly suggest not inheriting a method like that. In the long run it will produce poorer habits.

Thanks for the rest of the feedback. I agree with it.

Best Steve

On Saturday, April 25, 2015, railschallenge judge #04 < notifications@github.com javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Data model and DB:

• We've seen a couple different approaches for linking responders and emergencies. Some people use an emergency_id column as the foreign key, while others use emergency_code as a foreign key to the emergencies table. It looks like you have both https://github.com/sb8244/railschallenge-city-watch/blob/master/db/schema.rb#L27, so I would suggestion picking one and removing the other column that you won't then need.
• Models themselves are kept clean and straightforward.

Dispatching:

• These lines https://github.com/sb8244/railschallenge-city-watch/blob/master/app/models/emergency_dispatch.rb#L48-L58 seem to add extra methods where there don't need to be any. You could rewrite this method https://github.com/sb8244/railschallenge-city-watch/blob/master/app/models/emergency_dispatch.rb#L6 in the following way to avoid this extra code:

  def call assign_responders!('Fire', emergency.fire_severity) if emergency.fire_severity > 0 assign_responders!('Police', emergency.police_severity) if emergency.police_severity > 0 assign_responders!('Medical', emergency.medical_severity) if emergency.medical_severity > 0

  self end

This implementation avoids the extra methods, keeps your line length within the rubocop restrictions, and seems just as readable.

ResponderCapacity

• Looks like a reasonable solution. Could also do this with scopes on the models, but I think your solution keeps the capacity logic in one place
• I don't really see the purpose of this method https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/responders_controller.rb#L37, since it doesn't clarify what's being done, or save on total code. Why not just use respond_with Responder.all ? They are functionally equivalent, and not adding the extra method makes the controller shorter and less complex.

Parameter handling

-

Seems like you could DRY up your parameter handling. You have permitted parameter logic in all three of the RespondersController https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/responders_controller.rb#L25-L35, 'EmergenciesController https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/emergencies_controller.rb#L29-L35, and ApplicationController https://github.com/sb8244/railschallenge-city-watch/blob/master/app/controllers/application_controller.rb#L9-L43. In all I could 52 or 53 lines of code that are somehow involved in parameter handling, when instead you could either:

inline parameter handling in each action OR

• keep all logic for parameter handling for a given controller contained within just that controller

...either of which would result is less total code and complexity.

404 handling and routes:

• I don't see the need of having a dedicated ErrorsController. This could instead be a method on the ApplicationController that replies with a static response that all descendent controllers could use.
• Can you explain the decision to use the NotNewId class, and its placement into the config/routes.rb https://github.com/sb8244/railschallenge-city-watch/blob/master/config/routes.rb#L8 definitions?

active_model_serializers

• Fine use of this. Many other competitors also went this route rather than having dedicated view templates. Appears to clean up your controller code quite a bit.

— Reply to this email directly or view it on GitHub https://github.com/sb8244/railschallenge-city-watch/issues/1#issuecomment-96264084 .

Steve Bussey •SalesLoft

•Software Engineer

e steve.bussey@salesloft.com javascript:_e(%7B%7D,'cvml','steve.bussey@salesloft.com'); w salesloft.com http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

a 3423 Piedmont Rd NE, Atlanta, GA 30305

[image: Facebook] https://www.facebook.com/SalesLoft [image: Twitter] https://twitter.com/SalesLoft [image: Linkedin] https://www.linkedin.com/company/salesloft [image: wordpress] http://blog.salesloft.com/blog/ [image: http://salesloft.com/prospector/#video] http://salesloft.com/prospector/#video [image: Software Engineer Logo] http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

Steve Bussey •SalesLoft

•Software Engineer

e steve.bussey@salesloft.com w salesloft.com http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

a 3423 Piedmont Rd NE, Atlanta, GA 30305

[image: Facebook] https://www.facebook.com/SalesLoft [image: Twitter] https://twitter.com/SalesLoft [image: Linkedin] https://www.linkedin.com/company/salesloft [image: wordpress] http://blog.salesloft.com/blog/ [image: http://salesloft.com/prospector/#video] http://salesloft.com/prospector/#video [image: Software Engineer Logo] http://salesloft.com/?v=1&utm_expid=76406993-1.E5ncJhwyTAW9uGaR1WdBTA.1?utm_source=Email-Signature-Rescue&utm_medium=Email-Signature&utm_campaign=Email-Signature-Rescue

[ghost]

I do this because it makes it very easy to know the concept your working with instead of technical details. This is extremely obvious when scopes are introduced.

In the sense that a method hides details in general I can see that, but I disagree that hiding the call to .all is a worthwhile savings in this case.

Coming from the side of reading the code for the first time I see the method responders being called and wondering, "Okay, so what is that?" and the answer is, "It's just Responder.all." Both versions are a single line, but one is idiomatic, explicit, and common in Rails code. The other adds a layer of custom indirection that I don't think will pay off.

I'd be interested in more explanation of how this use becomes more obvious with scopes. I'm initially confused about what you mean, especially since you don't seem to have scopes in your implementation. Perhaps I'm just misunderstanding.

I will say my perspective comes from a bias toward being explicit in code, as additional layers of indirection often add debugging time as one has to move up and down the call stack in order to understand a piece of code. I'm perfectly willing to be proven wrong, and there's no saying that the other judges will agree with me.

Thanks for your participation!

[sb8244]

Now that I see it in this app, that is probably fair. In larger apps, I usually pull the resources into a method and the resource into another method. Let's say you had something like this: current_team.widgets.where(available: true). Instead of putting that in several places, you can just put it in widgets and then make a method like:

def widget
  widgets.find(params[:id])
end

But again, it's probably overkill for this very simple application. I find it almost always better in real world applications. (or at least in multi-tenant SaaS where there is 100% of the time a scope in consumer facing controllers)

[sb8244]

Actually, I still think it holds in this application now that I look. I prefer to use a plural resources scope to do something like widgets.create or widgets.find. Maybe just a personal preference, but alas.

[sb8244]

app/models/emergency_dispatch.rb:6:3: C: Metrics/AbcSize: Assignment Branch Condition size for call is too high. [18.25/15]

This happens when refactoring those 3 methods out. That is probably why I did it now that I look at it. So I will keep it how it is, and would like judging to keep in mind that it has a lower AbcSize.

[sb8244]

Thanks in general @railschallenge-judge04 I just pushed up some changes which cleaned up the model, routing, and removed a method in each controller.

I would be interested in seeing after how people handle the invalid parameters in a DRY and safe (not acronym) way. I would say my approach is very DRY as the actual error isn't repeated and the only thing that is "repeated" is the permitted parameters, which would need to be defined anyways. It's a very non-standard constraint so that's interesting!

[ghost]

Since you mention multi-tenant SaaS apps, it's only fair that I say my preference (in projects) is small, focused micro services. So I'm definitely biased in that direction, and will try to keep that in mind.

That's about all the feedback I've got time for today, as we have several more repos to review so that people have a little time to react to the input.

Good luck in the final scoring!

[ghost]

:trophy: Congrats on winning the first railschallenge!

You had the best emergencies and full_response components, according to the judges. We'd like to invite you to write up something about your approach to those components, as well as your overall approach, to be included in the feedback for all participants.

Please reach out to us at info@railschallenge.com

[sb8244]

:tada:

Yes, I will try to get to that asap. Thanks!

[ghost]

Your individual score breakdown:

314 total

• 114 points for passing tests
• 195 points for the judge's scoring
• 5 bonus points for this pull request (it actually put you over the top into 1st place - if you hadn't done this, and all other things were equal, you'd have come in 2nd)