search

quangbuule / TheMovieDB-iOS

11 stars 4 forks source link

Review required #1

Open quangbuule opened 8 years ago

quangbuule commented 8 years ago

Hi @chug2k,

I pushed the assignment, please review my project.

Thanks

chug2k commented 8 years ago

/cc @coderschoolreview

coderschoolreview commented 8 years ago

Wow. This is probably the best submission I have ever seen. Very good job. :bow: :clap: .

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! :tada:

quangbuule commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.