2
\$\begingroup\$

I've implemented a table view controller for the purposes of subclassing. The table view controller implements a pull-to-refresh which will automatically reload the table view's data. I am looking for general feedback on my class.

DXYBaseTableViewControllerHD.h

#import "DXYBaseViewControllerHD.h"

// Displays a table view for its UI.
//
// Unlike `UITableViewController`, this controller's `view` is not the table
// view itself. This means it is possible to insert content above the table
// view.
@interface DXYBaseTableViewControllerHD : DXYBaseViewControllerHD <UITableViewDataSource, UITableViewDelegate>

// A table view controller managing `tableView` and set up as a child of the
// receiver.
@property (nonatomic, strong, readonly) UITableViewController *childTableViewController;

// The table view controlled by the receiver.
@property (nonatomic, strong, readonly) UITableView *tableView;

- (instancetype)initWithRefreshControl:(BOOL)refreshControl automaticallyLoadMore:(BOOL)loadMore;

//refreshing or loading
@property (nonatomic, assign, readonly, getter=isRefreshing) BOOL refreshing;

//trigger refresh programmatically
- (void)triggerRefresh;

//called by refreshControl or triggered programmatically
//must be overrided by subclass
- (void)doRefresh;

//called when finish refreshing
- (void)endRefreshing;

//called when scroll to bottom
//must be overrided by subclass
//when overriding scrollViewDidScroll: must call [super scroll...] first
- (void)loadMore;

//called when finish loading
- (void)endLoadingMore;

//flag when there is no more data to load
@property (nonatomic, assign, getter=isNoMoreData) BOOL noMoreData;


@end

DXYBaseTableViewControllerHD.m

#import "DXYBaseTableViewControllerHD.h"

@interface DXYBaseTableViewControllerHD ()

@property (nonatomic, assign) BOOL hasRefreshControl;
@property (nonatomic, assign) BOOL needAutoLoadMore;
@property (nonatomic, assign, getter=isLoading) BOOL loading;

@end

@implementation DXYBaseTableViewControllerHD

#pragma mark Properties

- (UITableView *)tableView {
    return self.childTableViewController.tableView;
}

#pragma mark Lifecycle

- (instancetype)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil {
    self = [super init];
    if (self == nil) return nil;

    _childTableViewController = [[UITableViewController alloc] initWithStyle:UITableViewStylePlain];
    [self addChildViewController:_childTableViewController];
    [_childTableViewController didMoveToParentViewController:self];
    return self;
}

- (instancetype)initWithRefreshControl:(BOOL)refreshControl automaticallyLoadMore:(BOOL)loadMore {
    self = [self initWithNibName:nil bundle:nil];
    if (self == nil) return nil;

    self.hasRefreshControl = refreshControl;
    self.needAutoLoadMore = loadMore;
    if (refreshControl) {
        UIRefreshControl *control = [[UIRefreshControl alloc] init];
        [control addTarget:self action:@selector(doRefresh) forControlEvents:UIControlEventValueChanged];
        self.childTableViewController.refreshControl = control;
    }
    if (loadMore) {
        self.noMoreData = NO;
        self.loading = NO;
    }

    return self;
}

- (void)viewDidLoad {
    [super viewDidLoad];

    self.tableView.frame = self.view.bounds;
    self.tableView.delegate = self;
    self.tableView.dataSource = self;
    self.tableView.backgroundColor = [UIColor DXYMainBackgroundColor];
    [self.view insertSubview:self.tableView atIndex:0];

    if (self.hasRefreshControl) {
        [self triggerRefresh];
    }
}

#pragma mark UITableViewDataSource

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
    NSAssert(NO, @"This method must be overridden by subclasses");
    return 0;
}

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    NSAssert(NO, @"This method must be overridden by subclasses");
    return nil;
}

#pragma mark - refresh

- (BOOL)isRefreshing {
    return self.childTableViewController.refreshControl.isRefreshing || self.isLoading;
}

- (void)triggerRefresh {
    [self.childTableViewController.refreshControl beginRefreshing];
    [self doRefresh];
}

- (void)doRefresh {
    NSAssert(NO, @"This method must be overridden by subclasses");
}

- (void)endRefreshing {
    [self.childTableViewController.refreshControl endRefreshing];
}

- (void)loadMore {
    NSAssert(NO, @"This method must be overridden by subclasses");
}

- (void)endLoadingMore {
    self.loading = NO;
}

#pragma mark - scroll view delegate

- (void)scrollViewDidScroll:(UIScrollView *)scrollView {
    CGFloat offsetY = scrollView.contentOffset.y;
    CGFloat contentHeight = scrollView.contentSize.height;
    CGFloat frameHeight = scrollView.frame.size.height;
    if (self.needAutoLoadMore &&
        !self.isNoMoreData &&
        !self.isLoading &&
        (roundf(contentHeight-offsetY-frameHeight) < 44.0)) {

        self.loading = YES;
        [self loadMore];
    }   
}

@end
\$\endgroup\$
  • 3
    \$\begingroup\$ Can you add some description of what you class does? Are the comments actually in your source code, or just added for explanation here? \$\endgroup\$ – nhgrif Apr 7 '15 at 11:44
3
\$\begingroup\$

First and foremost, I have to point you to NS_REQUIRES_SUPER. This macro should be used with your methods which actually implement some logic. The ones, which if overridden, should be sure to call super, such as viewDidLoad.

Beyond that, I'm not entirely sure I'm satisfied with the design of this class. It doesn't really quite fit the design patterns we're used to seeing in iOS development. You're leaving four NSAssert() calls in production code. NSAssert is fine when testing to make sure your code is working correctly, but probably shouldn't be left in production code (leave them for unit tests?).

The NSAssert() calls feel kind of necessary though, because for our default implementation, what should we load into the table? Right? Realistically, what we need isn't a UIViewController subclass, but instead, a UIView subclass (much like UITableView is a UIView subclass). And in place of our NSAssert() calls, our subclass should have a delegate, and it should be calling required methods on our delegate in these spots.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the marco it's new to me. About assert it's actually an way to make sure the specific method has been overridden by all subclasses at compile time. Instead of generating a warning as the marco do this way directly crash the app. And you're right some kind of indicator of loading process is missing \$\endgroup\$ – dopcn Apr 8 '15 at 1:58
  • \$\begingroup\$ I understand what the assert is, but it's not really appropriate to leave it in production code in my opinion. Plus, subclasses won't get a warning or error at build time (only run time). If you implemented this as a view with a delegate (as UITableView is), your delegate would get a build/write time warning for not implementing it (Swift won't even compile it) and you still get the run-time exception, and you're not leaving an assert in production code. \$\endgroup\$ – nhgrif Apr 8 '15 at 2:15
  • \$\begingroup\$ My bad it should be "crash at run time" I mean then you'll find out during development. Whether leave assert in production code is indeed a controversial topic. Right now I just use it this way and add #ifdef DEBUG later \$\endgroup\$ – dopcn Apr 8 '15 at 3:46

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.