Skip to content

Tiger level challenge#24

Open
noahpatterson wants to merge 9 commits into
RubyoffRails:masterfrom
noahpatterson:master
Open

Tiger level challenge#24
noahpatterson wants to merge 9 commits into
RubyoffRails:masterfrom
noahpatterson:master

Conversation

@noahpatterson

Copy link
Copy Markdown

I replaced the api_key with mine from a .gitignore file, FYI. Also, 1 question, in movie_json.rb #movie_search_by_title is complicated. Is this best fixed with more, simpler, methods? Thanks!

Comment thread lib/api.rb Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the moving the key into a git-ignored file. But, you can just have that set your variable... in api_key.rb have it set APIKEY directly. This line ends up looking odd (and future-you won't remember what this is)

@jwo

jwo commented Dec 22, 2013

Copy link
Copy Markdown
Member

Great job on the specs! And I really love the thought of security with the gitignore. Let me know if you have any questions on my comments :)

@noahpatterson

Copy link
Copy Markdown
Author

Tried to fix the things you suggested, let me know if this looks better! Thanks

Comment thread lib/api.rb

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great!!

Only comment: rather than "store_movie", recommend "build_movie". Store tends to imply persistence.

@jwo

jwo commented Dec 26, 2013

Copy link
Copy Markdown
Member

Looks greast! Significant improvement in readability. yay!

@jwo

jwo commented Dec 26, 2013

Copy link
Copy Markdown
Member

👍 🎉

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