6
\$\begingroup\$

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 <Foundation/Foundation.h>

@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;
}
-(void) buildSkillMultipliers {
    NSMutableDictionary *tempDictionary = [[NSMutableDictionary alloc]init];
    [tempDictionary setObject:[NSNumber numberWithFloat:0] forKey:miningSkillMultiplier];
    [tempDictionary setObject:[NSNumber numberWithFloat:0] forKey:buildingSkillMultiplier];
    [tempDictionary setObject:[NSNumber numberWithFloat:0] forKey:workRoomSkillMultiplier];
    [tempDictionary setObject:[NSNumber numberWithFloat:0] forKey:fightingSkillMultiplier];
    [tempDictionary setObject:[NSNumber numberWithFloat:0] forKey:cleaningSkillMultiplier];
    [tempDictionary setObject:[NSNumber numberWithFloat:0] forKey:haulingSkillMultiplier];
    _skillMultipliers = [[NSMutableDictionary alloc]initWithDictionary:tempDictionary];
}

#pragma mark - Skill Increases
-(void) skillUpMining {
    _miningSkill++;
}
-(void) skillUpBuilding {
    _buildingSkill++;
}
-(void) skillUpWorkRoom {
    _workRoomSkill++;
}
-(void) skillUpFighting {
    _fightingSkill++;
}
-(void) skillUpCleaning {
    _cleaningSkill++;
}
-(void) skillUpHauling {
    _haulingSkill++;
}

#pragma mark - Compute Skill Multipliers
-(NSMutableDictionary *) skillMultipliers {
    [self computeSkillMultipliers];
    return _skillMultipliers;
}
-(void) computeSkillMultipliers {
    if (_miningSkill % 10 == 0 && _miningSkill <= 100) {
        NSNumber *tempNumber = [_skillMultipliers objectForKey:miningSkillMultiplier];
        int multiplier = _miningSkill / 10;
        tempNumber = [NSNumber numberWithFloat: multiplier * 0.10];
        [_skillMultipliers setObject:tempNumber forKey:miningSkillMultiplier];
    }
    if (_buildingSkill % 10 == 0 && _buildingSkill <= 100) {
        NSNumber *tempNumber = [_skillMultipliers objectForKey:buildingSkillMultiplier];
        int multiplier = _buildingSkill / 10;
        tempNumber = [NSNumber numberWithFloat: multiplier * 0.10];
        [_skillMultipliers setObject:tempNumber forKey:buildingSkillMultiplier];
    }
    if (_workRoomSkill % 10 == 0 && _workRoomSkill <= 100) {
        NSNumber *tempNumber = [_skillMultipliers objectForKey:workRoomSkillMultiplier];
        int multiplier = _workRoomSkill / 10;
        tempNumber = [NSNumber numberWithFloat: multiplier * 0.10];
        [_skillMultipliers setObject:tempNumber forKey:workRoomSkillMultiplier];
    }
    if (_fightingSkill % 10 == 0 && _fightingSkill <= 100) {
        NSNumber *tempNumber = [_skillMultipliers objectForKey:fightingSkillMultiplier];
        int multiplier = _fightingSkill / 10;
        tempNumber = [NSNumber numberWithFloat: multiplier * 0.10];
        [_skillMultipliers setObject:tempNumber forKey:fightingSkillMultiplier];
    }
    if (_cleaningSkill % 10 == 0 && _cleaningSkill <= 100) {
        NSNumber *tempNumber = [_skillMultipliers objectForKey:cleaningSkillMultiplier];
        int multiplier = _cleaningSkill / 10;
        tempNumber = [NSNumber numberWithFloat: multiplier * 0.10];
        [_skillMultipliers setObject:tempNumber forKey:cleaningSkillMultiplier];
    }
    if (_haulingSkill % 10 == 0 && _haulingSkill <= 100) {
        NSNumber *tempNumber = [_skillMultipliers objectForKey:haulingSkillMultiplier];
        int multiplier = _haulingSkill / 10;
        tempNumber = [NSNumber numberWithFloat: multiplier * 0.10];
        [_skillMultipliers setObject:tempNumber forKey:haulingSkillMultiplier];
    }
}

#pragma mark - Logging for Testing
-(void) logCurrentSkills {
    NSLog(@"M=%i, B=%i, W=%i, F=%i, C=%i, H=%i", _miningSkill, _buildingSkill, _workRoomSkill, _fightingSkill, _cleaningSkill, _haulingSkill);
    [self computeSkillMultipliers];
    [self logSkillMultipliers];
}
-(void) logSkillMultipliers {
    NSLog(@"M=%@, B=%@, W=%@, F=%@, C=%@, H=%@", [_skillMultipliers objectForKey:miningSkillMultiplier], [_skillMultipliers objectForKey:buildingSkillMultiplier], [_skillMultipliers objectForKey:workRoomSkillMultiplier], [_skillMultipliers objectForKey:fightingSkillMultiplier], [_skillMultipliers objectForKey:cleaningSkillMultiplier], [_skillMultipliers objectForKey:haulingSkillMultiplier]);
}

