Not-so-delicious spaghetti code

Game Play & Game Design
#1

Hey, so I’m making a game and have already completed the majority of the engine. It is developed in an object-oriented fashion. The game works well, but I have one issue that I’m afraid will begin to clutter up my code before too long.

My Player class is 1,499 lines long!

I’ll admit that there are a few reasons for the lines being this long. The Player inherits a lot of different interfaces. I tend to skip a lot of lines for readability purposes. For every action the player does, he refers to an Action object within an array which forces me to make many different classes implementing the Action interface who’s definition is encapsulated within the Player.

Yeah, so the last item I mentioned is probably responsible for 700 of the 1,499 lines in the Player.java file. Basically, every action the Player does is delegated by one action in an array of actions. When I want to start another action, I call upon the method setAction(int action). Something like this happens…


/**
* Action gets assigned
*/
public void setAction(int action)
{
    // Leaves our current action by calling the exit method.  Potentially cleans up some leftovers.  I might get rid of this method
    //since it has proven to be pretty useless so far.
    actions[actionPointer].exit();

    // Assigns the action pointer and calls on its enter method for housekeeping purposes.  Action gets ready to be run.
    actionPointer = action;
    actions[actionPointer].enter();
}


/**
* Action gets run
*/
public void update()
{
    actions[actionPointer].run();
}

This paradigm is really nice because the Player’s behavior is isolated to one Action object, and the Action object can determine when the player needs to switch to another action. If you are in the Standing action and are on the ground and pressing up, then the player will go to the Jumping action. If the player is Jumping, then the check to see if the user is pressing the Up key will never happen. That check is only in the Standing action. Many local variables can be stored within each Action object because they only ever related to that action.

This paradigm is also bad because each action takes up many lines of code since each new Action must have a definition that is written in the Player class.

Well, I don’t have TOO much of a problem with this since each Action is easy to maintain, but there are also a lot of variables that simply belong to the Player class that determine if the Player is allowed to do something or not. If the Player is close to a ladder, the ladder will invoke the method consider(Climbable climbable) from the player which alerts the player that a Climbable is nearby. The player will proceed to climb the ladder if the local boolean canClimb is true. canClimb is only true if the current Action that is running says it’s true. This code is starting to become tangled…

If anyone has experience preventing this pattern from becoming a disaster, I would appreciate it if you could give me some advice. As things currently stand, I am perfectly capable of proceeding. I am just afraid that 1,499 lines of code is unreasonable for a single class. Like I said before, most of the fluff comes from having to write out new class definitions for new Action object. I would also like to mention that it is too late for me to adopt the functional paradigm with entities/components because my game engine is rather OO. I would have to scrap my entire project, excluding some resources, in order to adopt that paradigm.

#2

It’s not a bad approach from the way you describe it, but does Action really need to be defined within Player? Seems like something you could break out into its own public class. I think that as your Action logic grows, it’s going to make Player into more and more of a God Class that becomes harder to maintain unless you refactor it good and hard right now.

If Action needs information that’s private to player, you probably have a design issue to work out, but if it really really needs privileged access, then there’s ways around that. You can either use protected or package-private and put them in the same package (which I consider a hack), or Player can itself pass an adapter object into an Action that grants it access because said “capability object” is defined by an inner class in Player.

#3

Sounds like a plan. So, lemme see if this describes what you’re suggesting. I could write something like this in the player…


public void addAction(Action action)
{
    // Adds the action
    actions[size] = action;
    size ++;

    // Gives object that references local variables that are important to the player
    action.setReferences(References ref);
}

Sounds awesome, but how exactly would the adapter object ref be able to point to the local variables in Player? Would it itself have variables that point to those variables?

#4

Couldn’t you pass the Player into the Action’s constructor?

#5

Yes, but in order for the Action to be aware of the Player’s local variables, they would have to be either package-private or public which sproingie described as “hacky”. I consider it to be a little hacky as well, but it could be done. At least the hacky behavior would be isolated to the player package if the player’s variables were package-private.

#6

I’m guessing addAction is something like GDX’s scene2d actor interface? If so, then yeah that’s basically the idea.

I don’t know that I’d make it as generic as something called “References”. Maybe an interface called “ActionContext”, and an implementation of it called “PlayerActionContext”. Then you’d have something like this:



private Stuff superSeekritThing;

// ActionContext is an interface and public
private class PlayerActionContext implements ActionContext {
  @Override
  public Stuff getSeekrit() { return superSeekritThing; }
}

public void addAction(Action action)
{
    // Adds the action
    actions[size] = action;
    size ++;

    // You can always pre-instantiate this if all it does is poke holes in access
    action.setContext(new PlayerActionContext());
}

You do have to think hard about what’s allowed to be accessed in an ActionContext, because you’re restricted to that generic interface, though you could subclass it for Actions that only make sense to be initiated by a Player and put more methods on that interface. Just don’t let your interface become another God class either.

#7

Hrrm… Okay, then. Aside from a poor naming decision, I pretty much had the idea down. One last question, though… Currently, the Player has a couple of Actions that expect other actions to exist. The Player’s standing action expects the Running action to exist at a particular index. To refer to it, I say something like…


