marin-hyatt / flix Goto Github PK

Awesome job! I can see you learning the material as you progress via your commenting. Your logic is sound, with no obvious anti-patterns. Good job also with committing often & clearly stating what each commit does!

You're doing a lot of things right, so I'm mostly going to walk through ways to improve your code quality. Since this feedback is coming the day your last assignment is due (this is my fault), I'm not going to expect this feedback to be part of Twitter — but cleaning up some things before final submission is always a good bet regardless :)

Refactoring

We have a lot of repetitive code, namely around error handling in network requests (and it looks like fetchMovies is exactly the same in the MoviesVC and MoviesGridVC)

One way that you can clean this up is creating a new class with class methods. For example, in the fetchMovies case, I might create something like:

FlixHelpers.h

@interface FlixHelpers : NSObject

+ (void)fetchMovies;
+ (void)networkErrorDialog;

@end

FlixHelpers.m

@implementation FlixHelpers

+ (void)fetchMovies {
    // code here
    // can call [self networkErrorDialog] here too
}

+ (void)networkErrorDialog {
   // code here
}

@end

To use:

#import "FlixHelpers.h"

// ...

- (void)viewDidLoad {
    //...

    [FlixHelpers fetchMovies];
}

This also helps in line with creating the separation of model-view-controller, where the data from the movies is essentially the model.

Nits

nits == tiny things that don't hurt anything, but are nice for legibility

Comments

@ FB, we don't want to leave behind commented out code; comments are strictly used to explain what's going on. So, for example, this would be cleaned up before being pushed to the FB codebase:

I also noticed TODOs for code that you actually implemented lol - you can probably remove the comment completely and/or the TODO so it isn't confusing to the reader.

Indentation

Not sure if this is actually XCode doing the wonky indentation, but if not, you can fix it by highlighting -> CTRL-I

Coding Standards

This is the TINIEST nit because tbh these are probably tiny typos, but going to mention it anyways to be reflective of FB coding standards, but for spacing, it's best to err on the side of how CodePath has been doing them:

- (void)searchBar:(UISearchBar *)searchBar textDidChange:(NSString *)searchText {
    // ...
}

// If no search, don't filter anything

UICollectionViewFlowLayout *layout = (UICollectionViewFlowLayout *)self.collectionView.collectionViewLayout;

Other / Good-to-Know

[self presentViewController:alert animated:YES completion:nil];

Nice work! The purpose of this project was to continue learning Objective-C and the Cocoa Touch (iOS) framework. You should be comfortable creating table views (one of the most common views in iOS) and working with array of dictionaries created from JSON (in this case from the Movies Database API).

A key part of these projects is that you work to polish the visuals and the small UI / UX touches. Developing your design sense is an important part of being a mobile engineer. You'll find that perfecting the UI / UX will often lead to interesting technical challenges as well because the libraries may not behave exactly as you want them to, so you'll have to learn how to achieve the effect that you want. A few other things to note:

• Did you use groups to organize your source files? It's pretty common to organize your source files into Groups, especially for larger projects. You can organize by type of class (i.e. View Controllers, Views, etc) or for larger apps by functionality (i.e. Home Screen, Search Screen, etc).
• Did you minimize the number of public methods and properties in your classes? You should always try to minimize the number of properties and methods that are in the header files. Only expose what you have to, e.g., properties and methods that must be accessed by other classes.
• Did you properly set the Content Mode for your ImageViews? By default the UIContentMode will stretch the image to match the dimensions of the image view, which is probably not what you want. The most common combination is to use Aspect Fill combined with clipping subviews.

However there were some user stories not reflected on your GIF:

• User can pull to refresh the movie list.
• User sees an error message when there's a networking error.