User Interface for Strategy Game

Question

I've been working a strategy game for iOS for a little while, and each time I add functionality to the game model I'm finding myself building a lot of UI code to sync up with it. I believe that my latest creation could use some code review.

The basic idea is that each worker can have certain job permissions turned on or off. The Scene grabs the worker array from the game and builds a window that will allow the user to enable or disable each type of job by ticking a set of boxes next to their name.

I needed to make it so that the list was always in the same order and so that dead workers would be removed from the list. If I grabbed a new, fresh array every time, the order of the list was changing. So I made an array inside the Scene, and I update it by adding a worker to the array only if it is not currently present. Once that is done, the dead workers are removed from the list. Then finally the UI is built with the edited array of workers.

I also needed to make it so that only a certain number of workers was displayed on each page, and that pages would be added once the number of workers passed that threshold.

Finally, I didn't want this list to be able to change while it was open, because it would be difficult to hit the checkboxes if they were able to move around. So when switching between pages, the contents of the worker array do not get updated.

-(void) buildDwarfJobPermissionBox:(int)pageNumber {
    _dwarfJobPermissionBox = [[SKNode alloc]init];
    [self addChild:_dwarfJobPermissionBox];

    NSMutableArray *tempDwarfArray = [[NSMutableArray alloc]init];

    //if the box is already open, use the existing array rather than build a new one
    if (!_dwarfJobPermissionBoxIsOpen) {
        tempDwarfArray = [_game getDwarvesForRender];
        for (DTDwarf *dwarf in tempDwarfArray) {
            if (![self isDwarfAlreadyInArray:dwarf]) {
                [_dwarfArrayForStatusBox addObject:dwarf];
            }
        }
        [self removeDeadDwarvesFromArray];
        _dwarfJobPermissionBoxIsOpen = YES;
    } else {
        tempDwarfArray = _dwarfArrayForStatusBox;
    }

    int numDwarvesPerPage = 6;
    int numPages = _dwarfArrayForStatusBox.count / numDwarvesPerPage;
    if (numPages < 1) {
        numPages = 1;
    }

    int positionInDwarfArray = 0;
    NSMutableArray *arrayOfPages = [[NSMutableArray alloc]init];
    for (int i = 0; i <= numPages; i++) {
        NSMutableArray *page = [[NSMutableArray alloc]init];
        for (int j = 0; j < numDwarvesPerPage; j++) {
            //stop adding dwarves to the list once at the end of the array
            if (positionInDwarfArray > _dwarfArrayForStatusBox.count - 1) {
                //set it to this value to easily break the outer loop
                j = numDwarvesPerPage;
                break;
            } else {
                [page addObject:[_dwarfArrayForStatusBox objectAtIndex:positionInDwarfArray]];
                positionInDwarfArray++;
            }
        }
        [arrayOfPages addObject:page];
    }
    NSMutableArray *currentPage = [arrayOfPages objectAtIndex:pageNumber];

    SKSpriteNode *dwarfJobPermissionBoxBackground = [[SKSpriteNode alloc]initWithColor:[SKColor blackColor] size:CGSizeMake(_initialScreenSize.width/1.1, _initialScreenSize.height/2)];
    dwarfJobPermissionBoxBackground.position = CGPointMake(_initialScreenSize.width/2, _initialScreenSize.height/2);
    [_dwarfJobPermissionBox addChild:dwarfJobPermissionBoxBackground];

    for (int i = 0; i < currentPage.count; i++) {
        DTDwarf *tempDwarf = [currentPage objectAtIndex:i];
        SKLabelNode *dwarfNameLabel = [[SKLabelNode alloc]initWithFontNamed:@"Arial"];
        [dwarfJobPermissionBoxBackground addChild:dwarfNameLabel];
        dwarfNameLabel.text = tempDwarf.name;
        dwarfNameLabel.fontSize = 15;
        dwarfNameLabel.position = CGPointMake(-dwarfJobPermissionBoxBackground.size.width/3.5, dwarfJobPermissionBoxBackground.size.height/2 - dwarfJobPermissionBoxBackground.size.height/7 - (i * (dwarfJobPermissionBoxBackground.size.height/numDwarvesPerPage)));

        SKLabelNode *dwarfJobLabel = [[SKLabelNode alloc]initWithFontNamed:@"Arial"];
        [dwarfJobPermissionBoxBackground addChild:dwarfJobLabel];
        dwarfJobLabel.text = [self getStatusName:tempDwarf];
        dwarfJobLabel.fontSize = 15;
        dwarfJobLabel.position = CGPointMake(dwarfJobPermissionBoxBackground.size.width/5, dwarfJobPermissionBoxBackground.size.height/2 - dwarfJobPermissionBoxBackground.size.height/7 - (i * (dwarfJobPermissionBoxBackground.size.height/numDwarvesPerPage)));

        SKLabelNode *dwarfAgeLabel = [[SKLabelNode alloc]initWithFontNamed:@"Arial"];
        [dwarfJobPermissionBoxBackground addChild:dwarfAgeLabel];
        dwarfAgeLabel.text = [NSString stringWithFormat:@"%i", tempDwarf.age];
        dwarfAgeLabel.fontSize = 15;
        dwarfAgeLabel.position = CGPointMake(dwarfJobPermissionBoxBackground.size.width/2.5, dwarfJobPermissionBoxBackground.size.height/2 - dwarfJobPermissionBoxBackground.size.height/7 - (i * (dwarfJobPermissionBoxBackground.size.height/numDwarvesPerPage)));
    }

    //dont show the page change buttons if at the start or end of the array of pages
    if (pageNumber > 0) {
        SKSpriteNode *lastPageButton = [[SKSpriteNode alloc]initWithColor:[SKColor blueColor] size:CGSizeMake(dwarfJobPermissionBoxBackground.size.width/5, dwarfJobPermissionBoxBackground.size.height/5)];
        [dwarfJobPermissionBoxBackground addChild:lastPageButton];
        lastPageButton.position = CGPointMake(-dwarfJobPermissionBoxBackground.size.width/3, -dwarfJobPermissionBoxBackground.size.height/1.5);
        lastPageButton.name = @"lastJobPermissionPage";
    }
    if (pageNumber < arrayOfPages.count - 1) {
        SKSpriteNode *nextPageButton = [[SKSpriteNode alloc]initWithColor:[SKColor blueColor] size:CGSizeMake(dwarfJobPermissionBoxBackground.size.width/5, dwarfJobPermissionBoxBackground.size.height/5)];
        [dwarfJobPermissionBoxBackground addChild:nextPageButton];
        nextPageButton.position = CGPointMake(dwarfJobPermissionBoxBackground.size.width/3, -dwarfJobPermissionBoxBackground.size.height/1.5);
        nextPageButton.name = @"nextJobPermissionPage";
    } 
}

