6
\$\begingroup\$

I have just finished with the first Ruby exercise on Level Up Rails, and wanted to get an idea on how I can refactor the code.

You can find the original exercise on github - including the requirements that I have to implement as well as the CSV data files (there are two).

Requirements

Go check out the CSVs and come back. Done? Cool, I've just got a few features I need:

  1. I loaded my favorite dinosaurs into a CSV file you'll need to parse. I don't know a lot about African Dinosaurs though, so I downloaded one from The Pirate Bay. It isn't formatted as well as mine, so you'll need to handle both formats.
  2. I have friends who ask me a lot of questions about dinosaurs (I'm kind of a big deal). Please make sure the dinodex is able to answer these things for me:
    • Grab all the dinosaurs that were bipeds.
    • Grab all the dinosaurs that were carnivores (fish and insects count).
    • Grab dinosaurs for specific periods (no need to differentiate between Early and Late Cretaceous, btw).
    • Grab only big (> 2 tons) or small dinosaurs.
    • Just to be sure, I'd love to be able to combine criteria at will, even better if I can chain filter calls together.
  3. For a given dino, I'd like to be able to print all the known facts about that dinosaur. If there are facts missing, please don't print empty values, just skip that heading. Make sure to print Early / Late etc for the periods.
  4. Also, I'll probably want to print all the dinosaurs in a given collection (after filtering, etc).

My code is as follows:

require 'csv'

# handle filtering on weight
def weight_filter(data, property, value)
  if value.downcase == "large"
    data.delete_if do |row|
      weight = row["weight_in_lbs"]
      weight.nil? ? true : weight <= 2000
    end
  else
    data.delete_if do |row|
      weight = row["weight_in_lbs"]
      weight.nil? ? true : weight > 2000
    end
  end
end

# properly format the arguments
def format_args(property, value)

  # make Insectivores and Piscivores into Carnivores
  if value == "Carnivore"
    value = Array.new
    value << "Carnivore" << "Insectivore" << "Piscivore"
  end

  return property, value
end

def filter_on(filters = {})
  # check arguments
  if filters.empty?
    message = <<-EOS

      Usage: filter_on( {property => value} )
      Where property can be:'WALKING' | 'DIET' | 'PERIOD' | 'SIZE'

      Example Usage: filter_on({ "WALKING" => "Biped", "DIET" => "Carnivore"})

    EOS
    message
  else
    # read data
    data = read_data

    # filter
    filters.each do |property, value|
      property = property.downcase

      if property == "size"
        # special handler for weight
        weight_filter(data, property, value)
      else
        property, value = format_args(property, value)
        data.delete_if { |row| !(value.include?(row[property])) }
      end 
    end

    # skip headers when printing
    no_headers = [] 
    data.each { |row| no_headers << row }

    no_headers
  end
end

def dinoinfo(dinosaur)
  # rationalize argument
  dinosaur.capitalize!

  single_dino_data = '' 

  # load data into memory
  data_set = read_data

  # extract single dinosaur row from data_set
  read_data.each do |row|
    single_dino_data = row if row["name"] == dinosaur
  end

  formatted_output = "\n"

  # did we find a match? 
  if single_dino_data.empty?
    formatted_output << "\tWe did not find a match for \"#{dinosaur}\" in our Dinodex!\n\n"
  else
    # format extraction
    single_dino_data.each do |property, value|
      if !value.nil?
        # add colon
        property = "#{property}:"

        formatted_output << "#{property.upcase.rjust(15)}   #{value}\n\n"
      end
    end
  end

  formatted_output
end

def read_data
  # load dinodex.csv into memory
  dinodex = CSV.read('dinodex.csv', headers: true, converters: :numeric, 
           header_converters: :downcase)

  # append information from african dinos

  CSV.foreach('african_dinosaur_export.csv', headers: true, converters: :numeric, 
              header_converters: :downcase) do |row|

    formatted_input = []

    formatted_input << row["genus"] << row["period"] << nil 

    # handle carnivore
    row["carnivore"] == "Yes" ? formatted_input << "Carnivore" : formatted_input << nil 

    # continue adding to formatted input
    formatted_input << row["weight"] << row["walking"] 

    # add to dinodex
    dinodex << formatted_input
  end

  dinodex
end

I would love to hear some feedback on what can be done better.

\$\endgroup\$
2
\$\begingroup\$

I have little idea of how I would use this. It sorta lives up the requirements piecemeal, but not in a useful way. For instance, formatting output only works for a single, named dinosaur, while filtering returns something that I can't readily format.

Overall, I'd propose a different structure altogether, but I'll just go through your current methods and comment on each. In general, there are a lot of side-effects going on and your naming could use some work. Your methods are also inconsistent in that they can return very different things rather than being predictable. Often it's because they try to do more than one thing, though that's a bad idea.


#weight_filter has a lot of repetition. And it has an argument (property) that is never used! You also use delete_if, which I strongly discourage, since it alters the array you're calling it on; i.e. it has side-effects. It'd be neater to use #select or #reject to produce a new subset array from the initial array.
In terms of naming, data isn't too descriptive - I'd just call it dinosaurs since that's what it is.

A straight-up rewrite (i.e. keeping the API intact, warts and all) of the code itself might be:

def weight_filter(data, property, value)
  target = value.downcase == "large" ? :large : :small
  data.delete_if do |row|
    size = row["weight_in_lbs"].to_i > 2000 ? :large : :small
    size != target
  end
end

#format_args is a very obtuse name. What args does it format exactly? Method arguments? Command line arguments? Well, no, nothing of the sort. All it does is "normalize" the diet attribute/property. And, again, there's a property argument that seems superfluous. The method only makes sense in the context of the "diet" attribute/property, yet the method itself doesn't check for that. The property argument just passes through. It also returns either an array or simply its input value, which means you really don't know what you're going to get back.

In terms of code, if you were to do a straight-up rewrite, I'd do this:

def format_args(property, value)
  if value == "Carnivore"
    property, %w{Carnivore Insectivore Piscivore}
  else
    property, value
  end
end

#filter_on is really strange. It'll either return a CSV::Table or a block of text describing usage. This, frankly, makes no sense. Sure, a command line program will often print its usage if given no arguments, or otherwise it'll print output. But this isn't a command line program; it's just a method within a program. And returning a table is not printing output. Like with #format_args you don't know what you're going to get back. If anything, you could maybe throw an ArgumentError if no filters are given, but it'd make more sense to just return the full data set when there are no filters. And because #filter_on also reads all the data, it'll always start from scratch, not allowing you to chain filters, though that's a soft requirement.
I won't bother with a rewrite, because frankly it wouldn't really help much.

#dinoinfo would make sense if you could give it a row of data, and have it format that and only that. But that's not what it does. Instead it's a filter method that just looks only at the name, and formats what it finds. So you can't combine it with #filter_on, which would seem to be the best use case.
You also have side-effects in the first line, when you capitalize! the argument; this is a no-no. You don't know where that string comes from, yet you're changing it in-place. In other words, you're altering data that does not belong to you.
You also read data twice for some reason. First you read everything into data_set, but you never use that. Then you read it all again, and use #each to find the dinosaur, when you should use #detect.
Again, won't bother with a rewrite.

#read_data assumes that the dinodex.csv file is pristine, correct, and represents the canonical format, and then attempts to change the african_dinosaur_export.csv file into that. That kinda makes sense, but you've missed one thing: In dinodex.csv the Yangchuanosaurus' period is "Oxfordian" - but that's not a geological period. "Oxfordian" is a stage within the Late Jurassic period (c.f. wikipedia). So in fact, the dinodex.csv file isn't perfect either.

So really, both files need some processing in order to match a common format.


So I said I'd choose a different approach altogether. Namely, I'd model a dinosaur.

When I say model, I mean make a class called Dinosaur with all the necessary methods and attributes. Don't just have a big CSV object.

The idea is to make the data set consistent and provide accessors that let you query the data in different ways.

For instance, you might have a class with an interface like this:

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

  # create a new dinosaur from a hash of attributes
  def initialize(attributes); end

  # Is this dinosaur bipedal?
  def biped?; end

  # Is this dinosaur a carnivore (incl. piscivores and insectivores)?
  def carnivore?; end

  # Does this dino weight more than 2000lbs
  def large?; end

  # Is this dinosaur from the given period?
  def from_period?(period); end

  # Formatted string
  def to_s; end

  # A hash that can be serialized as JSON
  def as_json; end
end

Given an array of Dinosaur objects, you can easily filter it using basic Ruby:

# find a specific dinosaur
dino = dinosaurs.detect { |dino| dino.name == "Yangchuanosaurus" }

# find large, carnivorous, bipedal dinosaurs
badasses = dinosaurs.select(&:biped?).select(&:large?).select(&:canivore?)

# print the badasses
badasses.each { |dino| puts dino.to_s }

And you can wrap the array in a Dinodex class to make the above filtering simpler.

\$\endgroup\$
  • \$\begingroup\$ Hi - thank you for taking the time to write such a thorough response. I'd like to go through your feedback one by one. There's a character limit to comments, so unfortunately I will have to break this up into multiple posts. \$\endgroup\$ – Steven L. Mar 15 '15 at 14:47
  • \$\begingroup\$ weight_filter feedback, it doesn't seem like the data object (CSV::Table) responds to a keep_if method. I look at the output of data.methods and couldn't find it. Am I missing something? (Maybe inheritance of methods from a parent / mixin)? When I look at the ruby-doc page for the CSV library, I don't see other methods such as select listed. Why? I really liked the way you handled both keeping only large and keeping only small dinos in that code. In terms of modifying the original data, I did that for memory efficiency's sake (though that's overkill for the dataset I am working with). \$\endgroup\$ – Steven L. Mar 15 '15 at 15:04
  • \$\begingroup\$ format_args feedback Thanks, I will get rid of the property parameter in this method and work only with the value parameter as you indicated. The call to this method will just be a bit different. \$\endgroup\$ – Steven L. Mar 15 '15 at 15:27
  • \$\begingroup\$ filters_on feedback OK, so the general sense that I am getting is that modifying data in place is a bad idea, and that memory is not a big issues so I shouldn't worry about it. I am going to re-write this so that it can be chained, which means I will have to take out the data read and use a wrapper method to call filter_on. (I will post a re-write of that). \$\endgroup\$ – Steven L. Mar 15 '15 at 15:38
  • \$\begingroup\$ dinoinfo I completely get what you mean here. I read the data, and then I didn't use that data. It will now read data_set.each (or a more descriptive name). Also, I'll make it such that this will be called when the filters_on method is complete, for each row, and then it will output the row. This will make it such that it can be chained with the filters_on method as you mentioned. And I will take out the in-place modification of the string. \$\endgroup\$ – Steven L. Mar 15 '15 at 15:40

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.