github page/ DHH Score Challenge in ruby

Question

I wrote the following code which when executed prints the score of https://github.com/dhh.

require 'typhoeus'
require 'json'
class DHHScoreChallenge
 def getData
 req = Typhoeus.get("https://api.github.com/users/dhh/events/public")
 body = req.body
 json_body = JSON.parse(body)
 end
 def calculateScore json_body
 type_and_score = {
 :IssuesEvent => 7,
 :IssueCommentEvent => 6,
 :PushEvent => 5,
 :PullRequestReviewCommentEvent => 4,
 :WatchEvent => 3,
 :CreateEvent => 2
 }
 score = 0
 json_body.each do |commit|
 commit_type = commit['type']
 if type_and_score.has_key? commit_type.to_sym
 score = score + type_and_score[commit_type.to_sym] 
 else
 score = score + 1
 end
 end
 score
 end
 def showScore score
 puts "DHH's github score is #{score}"
 end
end
begin 
 d = DHHScoreChallenge.new
 json_body = d.getData
 score = d.calculateScore(json_body)
 d.showScore(score)
end

I am a beginner in ruby programming and would like to know the best practices of Ruby. I am trying to include portability, encapsulation, reusability in my code. Please let me know if they have been achieved and if there are any improvements that I can make. Thanks in advance!

Answer 1

The major thing I see is, the code is imperative. But things will be lot more easier to read and follow, if you practise declarative way of writing code.

I would not name the class as DHHScoreChallenge. It should be ScoreChallenge, so that it can be used for any user that we pass in :)

I would make the method getData private, in order to externalise the service layer dependency.

Also I would move the type_and_score into a class level map

This logic is too complex to read

if type_and_score.has_key? commit_type.to_sym
 score = score + type_and_score[commit_type.to_sym] 
else
 score = score + 1
end

This could have eliminated by using a combination of dig and fetch. Dig will return nil if you cannot find the key in hash and fetch on a hash can return a value if the value of the key returns nil

Also I see that you are converting the event key to symbol and trying to find the symbol in hash. Instead you might as well have created a hash with strings as symbols, it won't be much of a over head if you freeze the hash.

Something like this

 EVENTS_SCORE_MAP = {
 'IssuesEvent' => 7,
 'IssueCommentEvent' => 6,
 'PushEvent' => 5,
 'PullRequestReviewCommentEvent' => 4,
 'WatchEvent' => 3,
 'CreateEvent' => 2
 }.freeze
 EVENTS_SCORE_MAP.fetch(event.dig('type'), 1)

I rewrote the whole thing in a manner which I would in my production system

require 'typhoeus'
require 'json'
# Github service dealing with all the requests to github
class Github
 def events(user_name)
 url = "https://api.github.com/users/#{user_name}/events/public"
 send_request(
 lambda {
 Typhoeus.get(url)
 }, url
 )
 end
 private
 def send_request(request, path)
 request.call
 rescue Timeout::Error
 raise GatewayTimeoutError.new(gateway: 'github api timed out', path: path)
 end
end
# This class deals calculating the score
class ScoreChallenge
 EVENTS_SCORE_MAP = {
 'IssuesEvent' => 7,
 'IssueCommentEvent' => 6,
 'PushEvent' => 5,
 'PullRequestReviewCommentEvent' => 4,
 'WatchEvent' => 3,
 'CreateEvent' => 2
 }.freeze
 def initialize(user_name, score = 0)
 @user_name = user_name
 @score = score
 end
 def pretty_score
 calculate
 puts "#{@user_name} github score is #{@score}"
 end
 private
 def calculate
 events_of_user = JSON.parse(github_service.events(@user_name).body)
 events_of_user.each do |event|
 @score += EVENTS_SCORE_MAP.fetch(event.dig('type'), 1)
 end
 end
 def github_service
 @github_service = Github.new
 end
end
score_challenge = ScoreChallenge.new('DHH')
score_challenge.pretty_score

As you can see, I have exposed only one method called pretty_score. Which does everything that we need. This is the declarative style of programming I was talking about. I am letting my code figure out what to do, rather than me telling it do something step by step

Also Github is a service class I have created. It should ideally sit under a services folder. I have not implemented GatewayTimeoutError here, but for all practical purposes we should assume that these third party services can timeout/error out we should be able to handle them. You can also look into passing blocks, and custom errors as part of the above code.

Answer 2

First thing I'd like to tackle right away is the name convention for method names and variables.

In ruby, we use underscore_style instead of camelCaseStyle - this makes a huge difference.

Now, let's tackle the code itself. One thing that I see that could be improved here is the fact that you aren't leveraging the power of ruby collections and classes.

So, let's start from the beginning. First, here is what I see that you coding is doing (needs to be done):

1. Get data from http
2. Parsing JSON
3. Calculate score
4. Show score

Those are 4 different responsibilities that can be separated into mini-classes/modules whatever. It's important to make this distinction, so then you can have a more modular system.

What I like to do is to create very small specialised classes and build on top of them creating more and more complex layers of abstractions.

For instance, starting from 1. We can create a class that only handle http requests, and knows nothing about the endpoints it needs to consume.

class Http
 attr_reader :client
 def initialize(client = Typhoeus)
 @client = client
 end
 def get(url)
 client.get(url).body
 end
end

Here I pass the client option, making this class some sort of a adapter. So then, down the road, if I want to I can change my http library without breaking the other classes that depends on this one - Also, Since I own Http class I have more control over its API and I can more easily upgrade Typhoeus to a new version since this is the only place where I interact with this gem.

Next up, consuming the Github JSON Api:

class Github
 BASE_URI = "https://api.github.com"
 attr_reader :http
 def initialize(http: Http.new)
 @http = http
 end
 def events(user)
 parse http.get(URI.join(BASE_URI, "/users/#{user}/events/public"))
 end
 private
 def parse(body)
 json(body).map { |entry| OpenStruct.new(entry) }
 end
 def json(body)
 JSON.parse(body)
 end
end

As you can see, this class tries to be very generic as well. In this case, we expose the events endpoint, but we can as well expose different endpoints of this API. Up to now there's no reference of scores or anything, so this class can be safely used for other purposes when interacting with Github API.

At this point we are left to the meat of the project, where the business logic comes into place.

One thing I'd like to point out is the usage of hash mapping for the scores:

 type_and_score = {
 :IssuesEvent => 7,
 :IssueCommentEvent => 6,
 :PushEvent => 5,
 :PullRequestReviewCommentEvent => 4,
 :WatchEvent => 3,
 :CreateEvent => 2
 }

I'd say this kinda works fine, but I believe there are better ways to handle that. This is usually what we call Primitive Obsession and here, I can clearly see that this hash contains too much knowledge about something (and this something is the core of the business logic - which is the score).

On this scenario, I'd rather create a class to represent the Score itself and make this class a bit smarter, instead of relying solely in the hash provided by ruby. Also, using it together with ruby Enumerable's module we can simplify the score calculation by a bunch, here's how:

First, I simply create a class that represents a score:

class Score
 attr_reader :name, :weight
 def initialize(name, weight)
 @name = name
 @weight = weight
 end
end

This class is very simple, and its sole purpose is to store information in a nice manner.

Then, I have something that I called ScoreSet (for lack of better name) which is a collection of scores.

class ScoreSet
 include Enumerable
 attr_reader :scores
 def initialize(scores)
 @scores = scores
 end
 def each(&block)
 @scores.each(&block)
 end
 def for(name)
 find { |s| s.name == name } || Score.new(name, 1)
 end
end

This class leverages the powerful Enumerable module, that gives us for free methods like map, sum and so son after implementing the each method in it.

Also, See how the factory method for decides which score to select as well as default one.

Finally, to keep things tidy, I calculate the score itself:

class CalculateUserScore
 def initialize(events, score_set)
 @events = events
 @score_set = score_set
 end
 def call
 @events.map { |e| @score_set.for(e.type) }.sum(&:weight)
 end
end

And the calculation is a simple map over the score set calling sum in the end (thanks to Enumerable module)

I'd say this is definetly a bit over-complex version of the problem and I wouldn't do this for such a small script. However, I do believe those patterns can be used in larger applications to have a more modular and robust system.

The full source is available here: https://gist.github.com/jonduarte/017121fa4804189091769eaf9fe7fdc9

• AlexV: Welcome to Code Review! I think you did a great job on your first answer. Keep it coming!