The code works, but certainly there are some problems. All of the code involving drawing boxes and text with SpriteKit is long and potentially confusing. I'm not sure what to do about that. I'm also not sure if the flow of the logic is clear to the reader.

Answer 1

First, I'll start by emphatically stating that this method is far too large and tries to accomplish far too much. When your method grows this large, it should definitely be tasked out into smaller methods and functions.

It's okay to write a method or function even if it's only ever called on a single line of code, in a single method, in a single class. If this convenience method/function does just one thing, is well-named, and replaced a handful of lines of code, we've drastically improved the readability of our code.

This method, after refactoring, should just be a series of calls to other methods and functions mostly. This may mean that the code as a whole is more total lines, but each individual method will be not too much more than a handful of lines.

For example, let's take a look at this loop:

for (int i = 0; i < currentPage.count; i++) {
    DTDwarf *tempDwarf = [currentPage objectAtIndex:i];

    SKLabelNode *dwarfNameLabel = [[SKLabelNode alloc]initWithFontNamed:@"Arial"];
    [dwarfJobPermissionBoxBackground addChild:dwarfNameLabel];
    dwarfNameLabel.text = tempDwarf.name;
    dwarfNameLabel.fontSize = 15;
    dwarfNameLabel.position = CGPointMake(-dwarfJobPermissionBoxBackground.size.width/3.5, dwarfJobPermissionBoxBackground.size.height/2 - dwarfJobPermissionBoxBackground.size.height/7 - (i * (dwarfJobPermissionBoxBackground.size.height/numDwarvesPerPage)));

    SKLabelNode *dwarfJobLabel = [[SKLabelNode alloc]initWithFontNamed:@"Arial"];
    [dwarfJobPermissionBoxBackground addChild:dwarfJobLabel];
    dwarfJobLabel.text = [self getStatusName:tempDwarf];
    dwarfJobLabel.fontSize = 15;
    dwarfJobLabel.position = CGPointMake(dwarfJobPermissionBoxBackground.size.width/5, dwarfJobPermissionBoxBackground.size.height/2 - dwarfJobPermissionBoxBackground.size.height/7 - (i * (dwarfJobPermissionBoxBackground.size.height/numDwarvesPerPage)));

    SKLabelNode *dwarfAgeLabel = [[SKLabelNode alloc]initWithFontNamed:@"Arial"];
    [dwarfJobPermissionBoxBackground addChild:dwarfAgeLabel];
    dwarfAgeLabel.text = [NSString stringWithFormat:@"%i", tempDwarf.age];
    dwarfAgeLabel.fontSize = 15;
    dwarfAgeLabel.position = CGPointMake(dwarfJobPermissionBoxBackground.size.width/2.5, dwarfJobPermissionBoxBackground.size.height/2 - dwarfJobPermissionBoxBackground.size.height/7 - (i * (dwarfJobPermissionBoxBackground.size.height/numDwarvesPerPage)));
}

