pocket-playlab/rails-challenge-simon — pull request #1 by simonbnrd (2014-12-03 14:00:42+00:00)
Title: Fix rails challenge simon

A honest guide to how I solve the rails-challenge project.
=================================================================

Note : the following text is written during the process of solving the rails-challenge project. My main interest here is to show you how I solve such problems. I'm not a Ruby/Rails guru, but I think that I know how to deal effectively with various kind of issues. Also, I would like to apologize for my English. As we say if you don't use your English regularly, it will get rusty. I hope that it will make the technical screening process a bit more human despiste the distance between us !

# Introduction

After a quick fork on Github and a git clone within my terminal, I start to
explore the code. I always start with specs, especially if they're good.
It's the best way, in my opinion, to discover somehow in what consists the project
and from where I should start to dig deeper in the code. That's how important specs
are for newcomers !

A spec for the api_controller.rb tells me that we're dealing with two main
models : User and Level. Now, I'm also sure that the project is an API only project :
the spec checks the results of some HTTP requests and then later inspects their JSON
outputs. Ok, at that point I decide to run the specs and see what happens.

We have two failures :
```shell
rspec ./spec/controllers/api_controller_spec.rb:16 # ApiController POST finish_level sets the high score
rspec ./spec/controllers/api_controller_spec.rb:37 # ApiController GET leaderboard returns a list of top scores per user
```

# First issue

I quickly open a vertical split window with the `config/routes.rb` file in VIM to understand what is going on, and I directly look for the `api_controller.rb` file.

Well, we indeed have only those two methods : `finish_level` and `leaderboard`.

About the first one :

It seems that we fetch the user_id, level_number and the score from params. Then before rendering a JSON output we call a specific method on the user : `user.finish_level(level, score)`

Based on what the spec tells me, I suppose that if we do a call to `finish_level` with a better score than before, this score will be considered the new highscore for the specified level. But since the test is failing, something is probably wrong there.

I quickly notice indeed that something is wrong :

```ruby
def finish_level(level, score)
transaction do
user_level = self.user_levels.find_or_create_by!(level: level)
user_level.attempts.create!(score: score)
user_level.update!(high_score: score)
end
end
```

We always update the high-score with the current score no matter if it's a real high-score. I'll check that later because I actually want to discover all the relations between these models :

```shell
➜ rails-challenge-simon git:(fix-rails_challenge_simon) ✗ ls -al app/models
total 32
drwxr-xr-x 6 hr staff 204 Dec 3 12:15 .
drwxr-xr-x 4 hr staff 136 Dec 3 11:03 ..
-rw-r--r-- 1 hr staff 716 Dec 3 11:02 level.rb
-rw-r--r-- 1 hr staff 439 Dec 3 11:02 user.rb
-rw-r--r-- 1 hr staff 133 Dec 3 11:02 user_level.rb
-rw-r--r-- 1 hr staff 73 Dec 3 11:02 user_level_attempt.rb
```

By reading the code I have a now a basic idea of how everything works, but to be honest, the best way for me to understand a new app is just to use a debugger. That's why I decide to use the gem `jazz_hands` combined with the `pry-byebug' debugger engine :

```ruby
gem 'jazz_hands', github: 'nixme/jazz_hands', branch: 'bring-your-own-debugger'
gem 'pry-byebug'
```

So from now I'll just put a `binding.pry` in `app/models/user.rb` here :

```ruby
def finish_level(level, score)
transaction do
user_level = self.user_levels.find_or_create_by!(level: level)
user_level.attempts.create!(score: score)
binding.pry
user_level.update!(high_score: score)
end
end
```

...and navigate through the process by running the controller spec on the most interesting line :

`bundle exec rspec spec/controllers/api_controller_spec.rb:16`

