8
\$\begingroup\$

I've been designing UIChessboardView in the likeness of UITableView using protocols and delegates.

UIChessboardView.h

@interface UIChessboardView : UIView

@property (retain) id chessdelegate;

-(void) selectChesstileWithID:(NSInteger) tileID;
-(void) deselectChesstileWithID:(NSInteger) tileID;

@end

@protocol ChessboardViewDelegate <NSObject>

-(UIImage*) chessboardView:(UIChessboardView*)chessboardView imageForChesstile:(NSInteger) tileID;

-(void)chessboardView:(id)chessboardView mouseDown_chessTileWithID:(NSInteger)tileID;
-(void)chessboardView:(id)chessboardView mouseUp_chessTileWithID:(NSInteger)tileID;


@end

UIChessboardView.m

@implementation UIChessboardView
{
    NSDictionary* chessTileViews;
    NSArray *tileKeys;
}

-(id)initWithFrame:(CGRect)frame
{
    if(self = [super initWithFrame:frame])
    {
        tileKeys = @[@"A1",@"A2",@"A3",@"A4",@"A5",@"A6",@"A7",@"A8",
                     @"B1",@"B2",@"B3",@"B4",@"B5",@"B6",@"B7",@"B8",
                     @"C1",@"C2",@"C3",@"C4",@"C5",@"C6",@"C7",@"C8",
                     @"D1",@"D2",@"D3",@"D4",@"D5",@"D6",@"D7",@"D8",
                     @"E1",@"E2",@"E3",@"E4",@"E5",@"E6",@"E7",@"E8",
                     @"F1",@"F2",@"F3",@"F4",@"F5",@"F6",@"F7",@"F8",
                     @"G1",@"G2",@"G3",@"G4",@"G5",@"G6",@"G7",@"G8",
                     @"H1",@"H2",@"H3",@"H4",@"H5",@"H6",@"H7",@"H8"];

        [self buildChessTileViews];
        [self createBackgroundWithImageNamed:@"darkwoodenback.png"];
    }
    return self;
}

-(void) buildChessTileViews
{
    NSMutableArray *tiles = [[NSMutableArray alloc] init];
    CGFloat tileHeight = self.frame.size.height/8;
    CGFloat tileWidth = self.frame.size.width/8;
    int index;
    BOOL darkBackround = false;
    for(int row = 0; row < 8; row++)
    {
        for(int column = 0; column < 8; column++)
        {
            index = (row*8)+column;
            UIView* tileView = [[UIView alloc] initWithFrame:CGRectMake((column*tileHeight), (row*tileWidth), tileWidth, tileHeight)];

            darkBackround = (column == 0) ? darkBackround : !darkBackround;
            [tileView setBackgroundColor:(darkBackround == true) ? [UIColor blackColor] : [UIColor lightGrayColor]];
            [tileView setTag:index];
            [tileView setBackgroundColor:[UIColor clearColor]];
            [self addSubview:tileView];
            [tiles addObject:tileView];
        }
    }

    chessTileViews = [[NSDictionary alloc] initWithObjects:tiles forKeys:tileKeys];
}

-(void) createBackgroundWithImageNamed:(NSString*) imageName
{
    UIImageView* image = [[UIImageView alloc] initWithImage:[UIImage imageNamed:imageName]];
    image.frame = CGRectMake(0, 0, self.frame.size.width, self.frame.size.height);

    [self addSubview:image];
    [self sendSubviewToBack:image];
}

-(void) selectChesstileWithID:(NSInteger)tileID
{
    [[chessTileViews objectForKey:tileKeys[tileID]] setBackgroundColor:[UIColor redColor]];
}

-(void) deselectChesstileWithID:(NSInteger)tileID
{
    [[chessTileViews objectForKey:tileKeys[tileID]] setBackgroundColor:[UIColor clearColor]];
}

-(void)touchesBegan:(NSSet *)touches withEvent:(UIEvent *)event
{
    UITouch* touch = [[touches allObjects] firstObject];
    UIView* view = [touch view];

    [self.chessdelegate chessboardView:self mouseDown_chessTileWithID:[view tag]];
}


@end

I think this would be a good time to use Key-Value Observing to have the model directly update the UI? This is my first real project that I want to carry to completion and I'm not sure if this is a good approach to its design. I figure by trying to keep the API as primitive as possible it would make interfacing with other objects easier later on.

Also what should I be focusing on to keep the UI responsive? I've been reading about constraints and layouts from apple's documentation.

