User Interface for Strategy Game

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 removeDeadD

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 fir

StackExchange Code Review Q#52552, answer score: 4