Top wiki pages as an app

Question

I recently did an interview task. I was rejected because of bad code quality. There was two tasks. Here I will present the first, second will be posted in time.

This project is avalible on GitHub

First: Table view

Please create an iPhone application that will present the list of most popular wikis from wikia.com

Requirements

• The application should include one screen using UITableView
• Each row in the table should contain: wiki title, wiki url, wiki thumbnail image
• API calls should not block the UI

Notes

• UI Code should be done in code, no *.xib or *.storyboard files
• The application can have additional screens
• The application should be "release ready" (shall not use any private API, etc.)
• The applicant can use any library that will help him/her in achieving the expected result
• Please implement this in Objective-C only

WikiApi.h (5 lines)

#import <UIKit/UIKit.h>

@interface WikiApi : NSObject

+(void)fetchTop10WithThumbnailDownload:(void (^)(NSString *key,UIImage *thumbnail))onThumbnailDownload complete:(void (^)(NSDictionary* json))complete;
+(void)fetchThumbnail:(NSString*)link complete:(void (^)(UIImage *thumbnail))complete;
@end

WikiApi.m (106 lines)

#import "WikiApi.h"

@implementation WikiApi


+(void)fetchTop10WithThumbnailDownload:(void (^)(NSString *key,UIImage *thumbnail))onThumbnailDownload complete:(void (^)(NSDictionary* json))complete {
    NSURL *url = [[NSURL alloc]initWithString:@"http://www.wikia.com/wikia.php?controller=WikisApi&method=getList&lang=en&limit=10"];
    NSURLRequest*request = [[NSURLRequest alloc]initWithURL:url];
    //Request list of wikis
    [[[NSURLSession sharedSession] dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) {
        if (error != NULL) {
            NSLog(@"%@",[error localizedDescription]);
            return;
        }
         NSDictionary *json = [NSJSONSerialization JSONObjectWithData:data options:0 error:&error];

        //Getting ids for details request
        NSMutableArray *ids = [[NSMutableArray alloc]init];
        for (NSDictionary *wikia in json[@"items"]) {
            NSString *wikiaid = [[NSString alloc]initWithFormat:@"%@",wikia[@"id"]];
            [ids addObject:wikiaid];
        }
        //Fetching Details
        [self fetchDetails:ids complete:^(NSDictionary *links) {

            //Async downloading thumbnails
            for (NSString *key in [links allKeys]) {
                NSString *link = links[key];
                [self fetchThumbnail:link complete:^(UIImage *thumbnail) {
                    //pass tumbnail and key for complete block. Called each time thumbnail is downloaded
                    onThumbnailDownload(key,thumbnail);
                }];
            }
        }];
        //Pass wikis data to complete block
        complete(json);
    }] resume];

}

+(void) fetchDetails:(NSArray*)fetchedWikias complete:(void (^)(NSDictionary *links))complete {

    //Building string for detail request, one request for all ids
    NSString *preUrl = [self buildURLForDetails:fetchedWikias];
    NSURL *url = [[NSURL alloc]initWithString:preUrl];
    NSURLRequest *request = [[NSURLRequest alloc]initWithURL:url];

    [[[NSURLSession sharedSession]dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) {
        if (error != NULL) {
            NSLog(@"%@",[error localizedDescription]);
        }
        NSDictionary *fetchedDetails = [NSJSONSerialization JSONObjectWithData:data options:0 error:&error];

        //Getting links from results and passing them to block
        NSDictionary *links = [self linksFromDetails:fetchedDetails[@"items"]];

        complete(links);

    }] resume];
}

+(void)fetchThumbnail:(NSString*)link complete:(void (^)(UIImage *thumbnail))complete {
    //Fetch thumbnail with cachePolicy returnCacheDataElseLoad
    NSURL *url = [[NSURL alloc]initWithString:link];
    NSURLRequest *request = [[NSURLRequest alloc] initWithURL:url cachePolicy:NSURLRequestReturnCacheDataElseLoad timeoutInterval:60.0];

    [[[NSURLSession sharedSession] dataTaskWithRequest:request completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) {
        if (error != NULL){
            NSLog(@"%@",[error localizedDescription]);
        } else {
            //If thumbnail is ok then pass it to block
            UIImage *img = [[UIImage alloc]initWithData:data];
            if (img.CGImage != NULL) {
                complete(img);
            }
        }
    }] resume];
}


+(NSString*)buildURLForDetails:(NSArray*)fetchedWikias{
    NSMutableString *buildedURL = [[NSMutableString alloc]initWithString:@"http://www.wikia.com/wikia.php?controller=WikisApi&method=getDetails&ids="];
    for (NSInteger i=0 ; i<[fetchedWikias count]; i++) {
        NSString *wId = [fetchedWikias objectAtIndex:i];
        [buildedURL appendString:wId];
        if (i < [fetchedWikias count]-1) {
            [buildedURL appendString:@","];
        } else {
            [buildedURL appendString:@"&height=400&width=400"];
        }
    }
    return buildedURL;
}

