1
\$\begingroup\$

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!

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I think you did a great job on your first answer. Keep it coming! \$\endgroup\$ – AlexV Jun 25 at 13:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.