Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panda + Tiger + Eagle #7

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Panda + Tiger + Eagle #7

wants to merge 10 commits into from

Conversation

Meshed
Copy link

@Meshed Meshed commented Nov 28, 2012

I still need to refactor my test, but i hope to go over that in the office hour if you don't already have a plan.

@@ -3,22 +3,47 @@
require "ostruct"
require_relative "./movie"
class Api
@@search_history = []
Copy link
Member

Choose a reason for hiding this comment

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

I don't like class variables in most cases... This will prevent you from going multi-threaded in the future (as each thread will overwrite this variable)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, that is a left over artifact from before I implemented the Search_History class.

@jwo
Copy link
Member

jwo commented Nov 28, 2012

Good stuff here!

I especially love the way the search_history class ended up looking... Looks great (small methods, readable, clean).

One minor note --- I found that if I had some valid searches, and then mispelled a classic (Billy Maddison), it would use 0 as a score and drop my movie score

Add a movie you really like
Fletch
Found: Fletch. Score: 75
Average movie score: 75.
Average movie year: 1982.
You are getting madder
Search Again (Y/N)
Y
Add a movie you really like
Billy Maddison
Found: NOTHINGFOUNDHERE. Score: 0
Average movie score: 50.
Average movie year: 1321.
Search Again (Y/N)
Y

@Meshed
Copy link
Author

Meshed commented Nov 28, 2012

Thank you for your great suggestions. I will add a test for the NOTHINGFOUNDHERE score bug.

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