AI Governor to Play a Strategy Game

I had the idea the other day to implement an artificial intelligence that could play my strategy game. This could be useful in a variety of different ways. For example, the player could "lose contact" with one of the worlds in the game, and the AI could take over in that event. Or perhaps the player could elect a worker as governor to lead one of the worlds. Or maybe there could be opposing players that are artificial intelligence working on their own worlds competing with the player.

I've devised just such an artificial intelligence, and after watching it play my game for a little while, I think it's actually doing pretty well! I know that there are many possible problems with the code, so I hope this one will be a good one for code review.

I have tried to reduce duplicate code as much as possible, and I think I have succeeded. Where I am not happy is with the extensibility and changeability of the code. For example, at the moment the logic is hard coded to match the rules of the game, and so if major aspects of those rules change the AI will need to be changed as well. I tried hard to think of a way to better abstract this away, but it seemed like I had to pass on a large amount of information for something like that to work, and some of the logic contained in this class would have to be repeated in those classes anyway.

The other thing I am not happy about is that I have to pass the Tower itself to the AI in order for it to choose the proper move. I could package up the list of floors and the list of workers, but I also need some job validation that is contained inside the Tower object. Is this a major problem in this case? After all, for an AI to make a good decision it is always going to need a lot of information. Maybe I should have a subclass of the Tower that I instantiate and pass to the AI? But then I would need to have the Game configure this properly before passing it to the AI, and copy the values from the real tower to this mock tower

DTGamePlayingAI

For this post, I'm only looking at the DTGamePlayingAI class. This is a big question, and this is a big answer. I don't want the lack of commentary on other posted classes to be taken as a sign that they're flawless. And some of the comments from this post may be applicable to the other classes as well, but as a full disclaimer, I'm writing this post before having even looked at the other classes.

for (id key in self.tower.towerDict) {
    DTTowerFloor *floor = [self.tower.towerDict objectForKey:key];
    if (floor.isRevealed) {
        [_revealedFloors addObject:floor];
    }
}

When iterating over all the objects in a dictionary and the key itself is irrelevant, we can iterate over the objects as an array:

for (DTTowerFloor *floor in [self.tower.towerDict allObjects]) {
    if (floor.isRevealed) {
        [_revealedFloors addObject:floor];
    }
}

if (youngEnoughDwarves.count  0) {
        return NO;
    }
    return YES;
}
return  NO;

Having BOOL returns where the logic works out like this is quite common. You use it more than once. I've ended up with structures like this myself before as well.

However, I find a different pattern to be slightly more pleasing.

BOOL needMoreDwarves = (youngEnoughDwarves.count  0);

return needMoreDwarves && canBirthMoreDwarves;

-(BOOL) needMoreFood:(int)currentFood;
-(BOOL) needMoreCommonResources:(int)commonResources;

These two methods are curious. They're not exposed publicly, so they're only used privately. The only argument they seem to take is a property of this class... a variable the method can already know about without it being passed. Why are we passing a variable? Just make the methods take no arguments and access the appropriate variable within the method.

You have a lot of methods which build and return a mutable array and in a lot of places, you're calling these methods only to call count on the array to get a number. And probably the worst part here is that you're not even actually concerned with the count. You're only concerned with whether or not an object exists in the array.

Maybe I've missed a spot, but it seems that your methods build these arrays to completeness, then the most that is ever done with the return value is to check whether it's count is greater than zero, and in some cases, do something with the first object in the array.

I'm suggesting we change that. These methods shouldn't build and return arrays. Instead, the should iterate through _revealedFloors and return the first object they would otherwise have added to the array they're building. If we make it through the entirety of the array and haven't found a match, we return nil.

Now, we've saved a bit of time and a bit of space. And we can still do everything we'd want to do.

In the cases where we'd just check if the count is greater than 0, instead we can just check the return to see whether it's nil or not.

In the cases where we'd check if the count is greater than 0 then grab firstObject, we can instead just use the return value (after checking it for nil).

And by the way, you don't need to check an array's count before calling firstObject or lastObject. These two convenience methods are relatively new, and Apple designed these so that they'll return nil if the array is empty, preventing the need for checking the array's size to prevent an index out of bounds exception. Just grab firstObject or lastObject and check if it's nil.

StackExchange Code Review Q#58537, answer score: 7