We can save several lines of code within this loop by creating individual methods for creating these labels:

for (int i = 0; i < currentPage.count; ++i) {
    [dwarfJobPermissionBoxBackground addChild:[self dwarfNameLabelForIndex:i]];
    [dwarfJobPermissionBoxBackground addChild:[self dwarfJobLabelForIndex:i]];
    [dwarfJobPermissionBoxBackground addChild:[self dwarfAgeLabelForIndex:i]];
}

Now it's more clear what this loop does. It iterates through dwarves, adding three different labels for each.

Moreover, we've compartmentalized our code. We've moved the logic for creating these three different sorts of labels into a different section. It may make sense for the method to be done in a way that can create all three types of labels.

But what's clear here is this, the loop shouldn't be creating the labels, it should just be adding them. And now, that's all the loop does. Sure, it calls the method to create the labels, but that's fine. Each one of those methods should be 5 lines each.

---

This comment and code section is a little confusing:

//if the box is already open, use the existing array rather than build a new one
if (!_dwarfJobPermissionBoxIsOpen) {
    tempDwarfArray = [_game getDwarvesForRender];
    for (DTDwarf *dwarf in tempDwarfArray) {
        if (![self isDwarfAlreadyInArray:dwarf]) {
            [_dwarfArrayForStatusBox addObject:dwarf];
        }
    }
    [self removeDeadDwarvesFromArray];
    _dwarfJobPermissionBoxIsOpen = YES;
} else {
    tempDwarfArray = _dwarfArrayForStatusBox;
}

Your comment says if x then y, but your code says if !x then z. So at a minimum the comment must be reworded.

However, considering that the condition we're checking is a simple bool, and we're checking it for falseness, I actually think the better option is to leave the comment exactly as is and refactor the code to something more like this:

if (_dwarfJobPermissionBoxIsOpen) {
    tempDwarfArray = _dwarfArrayForStatusBox;
} else {
    // stuff
}

Now the code matches the comment and we're not checking for falseness.

Now, don't get me wrong. There's nothing particularly bad about checking for falseness. And certainly, when no code needs to be execute when the conditional is true, then checking for falseness is definitely the right thing to do.

For example, when you do this:

