3
\$\begingroup\$

I developed this class to download a JSON from a server, parse the result and call a delegate back on the class that called it. Since I am self started I want to know if this is a good practise.

DataController.h

#import <Foundation/Foundation.h>
#import "Reachability.h"
#import "Movie.h"

//This class is a data controller which means that all requests on webservices should be here. We user enum to identify which request is calling this controller

typedef enum {SearchMode}RequestMode;
RequestMode currentMode;

//================

@protocol DataControllerDelegate <NSObject>

@required
- (void)DataControllerRequestSuccessful:(RequestMode)mode;
- (void)DataControllerRequestFailedWithReason:(NSString*)reason;

@end

//=================


@interface DataController : NSObject <NSURLConnectionDataDelegate>{

    NSURLConnection *conn;
    NSArray *responseArray;
    NSMutableData *responseData;

}

@property (nonatomic, assign) id <DataControllerDelegate> delegate;

@property(nonatomic,copy)NSMutableArray *movieArray;


-(id)initAndSearchForMovieWithTitle:(NSString*)title andDelegate:(id <DataControllerDelegate>)del;


@end

DataController.m

//We use MACRO to keep the URL. In the future when we will have more than 1 request we can change the URL for all requests from here.
#define API_URL @"http://api.nytimes.com"
//We do the same for API Key
#define API_KEY @"XXXXXXXXXXXXXXXXXX"


#import "DataController.h"

@implementation DataController
@synthesize delegate,movieArray;

#pragma mark - Init Function
-(id)initAndSearchForMovieWithTitle:(NSString*)title andDelegate:(id<DataControllerDelegate>)del{

    self = [super init];

    if (self) {

        currentMode=SearchMode;

        self.delegate=del;

        //Check for internet connection
        bool internet = [self internetConnectivity];

        //Sending the requests on the server
        if(internet){

            NSString* url = [NSString stringWithFormat:@"%@/svc/movies/v2/reviews/search.json?query=%@&thousand-best=Y&api-key=%@&order=by-title",API_URL,title,API_KEY];
            NSLog(@"url%@",url);
            NSLog(@"url %@",url);
            NSMutableURLRequest *request = [NSMutableURLRequest requestWithURL:[NSURL URLWithString:url]];

            conn = [[NSURLConnection alloc] initWithRequest:request delegate:self];

            [conn start];

        }

        else
        {

            //No internet connection

            [delegate DataControllerRequestFailedWithReason:@"There is no internet connection available!"];

        }




    }
    return self;

}


#pragma mark - Delegates for NSURLConnection
- (void)connection:(NSURLConnection *)connection didReceiveResponse:(NSURLResponse *)response {
    // A response has been received, this is where we initialize the instance var you created
    // so that we can append data to it in the didReceiveData method
    // Furthermore, this method is called each time there is a redirect so reinitializing it
    // also serves to clear it
    responseData = [[NSMutableData alloc] init];

}

