Skip to content

Instantly share code, notes, and snippets.

@zben
Created March 15, 2012 19:41
Show Gist options
  • Save zben/2046359 to your computer and use it in GitHub Desktop.
Save zben/2046359 to your computer and use it in GitHub Desktop.
need some critique.
people = People.new.add_dir('./data')
puts 'Output 1:'
people.sort(gender: :asc, last_name: :asc).print
puts
puts 'Output 2:'
people.sort(birthday: :asc).print
puts
puts 'Output 3:'
people.sort(last_name: :desc).print
#This program allows you to read from three types of data files that defines user record differently.
#csv: Abercrombie, Neil, Male, Tan, 2/13/1943
#pipe: Smith | Steve | D | M | Red | 3-3-1985
#space: Kournikova Anna F F 6-3-1975 Red
# the differences are order of attributes(last_name, first_name etc, initial, gender, color, birthday), length of gender string and birthday format.
# the results need to be sortable on any two attributes.
# I wrote the following code. Would like to gather some feedback on if there is a better approach to this problem. Currently the class inheritance is not
# really helping too much. Is there a better way to deal with different file format and keeping track of their peculiarity? I think I am also generating a parser
# instance for every record being inserted. Not efficient either. I tried to convert methods of parser to class methods but then I got a little stuck on how to store the parser perculiarity as class_variable or constant. There must be something better.
# Thank you for your comments.
require 'date'
class Person
attr_accessor :first_name, :last_name, :birthday, :fav_color, :gender
def initialize opts={}
@first_name = opts[:first_name]
@last_name = opts[:last_name]
@gender = (opts[:gender]=~/^F/i).nil? ? "Male" : "Female"
@birthday = opts[:birthday]
@fav_color = opts[:fav_color]
end
def print
"#{@last_name} #{@first_name} #{@gender} #{@birthday.strftime("%-m/%-d/%Y")} #{@fav_color}"
end
end
class People
attr_accessor :people_list
def initialize
@people_list = []
end
def add_file *file_paths
file_paths.each do |file_path|
File.new(file_path).lines.each do |line|
parser ||= get_parser_from_string(line)
@people_list << parser.build_record(line)
end
end
self
end
def add_dir dir_path
add_file(*Dir[File.join(dir_path,"**/*.txt")])
self
end
def get_parser_from_string string
parser = case string
when /\,/
CSVParser.new
when /\|/
PipeParser.new
when /\s/
SpaceParser.new
end
end
def sort opts={first_name: :asc}
opts = opts.to_a
if opts.count >= 2
@people_list.sort! do |a,b|
[eval(opts[0][1] == :asc ? 'a' : 'b').send(opts[0][0].to_s),
eval(opts[1][1] == :asc ? 'a' : 'b').send(opts[1][0].to_s)] <=>
[eval(opts[0][1] == :asc ? 'b' : 'a').send(opts[0][0].to_s),
eval(opts[1][1] == :asc ? 'b' : 'a').send(opts[1][0].to_s)]
end
elsif opts.count == 1
@people_list.sort! do |a,b|
eval(opts[0][1] == :asc ? 'a' : 'b').send(opts[0][0].to_s) <=>
eval(opts[0][1] == :asc ? 'b' : 'a').send(opts[0][0].to_s)
end
end
self
end
def print
@people_list.each{|x| puts x.print}
nil
end
end
class TextParser
def build_record(line)
info = line.split(@delimiter)
info_hash = Hash[*@headers.map(&:to_sym).zip(info.map(&:strip)).flatten]
info_hash[:birthday] = Date.strptime(info_hash[:birthday],@date_format)
Person.new info_hash
end
end
class CSVParser < TextParser
def initialize
@delimiter = ','
@headers = %w{last_name first_name gender fav_color birthday}
@date_format = '%m/%d/%Y'
end
end
class PipeParser < TextParser
def initialize
@delimiter = '|'
@headers = %w{last_name first_name middle_initial gender fav_color birthday}
@date_format = '%m-%d-%Y'
end
end
class SpaceParser < TextParser
def initialize
@delimiter = ' '
@headers = %w{last_name first_name middle_initial gender birthday fav_color}
@date_format = '%m-%d-%Y'
end
end
require 'test/unit'
require './text_parser.rb'
class PersonTest < Test::Unit::TestCase
def test_initializer
date = Date.strptime('31/3/1986','%d/%m/%Y')
person = Person.new(first_name: 'Ben', last_name: 'Zhang', gender: 'Male', birthday: date, fav_color: 'Brown')
assert_equal person.print, "Zhang Ben Male 3/31/1986 Brown"
end
end
class PeopleTest < Test::Unit::TestCase
def test_initializer
people = People.new
assert_equal people.people_list, []
end
def test_add_one_file
people = People.new
people.add_file('data/pipe.txt')
assert_equal people.people_list.count, 3
end
def test_add_multiple_files
people = People.new
people.add_file('data/csv.txt','data/pipe.txt','data/space.txt')
assert_equal people.people_list.count, 9
end
def test_add_directory
people = People.new
people.add_dir('./data')
assert_equal people.people_list.count, 9
end
def test_get_parser_from_string
people = People.new
assert_equal people.get_parser_from_string('asdf,asdf,dsf\n').class, CSVParser
assert_equal people.get_parser_from_string('asdf | asdf | dsf\n').class, PipeParser
assert_equal people.get_parser_from_string('asdf asdf dsf\n').class, SpaceParser
end
def test_sort_by_one_attribute
people = People.new.add_dir('./data')
people.sort(gender: :asc)
assert_equal people.people_list.first.gender, "Female"
people.sort(gender: :desc)
assert_equal people.people_list.first.gender, "Male"
people.sort(first_name: :asc)
assert_equal people.people_list.first.first_name, "Anna"
people.sort(gender: :desc)
assert_equal people.people_list.first.first_name, "Timothy"
end
def test_sorting_by_two_attributes
people = People.new.add_dir('./data')
people.sort(gender: :asc,first_name: :asc)
assert_equal people.people_list.first.gender, "Female"
assert_equal people.people_list.first.first_name, "Anna"
people.sort(gender: :asc,first_name: :desc)
assert_equal people.people_list.first.gender, "Female"
assert_equal people.people_list.first.first_name, "Sue"
people.sort(gender: :desc,first_name: :asc)
assert_equal people.people_list.first.gender, "Male"
assert_equal people.people_list.first.first_name, "Francis"
people.sort(gender: :desc,first_name: :desc)
assert_equal people.people_list.first.gender, "Male"
assert_equal people.people_list.first.first_name, "Timothy"
end
end
class ParserTest < Test::Unit::TestCase
def test_csv_parser
person = CSVParser.new.build_record("Kelly, Sue, Female, Pink, 7/12/1959")
assert_equal person.print, "Kelly Sue Female 7/12/1959 Pink"
end
def test_pipe_parser
person = PipeParser.new.build_record("Smith | Steve | D | M | Red | 3-3-1985")
assert_equal person.print, "Smith Steve Male 3/3/1985 Red"
end
def test_space_parser
person = SpaceParser.new.build_record("Seles Monica H F 12-2-1973 Black")
assert_equal person.print, "Seles Monica Female 12/2/1973 Black"
end
end
@mockdeep
Copy link

