5
\$\begingroup\$

I'm working on a basic strategy game for iOS and I have a question about the overall layout of the code in the program. I added some features to the game today and I had to add code to several different places to get the features to function properly. In general I think I am doing things correctly, but I fear I am falling into common programming pitfalls that I do not yet know about. In this question I will walk through the features I wanted to add to the program, and the way I approached them.

To start with I had basic enemies in the game that were randomly created on floors of the tower given certain conditions. When spawned they just stayed in place and did nothing. So I looked at the class and saw that once the enemies were created, there was no logic present to decide what they would do. So I added some logic to tell the enemies to move to attack the nearest wall if one was present on the floor that they spawned on.

Next I wanted to make the workers react to the presence of enemies on a floor. The floors have an enum that defines their state. After the tower creates an enemy, it immediately sends it to the correct floor. The floor accepts it with this code:

-(void) acceptEnemy:(DTEnemy *)enemy {
    [self.enemyArray addObject:enemy];
    if (self.floorState != FloorUnderAttack && self.floorState != FloorDestroyed) {
            self.floorState = FloorUnderAttack;
    }
}

Next I needed to cause the workers on the floor to react to the presence of the enemy. I did so with the following code:

-(void) updateFloor {
    switch (self.floorState) {
        case FloorUnderAttack:
            [self cancelJobDueToAttack];

            [self makeDwarvesFlee];

            [self handleEnemies];
            break;

This method also deals with the regular case of when the floor is not under attack, but that is not so important here. I also needed to add this code:

-(void) cancelJobDueToAttack {
    [self abandonCurrentJob];
    [_jobArray removeAllObjects];
    [_availableJobSlots removeAllObjects];  //should be redundant
    [_completedJobUnits removeAllObjects];
}

Basically this clears out all remnants of the job in the queue so that it cannot cause any problems in the future.

Now I needed to prevent the workers from accepting jobs on the floor if it was under attack, which I did so by preventing them from entering the floor in this case:

-(void) sendDwarvesToFloors {
    NSMutableArray *dwarvesInTower = [[NSMutableArray alloc]init];
    for (DTDwarf *dwarf in self.towerDwarves) {
        if (dwarf.dwarfMovement.dwarfMovementState == DwarfAtDestinationFloor && dwarf.age < 13) {
            dwarf.dwarfMovement.dwarfMovementState = DwarfIdleMovement;
        }
        if (dwarf.dwarfMovement.dwarfMovementState == DwarfAtDestinationFloor) {
            NSNumber *floorNum = [NSNumber numberWithInt:dwarf.dwarfMovement.destinationFloor];
            DTTowerFloor *tempFloor = [self.towerDict objectForKey:floorNum];
            if (tempFloor.floorState == FloorUnderAttack) {
                if (dwarf.jobPermissions & FightingJobsAllowed) {
                    [tempFloor acceptDwarf:dwarf];
                }
            } else if (tempFloor.floorState == FloorDestroyed) {
                if (dwarf.jobPermissions & CleaningJobsAllowed) {
                    [tempFloor acceptDwarf:dwarf];
                }
            } else {
                [tempFloor acceptDwarf:dwarf];
            }
        } else {
            [dwarvesInTower addObject:dwarf];
        }
    }
    self.towerDwarves = dwarvesInTower;
}

Now I wanted to make it so the enemy could destroy the walls, as well as other things. I did so with the following method:

-(void) handleEnemies {
    for (DTEnemy *enemy in self.enemyArray) {
        if (enemy.enemyMovement.enemyMovementState == EnemyJustSpawned) {
            if (self.floorBuildState & FloorHasWalls) {
                int positiveOrNegative = 0;
                if (enemy.enemyMovement.currentPosition.x >= 0) {
                    positiveOrNegative = 1;
                } else {
                    positiveOrNegative = -1;
                }
                enemy.enemyMovement.destinationPosition = CGPointMake((_worldSize.width / 2.1) * positiveOrNegative, enemy.enemyMovement.currentPosition.y);
                enemy.enemyMovement.enemyMovementState = EnemyMovingToAttackPosition;
            }
        }

        if (enemy.enemyMovement.enemyMovementState == EnemyInPositionForAttack) {
            if (self.floorBuildState & FloorHasWalls) {
                if ([self doDamageToWalls]) {
                    self.floorBuildState &= ~FloorHasWalls;
                    enemy.enemyMovement.destinationPosition = _floorCenterPosition;
                    enemy.enemyMovement.enemyMovementState = EnemyMovingToFloorExit;
                }
            }
        }

        [enemy updateEnemy];
    }
}

Now the enemy would move to the location of the walls and begin to destroy them once they reached that position. Unfortunately I had to add ivars to handle the health of the walls on the floor for this to work.

Now I wanted to finish creating the jobs to deal with the aftermath of the presence of the enemies on the floor. I will call these Cleaning Jobs and Fighting Jobs. To do so, I needed to modify many parts of the code, and this is really the heart of the question because I had to change so many different things to make everything work properly.

I needed to add new jobs to the jobs enum and to the job permissions enum. Then, I went ahead and started creating methods to add the new jobs to the queue. I looked at the method that actually adds new jobs, and saw that it itself did not need to change:

-(void) addJobToQueueOfType:(JobType)jobType forFloor:(int)floorNumber {
    NSNumber *floorNum = [NSNumber numberWithInt:floorNumber];
    DTTowerFloor *floor = [self.towerDict objectForKey:floorNum];
    DTJob *job = [[DTJob alloc]initWithType:jobType];
    [floor addJob:job];
}

I looked above this method located in the tower class, to the game class that calls it and saw this:

-(BOOL) addJobToQueueOfType:(JobType)jobType forFloor:(int)floorNumber {
    BOOL wasSuccessful = NO;
    DTTower *tempTower = [_towerArray objectAtIndex:_currentTower];
    if ([self hasEnoughResourcesForJob:jobType]) {
        if ([tempTower checkIfFloor:floorNumber isValidForJob:jobType]) {
            [tempTower addJobToQueueOfType:jobType forFloor:floorNumber];
            wasSuccessful = YES;
        }
    }
    return wasSuccessful;
}

So now I finding things that need to change. First, I need to add cases for hasEnoughResourcesForJob for all of the new jobs, like so:

-(BOOL) hasEnoughResourcesForJob:(JobType)jobType {
    BOOL hasEnoughResources = NO;
    switch (jobType) {
        case MiningJob:
            hasEnoughResources = YES;
            break;
        case LadderJob:
            if (self.currentCommonResources >= _ladderCost) {
                hasEnoughResources = YES;
            }
            break;
        case BottomBuildJob:
            if (self.currentCommonResources >= _bottomCost) {
                hasEnoughResources = YES;
            }
            break;
        case WallBuildJob:
            if (self.currentCommonResources >= _wallCost) {
                hasEnoughResources = YES;
            }
            break;
        case RoomBuildJob:
            if (self.currentCommonResources >= _roomCost) {
                hasEnoughResources = YES;
            }
        case SuperiorWallBuildJob:
            if (self.currentCommonResources >= _superiorWallCost) {
                hasEnoughResources = YES;
            }
            break;
        case HaulJob:
            hasEnoughResources = YES;
            break;
        case FightingJob:
            hasEnoughResources = YES;
            break;
        case CleaningJob:
            hasEnoughResources = YES;
            break;
        default:
            break;
    }
    return hasEnoughResources;
}

Then, I needed to add entries to the checkIfFloor:isValidForJob: method like so:

    case SuperiorWallBuildJob:
    {
        NSInteger requiredBuildings = (FloorHasBottom | FloorHasWalls);
        if ((tempFloor.floorBuildState & requiredBuildings) == requiredBuildings) {
            isValidForJob = YES;
        }
        break;
    }
    case FightingJob:
    {
        if (tempFloor.floorState == FloorUnderAttack) {
            isValidForJob = YES;
        }
        break;
    }
    case CleaningJob:
    {
        if (tempFloor.floorState == FloorDestroyed) {
            isValidForJob = YES;
        }
        break;
    }
    default:
        break;
}

Then I needed to add methods to create the products of these jobs once they were completed like so:

-(void) buildSuperiorWallsOnFloor:(DTTowerFloor *)floor {
    [self addResourceForCollectionWithCommon:_superiorWallCost andRare:0];
    floor.floorBuildState |= FloorHasSuperiorWalls;
    self.hasTowerChanged = YES;
}

And I also needed to add values for the cost of the jobs, as well as adding cases to the method that subtracts the cost of the job from the total resources once it is completed.

Now I needed to modify the UI code to add buttons for each of the new job types. When pressed, these buttons check with the game to make sure the job is acceptable, and then pass the job to the tower if this is so. Then, the tower passes the job to the floor. If jobs are present on the floor and the worker has the proper permissions, they will go to the floor and carry out the jobs.

As you can (hopefully) see, just to add one new building or job to the game, I needed to modify code in several different classes. I needed to modify the enums for the job permissions as well as for the jobs themselves. I needed to add a new type of building to the enum for building types. I needed to add cases to check for the permissions for the new jobs for both the state of the floor and for the job permissions of the workers. And I needed to check the costs of the jobs and also decrement their costs once the buildings were complete. Also I needed to add special cases for the methods that add a certain number of jobs to the floor based on the type of job and the state of the floor. And, I am sure there are pieces of code that I had to add that I am forgetting to mention here.

Is this approach common or typical, or am I doing things wrong? Is it normal to have to modify many parts of the code to add this type of functionality? Is there a way to group these things together in a common class? It seems to me that their responsibilities are very different. It seems to me that pretty much I would have to keep documentation on hand listing all the places where I need to add code in order to add to certain features, or I would need to have that memorized. Is this the proper approach, or am I missing something?

\$\endgroup\$
  • 1
    \$\begingroup\$ if (tempFloor.floorState == FloorUnderAttack) and if (tempFloor.floorState == FloorDestroyed) -- you probably need to replace the == with & for your bitmask here. \$\endgroup\$ – nhgrif Jun 18 '14 at 11:33
  • \$\begingroup\$ Actually the floorState is just a regular integer enum. Thank you though! \$\endgroup\$ – bazola Jun 19 '14 at 22:21
2
\$\begingroup\$

I can't offer any specific advice for how better to approach this specific problem, as I don't have my head completely wrapped around all that your project encompasses. Moreover, for this question, I think specific advice isn't particularly helpful. Even if I had a solution that could make the current and near-future situation better, this question is about seeing way beyond that.

There's nothing more frustrating to me as a programmer then to spend several days working on something, only to then realize, "Wow. This will actually never work. The entire structure is flawed to the core so badly that I can only restart from basically scratch." Yet, it happens. You should always be trying to think beyond the current scope of your project and attempt to future proof it as much as possible, but it's not always possible. So when you run into a situation where you have to update a lot of existing code to make new features work, don't feel too down on yourself.

But when you have time, look at that scenario, and try to imagine a way in which it could have been avoided. And by that, I don't mean how your past-self could have avoided it equipped with the knowledge of your current-self. Try to think of how your past-self could have avoided the scenario with the exact amount of knowledge your past-self had.

Thirty minutes of planning could save 3 days of recoding. A day of planning could save months of recoding time. Before you spend too much time coding, sit down, away from a computer and do some planning. Do some formal planning where you're outlining the plan, taking notes, etc., and do some informal note-taking as well while you're actually doing something else. Often times, my best ideas on how to tackle a programming challenge come while I'm pre-occupied with some other activity, sleeping, showering, whatever.

During this planning, the first thing to consider is how best to structure your code so that you will easily be able to include every feature that you currently have planned and will for sure be including at any point in time, as well as try to imagine and future features that you may want to include in the future.

Once you've got a bit of an idea on how to model your code, spend some more time looking for its glass ceilings. In what areas is your model going to limit you from as of yet unforeseen features and is there an way to change your model now so that this is not difficult in the future?

In this case, I'd argue that the changes you had to make only seem big because you're not breaking it down into the group of smaller changes that had to be made.

If the only task were to add enemies, that part seemed relatively painless.

If the only task were to add a single new job, that alone doesn't seem to bad.

The problem is, in order to add enemies to the game (a quite big feature), that required adding 3 new jobs. The total task of adding enemies and 3 new jobs seems like a lot. But if you broke it down into the 4 tasks of adding enemies and 3 new jobs, on balance, each individual task isn't that much.

If you were working for a boss, and your boss said "We want to be able to make the dwarves dance on a floor. Can you have this done in an hour?" What would happen? You'd spend two minutes adding the dance job, and 58 minutes adding the dancing animation.

So perhaps the real lesson here is to remember to look at your own tasks as a collection of smaller tasks, which are in turn, a collection of smaller tasks. Every time you add a job, you'll have to update any switch which switches on jobs. The job-specific methods arguably could all be a single method with a switch also. And so if you already had the necessary jobs in place, all that would've been left is writing the logic for the enemies... which is the feature you were adding, so I can't really see too much complaint about having to put in this effort.

\$\endgroup\$
  • \$\begingroup\$ Well I wasn't trying to complain about the amount of work involved. I think that your advice about planning is very important. Sometimes I approach programming problems through planning beforehand, and other times I approach them by building one method at a time that produces a piece of the puzzle that I know (or believe) that I will need, and then piece them together. The issue is trying to remember all the different pieces that need to be modified, and having to build, compile and test the program a number of times to discover the ones missed. I do appreciate the advice though, thank you! \$\endgroup\$ – bazola Jun 19 '14 at 18:53
  • \$\begingroup\$ Sorry for implying that you were complaining. That wasn't my intent. Was just trying to mainly highlight that there's a difference between new methods you have to add and things that could've already been handled with preplanning, etc. \$\endgroup\$ – nhgrif Jun 19 '14 at 21:10
  • \$\begingroup\$ No offense taken I just wanted to be clear. Thanks again. \$\endgroup\$ – bazola Jun 19 '14 at 22:21
1
\$\begingroup\$

On a quick scan of your code, I noticed that you missed a break; in the case RoomBuildJob:. Missing a break will cause control to fall through to the next case, which will probably do the wrong thing.

\$\endgroup\$
  • 1
    \$\begingroup\$ And if the missing break is deliberate, always add a comment stating it. Sooner or later, you'll need to remind yourself (or others) \$\endgroup\$ – Glenn Rogers Jun 19 '14 at 8:01
  • \$\begingroup\$ Definitely was not deliberate. Thanks for looking at the code and finding this bug for me. \$\endgroup\$ – bazola Jun 19 '14 at 18: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.