class RunningAction implements Action
{
    // Housekeeping
    public void enter()
    {
    }

    // Runs the action
    public void run()
    {
        // Player loops animation :D

        // Player tries to go to standing action if player is in a particular state
        if(Math.abs(velovityX) > 0 && bottomTouched)
            setAction(ACTION_RUNNING);
    }

    // Usually unnecessary housekeeping
    public void exit()
    {
    }
}

In this case, ACTION_RUNNING is a public static final integer. The Running action object is expected to be at a particular index. If I’m adding on actions on the fly, how exactly will I be able to anticipate another action’s position when the position may vary depending on the time it was added? I would rather not search for it using an instanceof check, because, you know, EVIL!!!

Maybe the Action could require another Action object in it’s constructor so it can always point to it? Then the setAction method could be more like…


public void setAction(Action action)
{
    for(int i=0; i<actions.length; i++)
    {
        if(action == actions[i])
        {
            actions[actionPointer].exit();
            actionPointer = i;
            actions[actionPointer].enter();
        }
    }
}

I could always keep the old setAction() method and just overload the name.

#8

I put an example of the “capability object” design up here. If you have maven you can just use run.sh (if you’re on windows, you do have cygwin, right?) Otherwise you can import the POM. Or just eyeball it, it’s pretty simple

http://dl.dropbox.com/u/52548846/actout.tgz

#9

I don’t recommend requiring any action to need others to exist in any specific place. If an Action depends on another one that’s currently running, that sounds like exactly the sort of thing that should be stuck in an ActionContext, something like [icode]public Action getCurrentAction()[/icode]. Where you choose to stuff the current action is then no longer Action’s problem, but your implementation of ActionContext instead.

I would also recommend using Enums and not integer constants. Also look into ArrayDeque for storing actions instead of the array you’re maintaining by hand. If the synchronization overhead is too much, and you only ever use it from one thread, and strict order isn’t important, the poorly-named Array class from libGDX might be what you want (it’s what scene2d uses).

#10

I see, I see. When I made the actions, I wanted them to be an easy way to break up instructions that were already going to exsist in the player from the start. Now I want to take a more flexible approach, though.

Also, in response to your previous post…

Did you just write that JUST now? If so, then I have to applaud you for your helpfulness! Um, but are you suggesting that I get the player’s local variables from a string??? I kind of have a problem with that… Sound a little, uh, dynamic… And not very performant. I might have to disagree with your specific style. After all…

550×560 50.9 KB

I definitely like the overall idea that you’ve put forth. I will not have my Action objects be expected to be located anywhere in particular. I’ll also probably have my actions be defined outside of the Player for a more flexible result.

#11

Christ no, it’s called an example ::slight_smile: It doesn’t have to be a String, it could be anything at all you want it to be. The idea is you’re passing an inner class to give another class “friend” access to the outer class, but with more granularity than C++'s ‘friend’ or protected/package-private. The implementation of the inner class is pretty well unrestricted in what it can do, but the interface it implements should at least inform it as to the limits of its abilities.

#12

Muahahaha! Okay, you had me worried there! Kinda similar to the event paradigm. I did something like that with collision code. Each collision object had the four methods hitLeft(), hitRight(), hitTop(), and hitBottom(). Each took a CollisionEvent argument. It this case, though, the object just acts as a carrier for local variables.

#13

You are definitely better off making action handling as stateless as possible, which lessons the need for Action to have any kind of privileged access – it just gets passed the information it needs, and returns any transformations it does on the environment, to be executed by some kind of “action resolver” object. This can obviously be taken to ridiculous extremes, but you should at least keep statelessness in mind while designing things

I just wish my current project allowed for statelessness easily. I might just write a pure functional fork of it just to have something to compare it against :stuck_out_tongue:

#14

I’ve run into another problem, although this one is slightly smaller. It still involves the Player, so I’ve decided to post my problem to the same thread.

I’ll give an example of a section of my code that already does work. I have a data structure that holds both objects that enforce collisions (Colliders), and objects that have collisions enforced on them (Collidables). The data structure loops through both lists and compares each element in both lists. The Collider’s method enforceCollision(Collidable collidable) is invoked by the data structure which allows it to communicate with the Collidables that are contained. When a collision happens, a Collider positions a Collidable depending on the rules of collisions for the particular Collider enforcing the collision. Then, the Collider it is supposed to invoke one of these methods of the Collidable: hitLeft(), hitRight(), hitTop(), hitBottom(). Each method takes in a CollisionEvent object that describes the collision itself.

This works because the Collider is the one enforcing the hit. It is in control of the collision, and the Collidable (often the Player) merely needs to be alerted about what side was hit, and a few pieces of information about the collision itself so that it may decide what animation needs to play, how it should move based on the surface of the Collider, etc.