if (![self isDwarfAlreadyInArray:dwarf]) {
    [_dwarfArrayForStatusBox addObject:dwarf];
}

It's fine. There's no else that corresponds here.

But for me, if we're using an else, let's handle the case when true first.

---

So, I'm going to comment on the names of a few of the methods that this class has. They're not quite technically part of the posted code, but the methods are called... so I can see their names.

- (NSArray *)getDwarvesForRender;

This should be named simply:

- (NSArray *)dwarvesForRender;

There are only a few specific cases where it's actually appropriate to use the word "get" in a method name like this in Objective-C, and this isn't one of those. Other languages use get to denote some sort of getter methods, but this isn't the Objective-C naming convention.

- (BOOL)isDwarfAlreadyInArray:(DTDwarf *)dwarf;

There are a couple ways to go with this method.

First of all, without changing anything but the method name, I'd change it simply to this:

- (BOOL)containsDwarf:(DTDwarf *)dwarf;

We don't need to expose the internal structure of our class (telling everyone we're storing dwarves in an array), moreover, this would be really annoying if we ever decided we'd rather use a dictionary or a set. And finally, this matches perfectly to NSArray's similar method:

- (BOOL)containsObject:(id)object;

The user doesn't need to know its an array, and the user should know that this object has some sort of collection of dwarves in it.

The same logic should be applied to this:

- (void)removeDeadDwarvesFromArray;

This would be fine simply as:

- (void)removeDeadDwarves;

Answer 2

Now that I look at this block mode, I'm finding the logic a bit confusing:

//if the box is already open, use the existing array rather than build a new one
if (!_dwarfJobPermissionBoxIsOpen) {
    tempDwarfArray = [_game getDwarvesForRender];
    for (DTDwarf *dwarf in tempDwarfArray) {
        if (![self isDwarfAlreadyInArray:dwarf]) {
            [_dwarfArrayForStatusBox addObject:dwarf];
        }
    }
    [self removeDeadDwarvesFromArray];
    _dwarfJobPermissionBoxIsOpen = YES;
} else {
    tempDwarfArray = _dwarfArrayForStatusBox;
}

The tempDwarfArray is NEVER used beyond this point in the method. And it has local scope. The else block here is entirely meaningless. The if is a little confusing but it might make sense.

But either way, because the tempDwarfArray is never used beyond this point and the assignment in the else is meaningless, the tempDwarfArray can be 100% eliminated by changing the forin to simple:

for (DTDwarf *dwarf in [_game getDwarvesForRender]) {
    // stuff
}

The method is only called once and the set of objects is constructed before the loop begins. The loop then iterates through the set of objects it got from the first method call.

I suppose that isDwarfAlreadyInArray and removeDeadDwarvesFromArray check against _dwarfArrayForStatusBox perhaps?

for (DTDwarf *dwarf in tempDwarfArray) {
    if (![self isDwarfAlreadyInArray:dwarf]) {
        [_dwarfArrayForStatusBox addObject:dwarf];
    }
}
[self removeDeadDwarvesFromArray];

Is it possible that a dwarf returned by getDwarvesForRender could be dead? (Are dead dwarves rendered?) If so, check not only that the dwarf is not already in the array, but also that the dwarf is not dead. Adding dead dwarves to the array increases the size of the array that removeDeadDwarvesFromArray has to iterate over and remove dwarves from.

---

int numDwarvesPerPage = 6;
int numPages = _dwarfArrayForStatusBox.count / numDwarvesPerPage;
if (numPages < 1) {
    numPages = 1;
}

I think this code snippet probably isn't working as intended for any value greater than 6 that's not a multiple of 6. This will give you 1 page for 11 dwarves. It won't bump up to two pages until 12 dwarves.

Your if probably need to look like this:

if (_dwarfArrayForStatusBox.count % 6 != 0 || numPages == 0) {
    numPages++;
}

As a note, the count method for NSArray returns an NSUInteger. Unsigned. The smallest possible value it can return is zero. Your numDwarvesPerPage is a constant and would never make sense to be negative, so numPages can never be negative.