```shell
From: /Users/hr/dev/rails-challenge-simon/app/models/user.rb @ line 13 User#finish_level:

9: def finish_level(level, score)
10: transaction do
11: user_level = self.user_levels.find_or_create_by!(level: level)
12: user_level.attempts.create!(score: score)
=> 13: binding.pry
14: user_level.update!(high_score: score)
15: end
16: end

[1] rails_challenge(#) » user_level
=> # {
:id => 1,
:high_score => nil,
:user_id => 1,
:level_id => 1,
:created_at => Wed, 03 Dec 2014 11:47:59 UTC +00:00,
:updated_at => Wed, 03 Dec 2014 11:47:59 UTC +00:00
}
[2] rails_challenge(#) » score
=> 2000
[3] rails_challenge(#) » c

Frame number: 0/72

From: /Users/hr/dev/rails-challenge-simon/app/models/user.rb @ line 13 User#finish_level:

9: def finish_level(level, score)
10: transaction do
11: user_level = self.user_levels.find_or_create_by!(level: level)
12: user_level.attempts.create!(score: score)
=> 13: binding.pry
14: user_level.update!(high_score: score)
15: end
16: end

[1] rails_challenge(#) » user_level
=> # {
:id => 1,
:high_score => 2000,
:user_id => 1,
:level_id => 1,
:created_at => Wed, 03 Dec 2014 11:47:59 UTC +00:00,
:updated_at => Wed, 03 Dec 2014 11:48:21 UTC +00:00
}
[2] rails_challenge(#) » score
=> 1000
[3] rails_challenge(#) » c
F

Failures:

1) ApiController POST finish_level sets the high score
Failure/Error: expect(user.user_levels.find_by!(level: levels.first).high_score).to eq(2000)

expected: 2000
got: 1000

(compared using ==)
# ./spec/controllers/api_controller_spec.rb:24:in `block (3 levels) in '
# ./spec/spec_helper.rb:37:in `block (3 levels) in '
# ./spec/spec_helper.rb:37:in `block (2 levels) in '

Finished in 33.64 seconds (files took 1.62 seconds to load)
1 example, 1 failure
```

As if it was not already sure, I now have the confirmation we always overwrite the high-score with current score. At first the high-score is set to `nil`, then is updated with the current score, we just have to add branch condition to only set the high-score if the current score is greater or equal (if we want to keep the record of the last high-score information) to the previous high-score.

One possible fix is :

```ruby
def finish_level(level, score)
transaction do
user_level = self.user_levels.find_or_create_by!(level: level)
user_level.attempts.create!(score: score)
if user_level.high_score.nil? || score > user_level.high_score
user_level.update!(high_score: score)
end
end
end
```

We check first for nil in order to allow the first high_score to be set since in the `schema.db` there is no default value of the `high_score` field, then we only update the `user_level`'s `high_score` if the current `score` is greater than its `high_score`.

I run the spec to check if everything is OK :

```shell
➜ rails-challenge-simon git:(fix-rails_challenge_simon) ✗ bundle exec rspec spec/controllers/api_controller_spec.rb:16
Run options: include {:locations=>{"./spec/controllers/api_controller_spec.rb"=>[16]}}
(db migration output)
.

Finished in 0.10471 seconds (files took 1.65 seconds to load)
1 example, 0 failures
```

Good !

# Second issue

Let's now have a look on the remaining failing test :

`rspec ./spec/controllers/api_controller_spec.rb:37 # ApiController GET leaderboard returns a list of top scores per user`

It seems there is a problem with the `leaderboard` method :

```ruby
it "returns a list of top scores per user" do
opponent = User.create!
level = levels.first
user.finish_level(level, 2000)
user.finish_level(level, 1000)
opponent.finish_level(level, 1500)
get :leaderboard, level_number: levels.first.number
expect(JSON.parse response.body).to include({
'leaderboard' => [
{ 'user' => user.id, 'score' => 2000 },
{ 'user' => opponent.id, 'score' => 1500 },
]
})
```

Actually we don't have the desired output :

```diff
-[{"leaderboard"=>[{"user"=>2, "score"=>2000}, {"user"=>1, "score"=>1500}]}]
+"leaderboard" => [{"user"=>2, "score"=>2000}, {"user"=>1, "score"=>1500}, {"user"=>2, "score"=>1000}],
+"status" => "success",
```

Let's go back to `app/controllers/api_controller.rb` :

```ruby
# Return a list of the 5 top scorers for the given level, along with their
# top scores. If a user has several of the top scores for the level, only
# include the top score for that user.
def leaderboard
level = Level.find_by!(number: params[:level_number])
attempts = level.attempts.includes(:user_level).order(score: :desc).limit(5)
render json: {
status: 'success',
leaderboard: attempts.map { |a| { user: a.user_level.user_id, score: a.score } }
}
end
```

Thanks to the comment we have more information about what the method should do, but it seems that its current behavior is not correct since it does not return only the top score of each user.

I'll drop another `binding.pry`, but this time within the `leaderboard` method.

```ruby
def leaderboard
level = Level.find_by!(number: params[:level_number])
attempts = level.attempts.includes(:user_level).order(score: :desc).limit(5)
binding.pry
render json: {
status: 'success',
leaderboard: attempts.map { |a| { user: a.user_level.user_id, score: a.score } }
}
end
```

I run the specific test :

