HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Downloading and parsing data

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
anddownloadingdataparsing

Problem

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 
#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 

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

@end

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

@interface DataController : NSObject {

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

}

@property (nonatomic, assign) id  delegate;

@property(nonatomic,copy)NSMutableArray *movieArray;

-(id)initAndSearchForMovieWithTitle:(NSString*)title andDelegate:(id )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)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);

Solution

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).

Code Snippets

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

Context

StackExchange Code Review Q#109124, answer score: 3

Revisions (0)

No revisions yet.