Now, I’ll give an example of a part of my code that I consider a little broken. Let’s say I have a data structure that takes these two types: Climbable, and Climber. Climbable can be a ladder, a vine, etc, and Climbable can be an Enemy, the Player, or some freaky spider, or WHATEVER! The Climbable might have a method called alertPresence(Climber climber) which alerts a Climber of its presence when it is in a good state for climbing. You can’t climb a fence when it’s fallen sideways on the ground, but it’s still a fence that has the potential to be climbed later. When it is in a good state, it alerts the Climber (usually the Player) by invoking its considerClimbing() method while passing itself as an argument. In this case, though, even though the Climbable says it’s ready to be climbed, we still need permission from the Player. If a fence is all set up and the Player is within range, from the Ladder’s perspective, it is ready to be climbed. The Player may decide, however, that it cannot climb while it is running or swimming. What I do currently is have the Player decide if it can be climbed RIGHT at the time that the considerClimbing() method is invoked by the Climbable. I decide if it can climb with a boolean flag which is determined to be either true or false depending of the action that is currently running. Problem is, this means that for all actions that may happen, they need to decide if they are in a state that allows the Player to climb or not. They have to be aware of this flag. Yeah, that’s not gonna work in the long run.

Then main difference between the physics interface and the climbing interface is who gets to decide how something happens. With collision, Colliders (walls) are the entities that decide how the collisions play out. In the case of climbing, however, Climbables simply alert the Climbers when climbing is possible, and the Climbables (the Player, usually) get the last word in. The Climber is doing the climbing, not the Climbable.

Mostly, I want to avoid making a bagillion and a half boolean flag variables in the Player that determine if it is ready to enter a certain action. I was thinking that I could add a data structure in the Player like a queue, or something. Every time the Climber’s considerClimbing() method is invoked, it will add the Climbable passed to the queue. Then, the it will determine if it can do anything with the Climbable(s) in its current state. If the Player has an Action object running that doesn’t even consider climbing, then the climbing queue will simply be ignored and cleared for the next update. If the Action that is running DOES say something about climbing and the Player has at least 1 climbable stored, then the Player will try to climb the Climbable(s). Of course, I don’t have to have a queue, or even a list for that matter. I could simply say “last come first serve” and only pay attention to the last Climbable that made its presence known and store it in a local variable.

By this logic, the considerClimbing() method would simply alert the Climber(Player) of Climbables (Ladder, maybe?) that could potentially be climbed in an update. The Climber would get the last word in.

What say someone else?

#15

I’m not sure I see the purpose of having interactive objects advertise their capabilities all the time. Why not just have the player poll for climbable objects in the vicinity when initiating a climbing action? No permission is needed, since the player initiated it. If one action blocks the possibility of another, you simply don’t let the player initiate the action.

#16

Hmm, okay. Yeah, that makes more sense. Since the Player is the one doing the climbing, it should be making the first move. Climbables could simply ignore the Player’s request to climb if it is in a bad state. I kind of wanted to avoid having the Player contain a reference of a collection of other Entities, though. There may be multiple collections that are used for a selective purpose. The purpose may vary from room to room. I am using an external data structure that acts as a bridge between two different types of entities. The way that two types communicate is by having their methods invoked by the data structure. Problem is, the Entities may not be ready to consider the data entered. It may be optimal for the entities to consider other entities in their update method. I’m kind of stuck trying to decide if I want collections of other entities to be stored by the entities themselves, or by external data structures that link entities together through method invocation.

#17

The player doesn’t need to reference the other entities directly, you’d get them by querying your map for entities near the player’s location, using BSP trees or grid hashing or some other heuristic, since the first criteria you’re narrowing your entity query down with is “nearby”.

#18

Does this mean that I need a new data structure for every new type of Entity I create? You’re suggesting that the map contain all of the Entities in some sort of partitioned way which sounds nice. I’m a little unclear as to how you know what type of Entity is nearby, though. For all the map data structure knows, every entity is just an entity and nothing more. In order to choose entities of a particular type, I would either have to make separate data structures for each type of entity, or use the ugly instanceof solution.

#19

The map doesn’t need to know anything about entities except keep track of where they are. Once you narrow down the list of nearby entities, you can always just query them all for their capabilities, and have a default implementation that does nothing. If that’s too many entities, then you could divide the map into layers containing specific entity subclasses (or better, interfaces) and only query the relevant layer of the map. That’s basically “separate data structures”, yes, but in a reasonably controlled manner.

The general problem of narrowing down Entities at runtime by their role in the game isn’t something you can always enforce easily in static types, it’s just one of those areas where using OOP to model real-life objects tends to break down. Using “instanceof” isn’t something I ever recommend, but you might still occasionally have to ask the runtime in some other way whether an object is an appropriate target for an action. Use static types to keep yourself from making nonsensical queries or statements, like trying to, oh, quaff a GameSaveSlot, then runtime predicates to ensure you’re not swinging a Shopkeeper or talking to a Sword – unless the sword’s sentient or the shopkeeper is consenting! :wink:

#20

Alright, I think I have somewhere to go from here. Gonna be a while before I can mass produce anything because I must refactor my code. May need to alter how a bunch of classes to get this working again. At least they’re on the outskirts of my code and not the core. I honestly appreciate the advice I’ve been given!