\$\endgroup\$
7
  • \$\begingroup\$ While its not much code I vote for reopen the question. The code shall compile and there can be a little review done. \$\endgroup\$
    – chillworld
    Commented May 25, 2014 at 8:19
  • \$\begingroup\$ I don't know how this got re-opened. Did we look at the code? There's nothing in here. Will it compile, technically yes. But it's two void with nothing but comments in them, and a non-void method with return NULL only added so it would technically compile. \$\endgroup\$
    – nhgrif
    Commented May 25, 2014 at 12:49
  • 1
    \$\begingroup\$ But just as an additional note, questions posted to code review are going to be review as posted code only. I know you really want us to go look at your github page, but it's not particularly constructive or beneficial to the site for this question to be a simply link to a github page and answers reviewing off-site code. Post any and ALL code you want reviewed. You can include the link to the github account if you want, but your code will be reviewed as if the code at your github page simple doesn't exist at all. \$\endgroup\$
    – nhgrif
    Commented May 25, 2014 at 22:20
  • 1
    \$\begingroup\$ One last thought, @Literphor You might consider breaking this into several questions. Each question should focus on one piece of the entire project. In that question, include all of the code for the primary class you're focusing on as well as all of the code for this class's superclasses. If these things are truly designed with an OOP mindset, I should be able to 100% completely review any individual class purely on its own merits. And please don't forget to include header files. \$\endgroup\$
    – nhgrif
    Commented May 25, 2014 at 22:53
  • 2
    \$\begingroup\$ The project you have here is relatively large. It would be beneficial for everyone if you could split this in to a number of smaller questions. For us, it would make it easier to review smaller parts without missing things. For you it would be good to find (or create) the logical code-chunks in your project (if you don't have logical chunks then it probably means your design is problematic, and creating responsibility silos will help). Also, everyone gets more rep with more questions. \$\endgroup\$
    – rolfl
    Commented May 25, 2014 at 22:54

1 Answer 1

8
\$\begingroup\$
@property (retain) id chessdelegate;

There are several things wrong with this line.

  • retain - While this technically still is a property attribute, it has been replaced by strong. It does the exact same thing as strong, but strong is the vastly preferred term here.
  • retain or strong are both wrong here anyway. Any delegate property should ALWAYS be weak or assign (and weak is better). In almost every instance, the object server as a delegate already has a strong reference to the object with the delegate property. If we create a strong reference back, we're creating a retain cycle and the memory will never be able to be deallocated.
  • id works as the type, technically, but we don't want to use that. What we actually want is something that conforms to the protocol we've outlined. We need to either forward declare the protocol, or probably better, just move the entire protocol definition before the class interface, and then change that type to id<ChessboardViewDelegate>.
  • chessdelegate - First, just delegate is more than fine. Since we've changed the type to id<ChessboardViewDelegate>, we know what kind of delegate it is... the only reason to give the property a name more descriptive than just delegate is if the class has and needs more than one type of delegate. AND, if you find yourself in this situation, please don't forget to camelcase the property. But in the end, it should just be delegate.

So, here's what the line should actually look line:

@property (weak) id<ChessboardViewDelegate> delegate;

mouseDown, mouseUp, and click. We're developing an iOS project, not OSX, right? All iOS devices are touch devices. Moreover, look at the UIView method you override: touchesBegan:withEvent:. There is mouse in iOS and our method names should reflect that.


Now then, as for the actual meat of this review and your code, what are we actually looking at?

This is a UI element meant to represent a chessboard. Apple is quite consistent with the Foundation classes in Cocoa Touch. Classes, even unrelated classes, will have similarly named methods that provide similar functionality when it makes sense to do so.

Let's consider UITableView and UICollectionView for a moment (and I'm choosing these for a reason). Look at the similarities between these objects--specifically, how their delegate/datasource methods work.

What does the actual view itself actually do? It only does two things. First, it asks its datasource for what it should draw at a particular index path (and then it takes care of displaying it). Second, it receives the touch events, then calls a method on its delegate to give it an opportunity to respond to the touch event.

Remember when I said that we should rename the delegate to simply delegate and that the only reason it should be different is if you have multiple? Well, it seems that in keeping with Apple's design for this sort of stuff, it makes sense that we would have both a delegate protocol and a datasource protocol, and as such, we should have a property for each:

@property (weak) id<ChessboardViewDelegate> delegate;
@property (weak) id<ChessboardviewDatasource> datasource;

