-
-
Save zben/2046359 to your computer and use it in GitHub Desktop.
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 | |
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.
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
.
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.
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.
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.
A few notes:
It goes a long way towards clarifying what's getting called where.
eval
andsave
. More lines is much better than longer, more confusing lines.People
class might be better called something likePeopleManager
.@
variables directly in the code. Instead use accessors. For example,@people_list
would probably be better handled withattr_accessor
or a private method: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.