# github page/ DHH Score Challenge in ruby

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!

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.

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

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"

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

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

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

• Welcome to Code Review! I think you did a great job on your first answer. Keep it coming! Jun 25, 2019 at 13:09