2
\$\begingroup\$

This is a project that I am looking to clean up. The code is fully functioning and it has already been turned in, so I figured I can let Code Review help me. The git repository can be found here: https://github.com/TylerP33/sports_today

Here is my user input handler for my Command Line interface:

def user_input

    input = nil
    input = gets.strip.downcase

    case input

    when  "football"
        info = SportsToday::Schedule.football
        show_info(info)
        post_search_options


    when "baseball"
        info = SportsToday::Schedule.baseball
        show_info(info)
        post_search_options

    when "college basketball"
        info = SportsToday::Schedule.college_basketball
        show_info(info) 
        post_search_options


    when "nba basketball"
        info = SportsToday::Schedule.nba_basketball
        show_info(info)
        post_search_options

    … 

The code above calls these methods in the SportsToday::Schedule class, which search for specific strings in attributes (derived from scraped HTML):

def self.select_by(sport)
    all.select do |object|
        ATTRIBUTES.any? { |attr| object.public_send(attr).include?(sport) }
    end
end 

def self.football
    select_by("Football")
end

def self.baseball 
    select_by("Baseball")
end

def self.college_basketball
    select_by("College Basketball")
end

def self.nba_basketball
    select_by("NBA")
end

…

And then, the results are displayed using this:

def show_info(info)
    info.collect {|info|
        puts""
        puts""
        puts "\nEvent: #{info.sport}"
        puts "\nTime: #{info.time}"
        puts "\nDescription: #{info.description}"
        puts "\nMatchup: #{info.matchup}"
        puts ""
        puts ""
    }
end

I have some nasty code repetition going on — 10 similar methods in my CLI and Schedule classes — and I have no idea how to get around it. There HAS to be a way for me to make some type of container for keywords, and be able to access them through my select_by method.

\$\endgroup\$
  • \$\begingroup\$ A couple of quick things: Indentation (2 spaces), use do/end for multiline blocks, and puts "\n"*2 is the same as puts ""; puts "". \$\endgroup\$ – thesecretmaster Feb 22 '18 at 20:01
  • \$\begingroup\$ Pretty nifty trick on the puts "\n"*2! Also, I never knew about that convention regarding multiline blocks. I will definitely use that. Thanks for the advice! \$\endgroup\$ – TylerP3358 Feb 22 '18 at 23:15
  • \$\begingroup\$ No problem! I'll try to write a full review if I have time. \$\endgroup\$ – thesecretmaster Feb 22 '18 at 23:53
  • \$\begingroup\$ That would be nice... I am sure some abstraction could definitely be used. The more I continue to learn, the more the code is starting to look more and more elementary. I guess that is how learning is suppose to go :D. But really, I would love to figure out how to improve the select_by functionality of it all. \$\endgroup\$ – TylerP3358 Feb 23 '18 at 3:51
0
\$\begingroup\$

Refactoring #1

The first thing I noticed is the repetition in user_input. You're using the case statement as flow control--it's no different than a series of if/else statements. Instead, use the case statement to set your info variable and move the other statements out:

info = case input
       when "football"           then SportsToday::Schedule.football
       when "baseball"           then SportsToday::Schedule.baseball
       when "college basketball" then SportsToday::Schedule.college_basketball
       when "nba basketball"     then SportsToday::Schedule.nba_basketball
       end

show_info(info)
post_search_options

Refactoring #2

After the first refactoring, it becomes clear that the case statement is simply translating a String to a method name (which gets translated back to a String in the method itself). This is inefficient. It might be tempting to generate the method name:

SportsToday::Schedule.call(input.tr(" ","_"))

but all that does is mask the underlying problem: you have a method for every possible search string.

Instead, consider changing the interface of the class. Instead of one method per search string, use one for all:

#SportsToday::Schedule

def self.sport(sportname)
  select_by(sportname)
end

Not only does this simplify your SportsToday::Schedule class, it simplifies your CLI:

info = SportsToday::Schedule.sport(input)
show_info(info)
post_search_options

Note that I "cheated" a little on this refactoring, as the interface changes slightly. The user needs to enter the case of the sport name differently. However, it's not much different than before, as an exact match was required in your original code too. If the intent is to make it accommodate user input, you can adjust for user input sloppiness in your Schedule class. I wouldn't do it in the CLI, because it requires knowledge of the Schedule's underlying data format and that should be kept encapsulated in your Schedule class.

[I don't have time to add more refactorings right now, I'll try to come back to this and add more.]

\$\endgroup\$
  • \$\begingroup\$ Mark, thanks a ton man! Sorry for the late response! I have been busy and haven't checked back here in sometime. These are brilliant ideas and I will definitely make use of them when I get a chance. Thanks for taking the time and writing all of that! \$\endgroup\$ – TylerP3358 Apr 11 '18 at 20:03

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.