@end

Now the relevant code inside the worker class. First I need the constants:

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";

static const int startingMiningCount = 8;
static const int startingBuildCount = 8;
static const int startingWorkRoomCount = 20;
static const int startingFightCount = 10;
static const int startingCleanCount = 30;
static const int startingHaulCount = 4;

Then this is the method called when the worker updates:

-(void) checkSkills {
    NSMutableDictionary *tempDictionary = [_dwarfSkills skillMultipliers];

    NSNumber *miningMultiplier = [tempDictionary objectForKey:miningSkillMultiplier];
    _miningDelayCount = [self computeSkillForSkillMultiplier:miningMultiplier andSkillValue:startingMiningCount];

    NSNumber *buildingMultiplier = [tempDictionary objectForKey:buildingSkillMultiplier];
    _buildDelayCount = [self computeSkillForSkillMultiplier:buildingMultiplier andSkillValue:startingBuildCount];

    NSNumber *workRoomMultiplier = [tempDictionary objectForKey:workRoomSkillMultiplier];
    _roomUpgradeDelayCount = [self computeSkillForSkillMultiplier:workRoomMultiplier andSkillValue:startingWorkRoomCount];

    NSNumber *fightingMultiplier = [tempDictionary objectForKey:fightingSkillMultiplier];
    _fightDelayCount = [self computeSkillForSkillMultiplier:fightingMultiplier andSkillValue:startingFightCount];

    NSNumber *cleaningMultiplier = [tempDictionary objectForKey:cleaningSkillMultiplier];
    _cleanDelayCount = [self computeSkillForSkillMultiplier:cleaningMultiplier andSkillValue:startingCleanCount];

    NSNumber *haulingMultiplier = [tempDictionary objectForKey:haulingSkillMultiplier];
    _pickupItemDelayCount = [self computeSkillForSkillMultiplier:haulingMultiplier andSkillValue:startingHaulCount];
}
-(int) computeSkillForSkillMultiplier:(NSNumber *)multiplier andSkillValue:(int)skillValue {
    int oldSkillValue = skillValue;
    float numberToSubtract = [multiplier floatValue] * oldSkillValue;
    int newSkillValue = oldSkillValue -= numberToSubtract;
    if (newSkillValue < 1) {
        newSkillValue = 1;
    }
    return newSkillValue;
}

I'm sure there are lots of ways to improve this, so please be as critical as you like. You won't hurt my feelings.

\$\endgroup\$
6
\$\begingroup\$

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 <Foundation/Foundation.h>

@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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your input on this. I had a feeling I was Doing It Wrong. I suppose all of the code duplication was an effort to make the class usable from the outside with a minimum of knowledge. But I should have used an enum instead, as well as having a skill for each class instead of handling it all in one place. Excellent answer! \$\endgroup\$ – bazola Jun 25 '14 at 10:58
  • 1
    \$\begingroup\$ I agree with Robert's suggestion for the skill class and the skill collection object to manage a collection of skills. It's better for the longer term. (Each dwarf has a skill collection, each collection has multiple skills--you could even create dwarf classes limiting each dwarf to say 5 skills even if there are numerous more to choose from etc.) HOWEVER, I do want to point out... I don't mind each skill having it's own method for convenience... but Robert is right, the logic for leveling up should all be in a single method. \$\endgroup\$ – nhgrif Jun 26 '14 at 11:31
5
\$\begingroup\$
static const NSString* miningSkillMultiplier = @"miningSkillMultiplier";

None of these lines are going to do exactly what you want them to do.

First of all, it is helpful if our constants follow the well established constant naming pattern by starting with a lowercase k and then the rest of the variable name in capitals.

For example: kMiningSkillMultiplier.

This helps us identify the variable as a constant value throughout the code. Although this variable isn't the multiplier, is it? It's a key to a dictionary to get the multiplier, right? So that should be made more clear in the variable name I feel.

Now then, the the more important part:

const NSString * varName;

This creates a pointer to a const NSString. NSString is already immutable so this is basically pointless.

What you instead need is this:

NSString * const varName;

This creates as const pointer to an NSString variable. Using NSString versus NSMutableString is enough to guarantee the contents of the object never change. What we actually want is to guarantee the memory location never changes.

And why do we want this?

All of the Foundation classes and any good custom class has an isEqual: method (string has isEqualToString: which is the same method in implementation for strings). In order to optimize this method, the first line usually looks something like this:

if (self == object) return YES;

And then continues on with other compares if that comparison returns false. The reason is, if these two pointers point to the same object, they're definitely equal.

Using NSString * const varName; means every variable we have to represent this constant immutable object is a pointer to the same memory location, so any time we do a compare (which is what is done when we use it as a key in a dictionary), we can take the optimized route through the isEqualToString: method.

\$\endgroup\$
  • \$\begingroup\$ Thanks for this, I did not realize that it wasn't doing what I expected it to do. It really helps to get an explanation of what is going on behind the scenes. \$\endgroup\$ – bazola Jun 26 '14 at 17:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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