+(NSDictionary*)linksFromDetails:(NSDictionary*)fetchedDetails {
    NSMutableDictionary *linksForReturn = [[NSMutableDictionary alloc]init];

    for (NSString *key in [fetchedDetails allKeys]) {
        NSString *link = fetchedDetails[key][@"image"];
        [linksForReturn setValue:link forKey:key];
    }

    return linksForReturn;
}

@end

WikiaTableViewCell.h (10 lines)

#import <UIKit/UIKit.h>

@interface WikiaTableViewCell : UITableViewCell


@property (readwrite,retain) UILabel *title;
@property (readwrite,retain) UILabel *url;
@property (readwrite,retain) UIImageView *thumbnail;
@property (readwrite) long idNumber;
@end

WikiaTableViewCell.m (38 lines)

#import "WikiaTableViewCell.h"

@implementation WikiaTableViewCell
@synthesize title,url,thumbnail,idNumber;

-(id)initWithFrame:(CGRect)frame {
    self = [super initWithFrame:frame];
    if (self) {
        [self setupPositions:frame];
    }
    return self;
}

-(void)setupPositions:(CGRect)frame {

    int x = frame.size.width * 0.375;
    int y = 8;
    title = [[UILabel alloc]initWithFrame:CGRectMake(x, 8, frame.size.width - x, 22)];
    int secondY = y + title.font.pointSize + 8;
    url = [[UILabel alloc]initWithFrame:CGRectMake(x, secondY, frame.size.width - x, 22)];
    int thumbnailSize = frame.size.height;
    thumbnail = [[UIImageView alloc]initWithFrame:CGRectMake(2, y, thumbnailSize, thumbnailSize- (y + 8))];
    thumbnail.contentMode = UIViewContentModeScaleAspectFit;
    [self addSubview:title];
    [self addSubview:url];
    [self addSubview:thumbnail];
}

- (void)awakeFromNib {
    // Initialization code
}

- (void)setSelected:(BOOL)selected animated:(BOOL)animated {
    [super setSelected:selected animated:animated];

}

@end

Top10WikiaTableViewController.h (5 lines)

#import <UIKit/UIKit.h>

@interface Top10WikiaTableViewController : UITableViewController

@end

Top10WikiaTableViewController.h (127 lines)

#import "Top10WikiaTableViewController.h"
#import "WikiaTableViewCell.h"
#import "WikiApi.h"

@interface Top10WikiaTableViewController () {
    NSArray *wikias;
    NSMutableDictionary *thumbnails;
    NSMutableDictionary *cells;
}

@end

@implementation Top10WikiaTableViewController
CGFloat CELL_HEIGHT = 100.0;

- (instancetype)init
{
    self = [super init];
    if (self) {
        self.title = @"Top 10";
    }
    return self;
}

- (void)viewDidLoad {
    [super viewDidLoad];
    wikias = [[NSArray alloc]init];
    thumbnails = [[NSMutableDictionary alloc]init];
    cells = [[NSMutableDictionary alloc]init];
    self.tableView.contentInset = UIEdgeInsetsMake(20, 0, 0, 0);
}

-(void)viewWillAppear:(BOOL)animated{
    [super viewWillAppear:animated];
    [self fetchData];
}

- (void) fetchData {

    [WikiApi fetchTop10WithThumbnailDownload:^(NSString *key, UIImage *thumbnail) {
        dispatch_async(dispatch_get_main_queue(), ^{
            [thumbnails setObject:thumbnail forKey:key];
            [self updateCellThumbnail:key];
        });

    } complete:^(NSDictionary *json) {
        dispatch_async(dispatch_get_main_queue(), ^{
            wikias = json[@"items"];
            [self.tableView reloadData];
        });
    }];
}

- (void)didReceiveMemoryWarning {
    [super didReceiveMemoryWarning];
}

//Updates
-(void)updateCellThumbnail:(NSString*)key {
    WikiaTableViewCell *cell = [self cellWithId:key];
    NSIndexPath *indexPath = [self.tableView indexPathForCell:cell];
    if (indexPath != nil){
        cell.thumbnail.image = [thumbnails objectForKey:key];
    }
}

-(WikiaTableViewCell*)cellWithId:(NSString*)key{
    return [cells objectForKey:key];
}

-(NSDictionary*)linksFromDetails:(NSDictionary*)fetchedDetails {
    NSMutableDictionary *linksForReturn = [[NSMutableDictionary alloc]init];

    for (NSString *key in [fetchedDetails allKeys]) {
        NSString *link = fetchedDetails[key][@"image"];
        [linksForReturn setValue:link forKey:key];
    }

    return linksForReturn;
}


#pragma mark - Table view data source

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    return 1;
}

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
    return [wikias count];
}


- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    CGRect frame = [UIScreen  mainScreen].bounds;
    WikiaTableViewCell *cell = [[WikiaTableViewCell alloc]initWithFrame:CGRectMake(0, 0, frame.size.width, CELL_HEIGHT)];

    NSDictionary *wikia = wikias[indexPath.row];
    cell.title.text = wikia[@"name"];
    cell.url.text = wikia[@"domain"];
    cell.idNumber = [wikia[@"id"] longValue];
    cell.thumbnail.image = [self thumbnailForId:cell.idNumber];

    [self addCellToArrayIfNeeded:cell];

    return cell;
}

//Adding cell if that cell is not alredy in dictionary
-(void)addCellToArrayIfNeeded:(WikiaTableViewCell*)cell {
    NSString *key = [[NSString alloc]initWithFormat:@"%ld",[cell idNumber]];
    if ([cells objectForKey:key] == nil) {
        [cells setObject:cell forKey:key];
    }
}


//If thumbnail exist for id then return it
-(UIImage*)thumbnailForId:(long)wId {
    return [thumbnails valueForKey:[[NSString alloc]initWithFormat:@"%ld",wId]];
}

-(CGFloat)tableView:(UITableView *)tableView heightForRowAtIndexPath:(NSIndexPath *)indexPath{
    CGFloat height = CELL_HEIGHT;
    return height;
}
@end

---

Questions

1. What comment will you give in code review for presented code?
2. In your oppinion did I fulfill requirements?

3. Now I see that there is lack of comments at top of every function. Also there is lack well organise structure in those classes like

   class {
       public fields
       private fields
       inits
       public methods
       private methods
   }
   

   Do you think that this is the case?

Summation

I also did some test but I think that there is no reason to put them here. You can found them in GitHub (link above).

Answer 1

The first thing I would have done in accomplishing this task is probably something very big that really cost you.

Each row in the table should contain: wiki title, wiki url, wiki thumbnail image

One of the absolute first things I would have done would be to create a class to represent the "wiki" object.

@interface WikiPage

@property NSNumber *id;  
@property NSString *title;
@property NSURL *url;
@property UIImage *thumbnail; 

@end

We then create an array of these objects for our table view's data source to deal with.

---

As for the code you actually wrote, I want to just comment on your WikiApi class for now.

It's not a particularly good sign that we've implemented only classs methods. Instead, we should make an instantiable WikiPageFetcher perhaps. And rather than passing blocks, let's set up a delegate. For that, we need a protocol. Something like this:

@protocol WikiPageFetcherDelegate <NSObject>

@required - (void)wikiFetcher:(WikiPageFetcher *) wikiFetcher didFetchWikiPage:(WikiPage *)wikiPage;

@optional - (void)wikiFetcher:(WikiPageFetcher *) wikiFetcher didFailWithError:(NSError *)error;

@end

You'll notice here that we're going to pass a WikiPage object one at a time. Each time we finish parsing and downloading the thumbnail for an individual page, we're going to pass it back. By doing this, we can actually dynamically update our table view and add the results one at a time as they come in instead of waiting for the whole thing to complete.

Additionally, we could add a whole separate protocol method for the completion of the thumbnail download. That way we can quickly inform the delegate that we've parsed the link (shouldn't take long) and come back a few seconds later to pass the image back.

Importantly, internally, we'll get our code passed out to multiple threads. Each of the top 10 (or however many) wiki links we're grabbing will be handled by its own thread, so we should be able to get our results slightly quicker.

It's not a good sign that we've hard coded the number of results we're requesting. Yes, they specified 10... but you'll have specifications like this throughout your career. And then the specification will change. So make the code more versatile and prepare ahead to be asked for a different number.

---

I think this is enough to get you started on a revision before I go into too much more specific detail.

The gist of this is, my end usage should look something like this:

@interface MyTableViewController() <WikiPageFetcherDelegate>

@property NSMutableArray *wikiPages;
@property WikiPageFetcher *wikiFetcher;

@end

@implementation MyTableViewController

- (void)viewDidLoad {
    [super viewDidLoad];
    self.wikiPages = [NSMutableArray array];
    self.wikiFetcher = [WikiPageFetcher wikiPageFetcherWithDelegate:self];
}

- (void)viewDidAppear {
    [super viewDidAppear];
    [self refresh];
}

- (void)refresh {
    [self.wikiFetcher loadTopWikiPages:10];
}

- (void)wikiFetcher:(WikiPageFetcher *)wikiFetcher 
   didFetchWikiPage:(WikiPage *)wikiPage {
    [self.wikiPages addObject:wikiPage];
    NSIndexPath *indexPath = [NSIndexPath indexPathForRow:(self.wikiPages.count - 1) 
                                                inSection:0];
    [self.tableView insertRowsAtIndexPaths:@[indexPath] 
                          withRowAnimation:UITableViewRowAnimationAutomatic];
}

^{I consider it important to separate out the refresh method, because you may want to implement a pull-to-refresh or let the user refresh in other ways, or refresh automatically every 5 minutes, etc., so this is just good practice for all sorts of other applications. This is particularly true when refresh does more than just a single method call.}

Once you can get things to this point, you'll be ready for another round of reviews.

Make sure that the delegate methods are called on the main thread but all the downloading stuff remains in the background.