-
-
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.
evalandsave. More lines is much better than longer, more confusing lines.Peopleclass might be better called something likePeopleManager.@variables directly in the code. Instead use accessors. For example,@people_listwould probably be better handled withattr_accessoror 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.