patternMinor
Downloading and parsing data
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
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);
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;
@endDataController.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.
Why not condense this to:
also why not make the name of this function suggestive of what it returns? For example, even better would be:
- 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)internetConnectivitythere'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 ofDRYgenerally 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
delegateis a property, why don't you refer to it asself.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
parseJSONForseems 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.