Interface to a CSV dinosaur database

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:

• 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.

• 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.

• 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.

• 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
end
end

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

# make Insectivores and Piscivores into Carnivores
if value == "Carnivore"
value = Array.new
value value} )
Where

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 biped

StackExchange Code Review Q#84115, answer score: 2