- (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data {
    // Append the new data to the instance variable you declared

    [responseData appendData:data];


    NSString *responseString = [[NSString alloc] initWithData:responseData encoding:NSUTF8StringEncoding];

    //In some cases the API returns this error
    if ([responseString isEqualToString:@"Internal Server Error"]) {

        [connection cancel];

        [delegate DataControllerRequestFailedWithReason:@"Something went wrong. Please try again!"];


    }

}

- (NSCachedURLResponse *)connection:(NSURLConnection *)connection
                  willCacheResponse:(NSCachedURLResponse*)cachedResponse {
    return nil;
}

- (void)connectionDidFinishLoading:(NSURLConnection *)connection {

    [connection cancel];

    //Request has finished successfuly
    [self parseJSONFor:currentMode];

}

- (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error {

    [delegate DataControllerRequestFailedWithReason:@"Something went wrong. Please try again!"];

}

#pragma mark - Parse method
-(void)parseJSONFor:(RequestMode)mode{

    //Declaring NSError to catch any error on Serialization
    NSError *jsonParsingError = nil;


    //Init the dictionaries that will help us to retrieve the data from JSON
    NSDictionary *movieDictionary = [[NSDictionary alloc]init];

    NSDictionary *allDataDictionary = [[NSDictionary alloc]init];

    NSDictionary *linkDataDictionary = [[NSDictionary alloc]init];


    //Init the arrays that will help us to retrieve data
    responseArray = [[NSArray alloc]init];

    movieArray = [[NSMutableArray alloc]init];


    //In this case the Switch is useless but in the future when we have more request it will help us to diferentiate the parsing for each request.
    switch (currentMode) {

        case SearchMode:{

            //Converting the JSON data to a Dictionary
            allDataDictionary = [NSJSONSerialization JSONObjectWithData:responseData options:0 error:&jsonParsingError];

            if (jsonParsingError) {

                //In case the JSON is invalid we call the failed delegate
                [delegate DataControllerRequestFailedWithReason:@"Something went wrong. Please try again!"];

                break;
            }      


            //Converting the "Result" object to an array so we can iterate and get all the values
            responseArray = [allDataDictionary objectForKey:@"results"];


            //Looping through the array and creating the objects Movie and adding them on a new array that will hold the objects
            for(int i=0;i<responseArray.count;i++){

                Movie *mov = [[Movie alloc]init];

                movieDictionary =(NSDictionary*)[responseArray objectAtIndex:i];

                //For each string we check that there is no NSNull value because it will crash the app.

                //For the reviewer we also make sure that the first letter of its name and surname is capitalized(because sometimes the server returns all chars capital which is inconsistend-ungly
                mov.reviewer=[[self validateString:[movieDictionary objectForKey:@"byline"]] capitalizedString];
                mov.review=[self validateString:[movieDictionary objectForKey:@"capsule_review"]];
                mov.releaseDate=[self validateString:[movieDictionary objectForKey:@"dvd_release_date"]];
                mov.title=[self validateString:[movieDictionary objectForKey:@"display_title"]];
                mov.movieID=[self validateString:[movieDictionary objectForKey:@"nyt_movie_id"]];

                linkDataDictionary= (NSDictionary*)[movieDictionary objectForKey:@"link"];

                mov.reviewURL = [linkDataDictionary objectForKey:@"url"];

                [movieArray addObject:mov];

            }

            //Notyfing the ViewController that the request was successfull and the data are ready
            [delegate DataControllerRequestSuccessful:SearchMode];
        }
            break;
        default:
            break;
    }


}
#pragma mark - JSON Helper method
//Checks if the JSON has any NSNull values and convert to N/A string
-(NSString*)validateString:(NSString*)str{
    NSString *validatedString;

    if (str==(NSString *)[NSNull null]) {
        validatedString=@"N/A";

    }
    else{

        validatedString = str;

    }

    return validatedString;
}

#pragma mark - Internet Connectivity method
//Check the internet connectivity
-(BOOL)internetConnectivity{

    Reachability *networkReachability = [Reachability reachabilityForInternetConnection];
    NetworkStatus networkStatus = [networkReachability currentReachabilityStatus];

    if (networkStatus == NotReachable) {

        return FALSE;

    }
    else
    {

        return TRUE;

    }

}
@end
\$\endgroup\$
  • \$\begingroup\$ What's the language of this code? \$\endgroup\$ – IEatBagels Oct 29 '15 at 15:15
  • \$\begingroup\$ It's Objective-C! \$\endgroup\$ – BlackM Oct 29 '15 at 15:16
  • \$\begingroup\$ I added the tag! You'll realise that the language tag is the most important in the questions :p \$\endgroup\$ – IEatBagels Oct 29 '15 at 15:17
  • \$\begingroup\$ Yeap! I forgot about it since iOS is the most popular tag for iOS projects! Thank you \$\endgroup\$ – BlackM Oct 29 '15 at 15:18
3
\$\begingroup\$

I have a few comments.

  • Formatting: you have a lot of extra white space. I would recommend making line breaks mean something rather than putting them automatically between almost all lines of code. This also makes the code harder to read because the outlines/structure are less obvious when there is more white space.
  • Sometimes you declare a variable and use it only once. For example here (and in (BOOL)internetConnectivity there's the same critique):

//Check for internet connection bool internet = [self internetConnectivity];

    //Sending the requests on the server
    if(internet){

Why not condense this to:

    //Sending the requests on the server if there is internet
    if([self internetConnectivity]){

also why not make the name of this function suggestive of what it returns? For example, even better would be:

    //Sending the requests on the server if there is internet
    if([self hasInternetConnectivity]){
  • Consider defining constants to hold strings with error messages. You have this message repeated a few times: "Something went wrong. Please try again!" In the interest of DRY generally and easier customization in the future, I'd recommend putting all your error messages as defined constants in a config file.
  • I probably have something to learn here. Given that delegate is a property, why don't you refer to it as self.delegate? Seems good to make clear what is an instance variable vs instance property to the reader no?
  • Most of your methods are quite generalizable, but then your parseJSONFor seems quite specific to the NYT API. Why not put all this specific stuff into its own function and not into such a generic sounding function? This might even be a function you would want to supply from the delegate (as would the url and api key ultimately).
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.