Skip to content

Conversation

@jomarquez
Copy link

No description provided.

@jomarquez jomarquez closed this Feb 24, 2013
@jwo
Copy link
Member

jwo commented Feb 24, 2013

Hi! Did you mean to close the pull request?

@jomarquez jomarquez reopened this Feb 24, 2013
@jomarquez
Copy link
Author

Hola! Sorry, no I did not mean to close it. I have reopened it...
Thanks,
Joanna

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sooo, this method breaks a couple of my rules for better code:

  1. it is longer than 4 lines long.
  2. it has a begin/rescue that continues on in the method
  3. it has more than 1 line inside of the begin/rescue

Alternatively, I recommend:

def find_movie(movies_hash)
  movies = fetch_movie
  ###... more code
end

def fetch_movie
  puts "Add a movie you really like"
  movie_title = gets.chop
  begin
    Api.search_by_title(movie_title)
  rescue MovieNotFoundError => e
    puts e
    puts "Error " #...
  end
end

You'd then made the Api raise a MovieNotFoundError if a movie wasn't found

@jwo
Copy link
Member

jwo commented Feb 25, 2013

Hi Joanna --

functionally, it looked good! I added some comments about the find_movie method that might help keep code easy to read later on.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants