Skill Upgrades for Strategy Game

I built a simple class to handle skill increases for the workers in a strategy game for iOS. The basic idea is that whenever a worker finishes a job, their skill (which starts at 0) will increase by 1. Whenever the skill reaches a multiple of 10, this increases the skill multiplier by 0.10. This multiplier is multiplied with the starting amount of time required to finish a job to calculate the new amount of time required to finish a job. The minimum amount of time to finish is 1. The result is an integer because it is currently counting down ticks of the game rather than actual seconds.

Here is the header for the skill class (every worker has one of these):

#import 

@interface DTDwarfSkills : NSObject

-(void) skillUpMining;
-(void) skillUpBuilding;
-(void) skillUpWorkRoom;
-(void) skillUpFighting;
-(void) skillUpCleaning;
-(void) skillUpHauling;

-(NSMutableDictionary *) skillMultipliers;

-(void) logCurrentSkills;

@end

Here is the implementation:

```
#import "DTDwarfSkills.h"

@implementation DTDwarfSkills {
int _miningSkill;
int _buildingSkill;
int _workRoomSkill;
int _fightingSkill;
int _cleaningSkill;
int _haulingSkill;

NSMutableDictionary *_skillMultipliers;
}

static const NSString* miningSkillMultiplier = @"miningSkillMultiplier";
static const NSString* buildingSkillMultiplier = @"buildingSkillMultiplier";
static const NSString* workRoomSkillMultiplier = @"workRoomSkillMultiplier";
static const NSString* fightingSkillMultiplier = @"fightingSkillMultiplier";
static const NSString* cleaningSkillMultiplier = @"cleaningSkillMultiplier";
static const NSString* haulingSkillMultiplier = @"haulingSkillMultiplier";

#pragma mark - Initialization
-(id) init {
self = [super init];
if (self) {
_miningSkill = 0;
_buildingSkill = 0;
_workRoomSkill = 0;
_fightingSkill = 0;
_cleaningSkill = 0;
_haulingSkill = 0;

[self buildSkillMultipliers];
}
return self

Lots of duplicated code

You have 7 methods like -(void) skillUpMining;. They all do the same thing - increment a variable. Why not write one method that takes an argument specifying which skill to tweak? Keep 1 copy of the code, not 7 copies. The duplication seems harmless for such a simple method, but you have other methods that aren't so simple.

Instead of calling [gimli skillUpMining];, you'd call [gimli skillUp:miningSkill];. miningSkill, buildingSkill, etc. could be enum constants.

Inside the class, you could store the skills in an array or a dictionary. You already have a dictionary to hold the skill multipliers. You could extend the dictionary so each key tracks an object with all the "stuff" for one skill.

Continuing down the page, I keep seeing chunks of code repeated 7 times, once for each skill. There are VERY few differences between skills. They have different starting counts. The two logXXX methods use a different label for each skill. That's about it; everything else is the same for all of the skills.

You should try to design the code so that a common feature/behavior is only written once, and then used by each skill. I think the code is begging for a new class, something like DTDwarfSkill. Note the name is singular. It deals with one skill only.

Here's the key question to ask yourself about your design: "How many places in the code have to change if I add another skill?" The best answer is ONE. I think I counted 12 in your code. But I don't want to count that high (I'm lazy), so my count may be inaccurate.

You might end up keeping your current DTDwarfSkills class, but with much less in it than now, and with a revised name like DTDwarfSkillCollection.

I'll try to outline DTDwarfSkill off the top of my head...

#import 

@interface DTDwarfSkill : NSObject
@property (readonly) NSInteger skill;
@property (readonly) double skillMultiplier;

-(id) initWithStartingCount:(NSInteger)count shortLabel:(NSString *)shortLabel;
-(void) skillUp;
//etc.
@end

You want this interface to provide everything you need for one skill. I might not have captured everything.

You would create objects for miningSkill, buildingSkill, etc. like this:

DTDwarfSkill * miningSkill = [[DTDwarfSkill alloc] initWithStartingCount:8 shortLabel:@"M"];
DTDwarfSkill * buildingSkill = [[DTDwarfSkill alloc] initWithStartingCount:8 shortLabel:@"B"];
//etc.

A lot of your existing code would migrate to the implementation of DTDwarfSkill. First, a class extension to hold "private" stuff. These things help you implement the class, but they don't need to be visible to code that uses the class:

@interface DTDwarfSkill ()
@property (readonly) NSInteger startingCount;
@property (readonly) NSString * shortLabel;
-(void) computeSkillMultiplier;
@end

Then the implementation code:

@implementation DTDwarfSkill

-(id) initWithStartingCount:(NSInteger)startingCount shortLabel:(NSString *)shortLabel {
    self = [super init];
    if (self) {
        _skill = 0;
        _skillMultiplier = 0.0;
        _startingCount = startingCount;
        _shortLabel = shortLabel;
    }
    return self;
}

-(void) skillUp {
    _skill++;
}

-(void)computeSkillMultiplier  {
    if (_skill % 10 == 0 && _skill <= 100) {
        _skillMultiplier = _skill / 10 * 0.10;
    }
}

//etc.
@end

After this refactoring, what would be left for DTDwarfSkillCollection?

• It could hold the collection of DTDwarfSkill objects (in an array or a dictionary).

• It could provide a method like -(void) skillUp:(...)skill, along with a set of symbols to use in place of .... when calling the method. If you put the collection in an array, the symbols could be integers; if you use a dictionary, the symbols could be strings. This method just finds the right DTDwarfSkill object in its collection and calls that object's skill getter.

• Or you could provide a bunch of methods -(void) skillUpMining, skillUpBuilding, etc.

• It would have a method computeSkillMultipliers, which would just call the computeSkillMultiplier method on every object in the collection.

• It would have logCurrentSkills and logSkillMultiplier methods. Again, they would build the result strings by calling each object's shortLabel, skill, and skillMultiplier methods.

• etc.

I don't know what I'd suggest after these changes. I can't see beyond all the duplicate code. I'd suggest some improvements in names, but I suspect a lot of names would go away. I would revisit names after refactoring the class structure.

StackExchange Code Review Q#55139, answer score: 6