A few notes:

  • I generally use parenthesis pretty extensively, so:
def add_file(*file_paths)
assert_equal(person.print, "Smith Steve Male 3/3/1985 Red")

It goes a long way towards clarifying what's getting called where.

  • Try not to get meta. Your sort method looks really confusing and I'd imagine it can be structured a lot more cleanly without eval and save. More lines is much better than longer, more confusing lines.
  • It looks like your classes might be a bit off. I think the People class might be better called something like PeopleManager.
  • I generally shy away from accessing @ variables directly in the code. Instead use accessors. For example, @people_list would probably be better handled with attr_accessor or a private method:
def people_list
  @people_list
end

It doesn't make a big deal right now, but in the long run it makes your code more flexible, which is a quality most decent interviewers will be looking for.

@mockdeep
Copy link

Also, your tests don't really look very well structured to me. I like to see them nested inside of context blocks and focused on testing a specific method. Carbon Five's guide is really good:

http://blog.carbonfive.com/2010/10/21/rspec-best-practices/

It's for rspec, but I doubt it would be difficult to port it to Test::Unit.

@mockdeep
Copy link

The print method on Person doesn't actually print. It returns a string representation of that person. I'd probably rename it to to_s.

@mockdeep
Copy link

The print method on People might be better off being changed to to_s as well, removing the puts inside and just returning a string representation of the group.

@mockdeep
Copy link

TextParser#build_record is a little confusing to me. Line 97 in particular looks like it should be expanded across 3-4 lines. Again, clarity is more valuable than brevity. It's fun to come up with one line solutions to problems, but ultimately expanding it is a lot easier to maintain.

@nstielau
Copy link

Whitespace: you use 2, 3, and 4 spaces to indent methods in different places, and some methods have an empty line before the end. Tighten that up.

I'd move the +get_parser_from_string+ into the TextParser class, so that People doesn't care about delimiters at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment