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

Review required #1

Open
quangbuule opened this issue Nov 16, 2015 · 5 comments
Open

Review required #1

quangbuule opened this issue Nov 16, 2015 · 5 comments

Comments

@quangbuule
Copy link
Owner

Hi @chug2k,

I pushed the assignment, please review my project.

Thanks

@chug2k
Copy link

chug2k commented Nov 16, 2015

/cc @coderschoolreview

@coderschoolreview
Copy link

Wow. This is probably the best submission I have ever seen. Very good job. 🙇 👏 .

There are a few bugs / areas for improvement, though! One thing you'll quickly learn is the more features you add, the more complicated things can get. =)

I see the Man From Uncle twice after the infinite scroll:
image

Strangely this only happens on the first infinite scroll. I wonder if it's because of this:

  func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
    return movieCollection.count + 1
  }

In the "Top Rated" tab, this crashes:
image

Is there a reason you subclassed UITableView instead of just:

    tableView.backgroundColor = colors["primaryBackgroundColor"]
    tableView.separatorColor = colors["borderColor"]

Usability/Design Bug: If I execute a search, it's a little non obvious to get back to "Popular" from "Search". Suggestion is clicking the x should reset back to the original list? Right now I figured out that I have to do a search on an empty box.
image

Small design feedback: Maybe the low rated movie label should be red instead of green? Green usually means good. ;)

You have this ProgressHUD which I don't think you use.

Really nice encapsulation with all the code. Some of the cleanest stuff I've ever seen. Very well done! 🎉

@quangbuule
Copy link
Owner Author

Is there a reason you subclassed UITableView instead of just:

    tableView.backgroundColor = colors["primaryBackgroundColor"]
    tableView.separatorColor = colors["borderColor"]

No, just colors :D. but this shouldn't be done in the controller. Same as MovieCollectionView. In my mind: Rendering view should on a ViewClass, ViewController handle interaction (user events, computer events)

Usability/Design Bug: If I execute a search, it's a little non obvious to get back to "Popular" from "Search". Suggestion is clicking the x should reset back to the original list? Right now I figured out that I have to do a search on an empty box.

Yeah, it's obviously non obvious. But (x) button is mean clear the text box (maybe for another search). There must be another way like another button, another Tab for searching movies (I have thought about this way but would be time wasting)

You have this ProgressHUD which I don't think you use.

At first I wanted to use it in detail view (you will notice that I load some more small data in detail view). But showing HUD while loading very small things is make user feel slowness. So I give it up, I forget about Podfile.

You searched my name? I haven't done any movie yet lol

@quangbuule
Copy link
Owner Author

Two "The Man from U.N.C.L.E.". It's themoviedb false, not mine.
Check it out:

https://api.themoviedb.org/3/discover/movie?sort_by=popularity.desc&api_key=1d3a19c4302b7225834283260b923e20

https://api.themoviedb.org/3/discover/movie?page=2&sort_by=popularity.desc&api_key=1d3a19c4302b7225834283260b923e20

Btw, thanks for reviewing my assignment. I'll try my best on this week assignment.

@quangbuule
Copy link
Owner Author

Top Rated crashed because old movies don't have release date field (in themoviedb) which I assumed that they should have. TopRated movies is random everyday, you know, many films have 10.0 point.

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

No branches or pull requests

3 participants