7
\$\begingroup\$

I am trying to complete a Ruby coding exercise. The specifics of the excercise can be found on a the level_up_exercises GitHub repo. This repo also contains the CSV file used in the exercise.

I would like you to comment on how well you think I met the requirements with the exception of

  1. loading the African dinosaur data
  2. exporting the data to JSON (the reason I have not completed these is because I am pretty sure I can do the first one, and I need to do further homework before I can do the second one)

I have put my code below, but I think a run-down of how it works might make it easier for you to comment on the code.

First, I created a Dinosaur class. Most of the attributes are readable (except for the @period instance variable). I've created a custom Dinosaur#to_s method in order to print the dinosaur object. I've also created Dinosaur#carnivorous, Dinosaur#big, and Dinosaur#small. These methods return Booleans, but I didn't name them with an ending "?" for reasons that might be clear in the implementation of the filter method.

Secondly, I created a Dinodex class. This class holds Dinosaur objects in an array. Because the internal data structure is an array, I've defined Dinodex#<<(Dinosaur). Dinodex#to_s merely aggregates the strings returned calls to Dinosaur#to_s, for all the dinosaurs in the dinodex.

The interesting part about the Dinodex (or at least the one I had the most trouble with), is the Dinodex#filter method. The Dinodex#filter method takes an options hash, where (ideally) the keys are attributes of the Dinosaur class, and the values are what the user is selecting for.

NOTE: I am just realizing now that I am writing this out that the else branch in the Dinodex#filter method that I should check if param is valid. I would do this by using the respond_to?(param) method.

I had a tough time figuring out how to make the Dinodex#filters method recursive, until I realized I can just update what the local variable selection points to. Once the right dinosaurs have been selected, a new Dinodex object is returned, so that chaining is possible.

require 'csv'

class Dinosaur
  attr_reader :name, :continent, :diet, :weight, :locomotion, :description

  def initialize(name, period, continent, diet, weight, locomotion, description)
    @name = name
    @period = period
    @continent = continent
    @diet = diet
    @weight = weight
    @locomotion = locomotion
    @description = description
  end

  def carnivorous
    carnivorous = ["Yes", "Carnivore", "Insectivore", "Piscivore"]
    carnivorous.include?(@diet)
  end

  def period(timespan)
    @period.downcase.include?(timespan.downcase)
  end

  def size
    return nil if @weight.nil?

    @weight > 2000 ? "big" : "small"
  end


  def to_s
    justification = 15

    representation = ''

    representation << "-" * 99 + "\n"
    representation << "Name:".ljust(justification) + "#{@name}\n" if @name
    representation << "Period:".ljust(justification) + "#{@period}\n" if @period
    representation << "Continent:".ljust(justification) + "#{@continent}\n" if @continent
    representation << "Diet:".ljust(justification) + "#{@diet}\n" if @diet
    representation << "Weight:".ljust(justification) + "#{@weight}\n" if @weight
    representation << "Locomotion:".ljust(justification) + "#{@locomotion}\n" if @locomotion
    representation << "Description:".ljust(justification) + "#{@description}\n" if @description

    representation
  end
end

class Dinodex
  def initialize(dinosaurs = [])
    @dinosaurs = dinosaurs
  end

  def <<(dino)
    @dinosaurs << dino
  end

  def filter(options = {})
    selection = @dinosaurs

    options.each do |param, value|
      case param
      when "period"
        selection = selection.select { |dino| dino.send(param, value) }
      when "carnivore"
        selection = selection.select { |dino| dino.carnivorous }
      else
        selection = selection.select { |dino| dino.send(param) == value }
      end
    end

    Dinodex.new(selection)
  end

  private

  def to_s
    string_rep = ''

    @dinosaurs.each do |dino|
      string_rep << dino.to_s
    end

    string_rep << "-" * 99

    string_rep
  end
end


# Load Data Into dinodex

dinodex = Dinodex.new

OPTIONS = { :headers => true, :converters => :all }

CSV.foreach("dinodex.csv", OPTIONS) do |row|
  name = row["NAME"]
  period = row["PERIOD"]
  continent = row["CONTINENT"]
  diet = row["DIET"]
  weight =  row["WEIGHT_IN_LBS"]
  locomotion = row["WALKING"]
  description = row["DESCRIPTION"]

  dinodex << Dinosaur.new(name, period, continent, diet, weight, locomotion,
                          description)
end

CSV.foreach("african_dinosaur_export.csv", OPTIONS) do |row|
  name = row["Genus"]
  period = row["Period"]
  continent = row["Continent"]
  diet = "Carnivore" if row["Carnivore"] == "Yes"
  weight =  row["Weight"]
  locomotion = row["Walking"]
  description = nil

  dinodex << Dinosaur.new(name, period, continent, diet, weight, locomotion,
                          description)
end

# puts dinodex
puts dinodex.filter({"size" => "big", "diet" => "Carnivore"})
\$\endgroup\$
7
+100
\$\begingroup\$
carnivorous = ["Yes", "Carnivore", "Insectivore", "Piscivore"]

This should be a constant, declared in the Dinosaur class and not in the carnivorous method. Also it doesn't make a lot of sense. If "Yes" means carnivorous, what does "Carnivore" mean? I would change this to

DIETS = ["Carnivore", "Insectivore", "Piscivore"]

I would rewrite Dinodex#filter to use Enumerable#inject. Examine:

def filter(options = {})
  options.inject(@dinosaurs) do |remaining, option|
    remaining.select { |dino| dino.send(option[0]) == option[1]}
  end
end

This requires you to have methods for all your queryables, which you seem to already have by using attr_reader. If you wanted to get really fancy you could modify your options hash to look something like this:

options = [
  [:size, 500, :<],
  [:diet, "Piscivore"] # assume no third parameter means `==`
]

And then do something like this:

def filter(options = [])
  options.inject(@dinosaurs) do |remaining, option|
    remaining.select { |dino| dino.send(option[0]).send(option[2] || :==, option[1])}
  end
end

See a test of this here. Also I just tested it again with the option [[:name, /saur/, :=~]] and it works beautifully as well (returning Stegosaur and Apatosaur but not T-Rex), so that's cool.

That's a bit unreadable, so maybe make each option a hash like {info: :size, value: 500, op: :<}


You can cut down a bunch of repetition in your Dino#to_s method like this:

[:name, :period, :continent, :diet, :weight, :locomotion, :description].each do |stat|
  value = self.send(stat)
  representation << stat.to_s.capitalize.ljust(justification) + value + "\n" if value
end

In class Dinodex, these two methods:

def initialize(dinosaurs = [])
  @dinosaurs = dinosaurs
end

def <<(dino)
  @dinosaurs << dino
end

should both be making sure that everything being inserted into @dinosaurs actually is a dinosaur, or your later methods that try to call methods like continent on will crash.


Your entire Dinodex#to_s method can be shortened to this:

def to_s
  @dinosaurs.map(&:to_s).join("\n") + "\n" + ("-" * 99)
end
\$\endgroup\$
  • 1
    \$\begingroup\$ I think you mean DIETS as a constant, not @DIETS \$\endgroup\$ – Flambino Jun 5 '15 at 20:34

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.