Now, we the chessboard view detects a touch event, it lets the delegate know and give the delegate a chance to respond to this event. Meanwhile, the chessboard should have a public method called reloadData (exactly as UITableView and UICollectionView have). When this method is called, the chessboard view goes through a series of calls to its datasource to determine what to draw at each square.

The chessboard view itself can maintain some information like background image, background colors, how to draw the squares, borders, etc., and it can even have information for what a knight looks like or what a pawn or king, etc looks like, but it should call to a datasource to say "What occupies this square?" 64 times. And with this in mind, it shouldn't even care about any of the chess rules. If I want to put 64 black kings on the board, the chessboard view should be fine with it.

So here's how the program flow should work with this setup:

  1. First, we load the chessboard. Next, we "reload" data, this is the first time we call to the datasource. We ask the datasource what the board position should look like. This first call might be a new game of chess, or perhaps loading a chess puzzle, or loading a previously saved game.
  2. Next, we wait for user input. Once we have some user input, we call out to the delegate. The delegate performs some logic. Is this a valid move or not? Do we need to calculate the AI's response to this move? Anyway, the delegate is informed of the move that's attempting to be made.
  3. Once we've determined the result of the attempted move, we update the data model, and we ask the chessboard view to reload. We the view reloads via calling out to the datasource, which has updated data, so the board will be drawn differently, and we're back to step two of waiting for user input. Step 2 and 3 repeat in a cycle until the logic in step 3 dictates that one player or the other has won.

What would make sense to accompany this class is an enum that accounts for all the possible square states. Each square is limited to only a dozen-ish possible states:

typedef NS_ENUM(NSInteger, ChessboardViewSquareState) {
    ChessboardViewSquareStateUnoccupied,
    ChessboardViewSquareStateWhitePawn,
    ChessboardViewSquareStateWhiteRook,
    ChessboardViewSquareStateWhiteKnight,
    ChessboardViewSquareStateWhiteBishop,
    ChessboardViewSquareStateWhiteQueen,
    ChessboardViewSquareStateWhiteKing,
    ChessboardViewSquareStateBlackPawn,
    ChessboardViewSquareStateBlackRook, 
    ChessboardViewSquareStateBlackKnight,
    ChessboardViewSquareStateBlackBishop,
    ChessboardViewSquareStateBlackQueen,
    ChessboardViewSquareStateBlackKing
}

Now, your datasource has a method that asks for one of these values:

@required - (ChessboardViewSquareState)chessboardView:(ChessboardView *)chessboardView stateForSquare:(ChessboardSquare)square;

The chessboard view calls this method on the datasource. It tells the datasource which chessboard is asking for the square state and what square it's asking about. The datasource simply returns a value from the above enum.

Notice the similarity between this method and UITableView's datasource method:

@required - (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath)indexPath;

Oh, and the ChessboardSquare data type I mentioned? This could be an enum with 64 different values, but what I think is actually better is a struct of two enums.

typedef NS_ENUM(NSInteger ChessboardRow) {
    ChessboardRow1,
    ChessboardRow2,
    // etc... to 8

typedef NS_ENUM(NSInteger ChessboardColumn) {
    ChessboardColumnA,
    ChessboardColumnB,
    // etc...


typedef struct {
    ChessboardRow row;
    ChessboardColumn column;
} ChessboardSquare;

I think it would be very beneficial to spend a lot of time studying the delegate/protocol pattern and write some unrelated practice projects dealing with UITableView and UICollectionView and really completely understanding how these ui elements interact with the data model and controllers behind them.

If I were writing this chessboard view, it'd be a UIView subclass, yes, but the view would have an 8x8 UICollectionView on it to serve as the chessboard. It'd be the delegate and datasource to the collection view. Now, when the collection view calls a delegate/datasource method on the ChessboardView, some of them the chessboard view can handle entirely on its own. The rest it can handle mostly on its own, but it's going to call out to the Chessboard delegate/datasource to get the rest of the information concerning how it needs to display the board based on the current game state.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much nhgrif that's exactly the input I needed. I feel stupid for doing MouseDown, MouseUP, I guess I forgot what platform I'm developing on now, but your response helped reinforce the mindset I need for iOS development.I also see the benefit of using a datasource instead of KVO. At least then I can easily change the datasource and this should save me issues later on. \$\endgroup\$
    – Literphor
    Commented May 27, 2014 at 21:01
  • \$\begingroup\$ Yea, like I said, spend a bit of time really studying UITableView or UICollectionView and the relationship these have with their delegates and datasources. This should be an extremely good example to model your ChessboardView after. \$\endgroup\$
    – nhgrif
    Commented May 27, 2014 at 22:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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