Judge's feedback

[ghost]

Placeholder issue for judge's feedback.

[ghost]

Data model and DB:

• Not sure what has made several people do this (an ambiguity in the spec?), but this column introduces data duplication that you don't need. You already have Emergency#code, and your foreign key in Responders#emergency_id, so having Responders#emergency_code is a problem because it could potentially get out of sync with Emergency#code. So, you'll want to remove that from your data model.
• Another unnecessary column - if Emergency#resolved_at is nil, then you already know that unresolved is true, and if it's not nil, then unresolved is false . It's better to add a dash of business logic to figure that out than to add unnecessary columns to your data model that need to be kept in sync, adding future maintenance.
• You'll find that I'm a hawk for needless duplication, mostly because I've had duplication in my own code pointed out many times. In that vein I would recommend not using the annotate gem since it duplicates information that's already in your db/schema.rb file. It's just documentation that's likely to get out of sync if you forget to run the rake task to update it, which most developers eventually do. Even if you're perfect at remembering to do it it's one more application dependency to keep up-to-date, which I personally think is unnecessary for the value it provides.
• You're the first person I've seen to assign a slug to the models - interesting. I won't say it's a positive or negative at this point, but let me look at the controller + view code first. Another judge might have stronger or less strong feelings. ;)
• You could replace the Emergency#call_off_responders method's implementation with the following code, which (a) avoids iterating over the associated objects unnecessarily, and (b) avoids putting logic in two models instead of one:

    def call_off_responders
      responders = []
      save!
    end

• With that change Responder#return_to_base should be able to be removed entirely.
• You could replace Emergency::count_resolved with a scope, something like:

    scope :resolved, -> { where.not(:resolved_at, nil) }  # untested code

...and then call it like this:

Emergency.resolved.count

Controllers

• Alright for the most part. Inline rendering avoids the need for separate view files.
• Not sure about the slug thing now. It means you add code to both your model and your EmergenciesController. Seems unnecessary when you could do

@emergency = Emergency.find_by_code(params[:id])

...or alter your route to convert the id parameter to code instead. Another judge might have more to say on this point, maybe not.

[mms-pl]

@railschallenge-judge04 thanks for the input. It seems I need to up my DRY skills :)

First of all, I didn't thought about using emergency_code as an virtual attribute. It's in the spec, but I see the benefit of not saving it into DB directly.

Secondly, I'm not sure about Emergency#resolved_at being a good spot to check for full_response, as it is possible to set it via request without dispatching any Responder. I used Emergency#unresolved as a numeric counter to measure if ResponderDipatcher was abble to bring all Severity to 0 (it's a numeric field with sum of remainig Severity). I will have to think about it.

About the slug. my logic was as follows:

1. Code is assigned via request
2. Code is returned in response, so I shouldn't manipulate it
3. 1&2 give the possiblity to make it not-pretty / dangerous (like "f100" vs "F100")
4. The slug is parametrized to make sure that the above won't be a problem. Can I assume, that the Senior Dev will make sure the code is always correct?

About the rest - great ideas.

[ghost]

I'll see if I can circle back to this sometime tomorrow. I know that doesn't leave you much time for further changes, but we have many more repos to cover this weekend with early feedback, and right now I'm working to make sure everyone gets at least one round with a judge.

Thanks again for your participation!

[ghost]

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

[mms-pl]

After much consideration, I have removed the slugs

[ghost]

...I'm not sure about Emergency#resolved_at being a good spot to check for full_response, as it is possible to set it via request without dispatching any Responder

This should not be the case. This test ensures that you cannot set Emergency#resolved_at during creation, and these other tests ensure that dispatch happens on creation. So it should not be possible, if you have all tests passing, to set Emergency#resolved_at without the proper Responders having already been dispatched.

[mms-pl]

Ok, I get it. I tried to keep the starting values of severities and that is not required by spec. Updated

[ghost]

Sounds good.

[ghost]

Your score breakdown (you placed 19th):

174 points total:

• 114 points for passing tests
• 60 points for the judge's section.
• Components:
  • responders: 2
  • emergencies: 0
  • capcicty: 0
  • dispatch: 1
  • full_response: 1

I wanted to say a special thank you to you for the conversations on Twitter. Was really cool to see the engagement, and I enjoyed interacting with you online (says the faceless, robot-like general @railschallenge account :tongue: ).

Keep your eye out for our post on what judges thought were the best solutions. We're hoping there's a lot of learning to be had. The plan is the following:

Writeup on the following:

• Judges announce themselves and say what they were looking for, and their rationale for scores in general
• Top competitors talk briefly about the components they scored best in, and how they approached the problem

The article summarizing the code, we're hoping, provides a lot of insight into how other programmers think, and our goal is to spread that knowledge to everyone who took the time to participate.

Thanks a million @michalsapka for participating!