```shell
➜ rails-challenge-simon git:(fix-rails_challenge_simon) ✗ bundle exec rspec spec/controllers/api_controller_spec.rb:37
Run options: include {:locations=>{"./spec/controllers/api_controller_spec.rb"=>[37]}}
(db migration output)

Frame number: 0/65

From: /Users/hr/dev/rails-challenge-simon/app/controllers/api_controller.rb @ line 22 ApiController#leaderboard:

19: def leaderboard
20: level = Level.find_by!(number: params[:level_number])
21: attempts = level.attempts.includes(:user_level).order(score: :desc).limit(5)
=> 22: binding.pry
23: render json: {
24: status: 'success',
25: leaderboard: attempts.map { |a| { user: a.user_level.user_id, score: a.score } }
26: }
27: end

[1] rails_challenge(#) » attempts
=> #, #, #]>
[2] rails_challenge(#) » attempts.map { |a| { user: a.user_level.user_id, score: a.score } }
=> [
[0] {
:user => 2,
:score => 2000
},
[1] {
:user => 1,
:score => 1500
},
[2] {
:user => 2,
:score => 1000
}
]
```

Ok it's obvious that actually the code does not take only the top score of each user.
But here's, imho, the tricky part of the challenge. Should I keep using `level.attempts` or should I use something else?

After a while I decide to use something like :

```sql
SELECT DISTINCT ON (user_level_attempts.user_level_id) user_level_attempts.* FROM user_level_attempts HAVING MAX(user_level_attempts.score)'
```

or :

```sql
SELECT DISTINCT ON (user_level_attempts.user_level_id) user_level_attempts.* FROM user_level_attempts ORDER user_level_attempts.score LIMIT 5
```

BUT, SQLite does not have `DISTINCT ON`, but only `DISTINCT`. What a fail... :broken_heart:

One solution with SQLite is to use `GROUP BY` + `HAVING(MAX("score"))` :

```ruby
level.attempts.includes(:user_level).group('user_level_id').having('MAX("score")').order(score: :desc).limit(5)
```

Let's see what happens in live :

```shell
[7] rails_challenge(#) » attempts = level.attempts.includes(:user_level).group('user_level_id').having('MAX("score")').order(score: :desc).limit(5)
=> #, #) » attempts.map { |a| { user: a.user_level.user_id, score: a.score } }
=> [
[0] {
:user => 2,
:score => 2000
},
[1] {
:user => 1,
:score => 1500
}
]
```

Ok it works and tests are passing. But there's a problem from what I've recently learned (I'm using NoSQL for two years now) : it would work by default on MySQL but it wouldn't work on PostgreSQL or a MySQL instance configured to follow the SQL standard for GROUP BY because the current GROUP BY leaves ambiguous columns. Thanks to `#sqlite` on `irc.freenode.net`, I received a confirmation of the "dangers" of this request : it can break the Internet.

So in our case, we can stick with the working `.group` version, but we should put a note on it...something like `# TODO: This would need changing if deploying on PostgreSQL`.

*But* let's remember the following comment :

```ruby
# Return a list of the 5 top scorers for the given level, along with their
# top scores. If a user has several of the top scores for the level, only
# include the top score for that user.
```

Maybe we can use `level.user_level` instead of the `level.attempts` :

```shell
[12] rails_challenge(#) » level.attempts
=> #, #, #]>
[13] rails_challenge(#) » foo = level.attempts
=> #, #, #]>
[14] rails_challenge(#) » foo.map { |a| { user: a.user_level.user_id, score: a.score } }
=> [
[0] {
:user => 2,
:score => 2000
},
[1] {
:user => 2,
:score => 1000
},
[2] {
:user => 1,
:score => 1500
}
]
[15] rails_challenge(#) » level.user_levels
=> #, #]>
[16] rails_challenge(#) » foo = level.user_levels
=> #, #]>
[17] rails_challenge(#) » foo.map { |a| { user: a.user_id, score: a.high_score } }
=> [
[0] {
:user => 2,
:score => 2000
},
[1] {
:user => 1,
:score => 1500
}
]
```

The `user_levels` collection seems more appropriate to return a list of the 5 top scorers for a given level, plus, we will only get the top score of each user. Personally, I prefer this solution. Note that I don’t have a complete knowledge of the project and I could be wrong (mistakes are the best way to learn anyway!).

We could implement it like this:

```ruby
def leaderboard
level = Level.find_by!(number: params[:level_number])
entries = level.user_levels.order(high_score: :desc).limit(5)
render json: {
status: 'success',
leaderboard: entries.map { |e| { user: e.user_id, score: e.high_score } }
}
end
```

# Conclusion

Final check :

```shell
➜ rails-challenge-simon git:(fix-rails_challenge_simon) ✗ bundle exec rspec spec/
(db migration output)
.......

Finished in 0.20533 seconds (files took 1.67 seconds to load)
7 examples, 0 failures
```

Yay, everything is green, we’re good to go.