# C++ Code Organization / Refactoring

This topic is 3327 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

I need some advice on how to organize my code. Specifically, I have a creature in my game; I am building this generic class so that I can learn how to do more specific things to add to my game world. I am using C++. I apologize in advance for the verbosity, but I wanted to explain my situation as thoroughly as possible so that I might obtain useful advice. Thanks for reading. So let's say I have a class (class.cpp):
#include "creature.h"

class Creature
{
private:
// private member variables, etc here.
int xCo;  // creature's x-coordinate.
int yCo;  // creature's y-coordinate.

public:
// member functions here.
// member functions for AI.
}

I set this class up to be basic: it moves in the game world and its position is recorded using private member variables xCo and yCo. The creature is animated. It responds to collision detection. It has a wandering member function. It is updated and rendered; all the basic stuff. Then I added A* path finding. Path finding (for my code), so far only needs two sets of data: 1) where the creature currently is (xCo, yCo) and 2) where it is going. I created two more files: pather.h and pather.cpp. I then modified the creature class to:
#include "creature.h"
#include "pather.h"

class Creature
{
private:
// private member variables, etc here.
int xCo;        // creature's x-coordinate.
int yCo;        // creature's y-coordinate.
Pather pather;  // create path finding interface.

public:
// member functions here.
// member functions for AI.
}

Now the pather object, in my code, only needs access to the Creature class private member data int xCo and int yCo, so I just passed that when calling the pather functions. Now I am refactoring the AI portion of the Creature class code. I wish to create another class, called AI, and utilize it in a similar manner as the aforementioned Pather class. So I created AI.h and AI.cpp. The Creature class code can then be modified to:
#include "creature.h"
#include "pather.h"
#include "ai.h"

class Creature
{
private:
// private member variables, etc here.
int xCo;        // creature's x-coordinate.
int yCo;        // creature's y-coordinate.
Pather pather;  // create path finding interface.
AI ai;          // create artificial intelligence interface.

public:
// member functions here.
// member functions for AI no longer are here.
// these functions are in the AI class.
}

Here is my overall organization question: I am refactoring the already existent AI code from the Creature class. I put all of the member variables that only AI functions use as private member variables in the AI class. However, unlike the Pather class which only used the Creature class's private variables xCo and yCo, the AI class needs access to a considerably larger portion of the Creature class's private variables. What is the best way for the AI class to obtain access to these variables? I could use get() and set() member functions, friend class, or ???? What can I do here? What is the wise way in which to organize this code? Thanks in advance for any suggestions.

##### Share on other sites
Quote:
 What is the best way for the AI class to obtain access to these variables?

The AI class shouldn't have access to the creature's variables. Instead, the creature should provide functions that the AI class would call, and then the function would determine whether or not the creature can do whatever the AI is telling it to do.

For example, imagine the AI class as the brain of the creature. The brain would not tell the creature to reposition itself at (x,y), instead the brain would tell the creature to move to (x,y). In code,

NO
creature.x = 12;
creature.y = 20;

YES
creature.moveTo(12, 20);

##### Share on other sites
I would use interfaces combined with get/set. In your case it would look like:

class AiCreatureInterface
{
public:
// declare all member variables necessary for AI
virtual int getXCo()=0;
virtual void setYCo(int)=0;
virtual int getYCo()=0;
...
}

class Creature : public AiCreatureInterface
{
..
public:
int getXCo() { return xCo;}
...

}

Your Ai class should look like:

class Ai
{
...
void processAI( AiCreatureInterface& creatureInterface);

}

You could use the same for the path finding class with an other set of declared member variables.

--
Ashaman

##### Share on other sites
Thank you both for the responses. I have been thinking about this code organization problem for most of today, scouring the web, reading all I could on the matter.

I was thinking about something using something like
class Creature : public AiCreatureInterface{.. etc.
I just didn't know how others handle this situation; I wasn't sure if I was on the right track. Now I am thinking you both have good points and I shall head down that path. I am just trying to write and maintain organized and well-commented code since this project is the longest I've written and I need to give myself the best chances of completing it.

Thanks again. Take it easy.

##### Share on other sites
'Controller' type objects such as pather and ai typically shouldnt be separate objects following a strict OOP design. They serve no purpose on their own, and only exist to modify the creature object. Thus, a more efficient layout would have the AI and Pather functions as protected methods of the Creature class. These protected methods would typically be called by some sort of public virtual Update() method. Then, if later on you make a child class of Creature, you can override Update method, which will call the old behavior/ai methods and possibly add some new ones.

The downside of this is you have to put the variables from pather and AI inside your Creature class. It might make Creature a little bigger and harder to understand at a glance. However, this allows you to bypass the issue of giving ai and pather access to xCo/yCo, because theyre in the same class scope by default.

##### Share on other sites
Quote:
 Original post by WhatsUnderThere'Controller' type objects such as pather and ai typically shouldnt be separate objects following a strict OOP design. They serve no purpose on their own, and only exist to modify the creature object. Thus, a more efficient layout would have the AI and Pather functions as protected methods of the Creature class. These protected methods would typically be called by some sort of public virtual Update() method. Then, if later on you make a child class of Creature, you can override Update method, which will call the old behavior/ai methods and possibly add some new ones.

I don't think this is good advice. You are violating the single responsibility principle here. Putting the code for pathfinding and AI in separate objects is an excellent idea. Those objects can then later be reused in other places. You should use composition instead of inheritance as much as possible. see this article
Quote:
 However, this allows you to bypass the issue of giving ai and pather access to xCo/yCo, because theyre in the same class scope by default.

You are not improving the situation here. Throwing all functionality in one class still keeps the dependencies on xCo and yCo you are complaining about. A good design will result in very small interfaces of objects with single responsibilities, without access to another object's internals. Note that I'm not claiming this is easy to do :) The result however, will be worth it.

##### Share on other sites
Thanks for the response, WhatsUnderThere.
Quote:
 'Controller' type objects such as pather and ai typically shouldnt be separate objects following a strict OOP design. They serve no purpose on their own, and only exist to modify the creature object.

Yeah, I was finding this to be a common sentiment in the stuff I read today on all these other websites & postings, in a addition to some older GD.net posts.

Anyway, I tried to follow the advice given in this thread and I actually have it refactored and working properly. Nice and commented, nice and easy to understand (for me at least, but this is a solo project =)).

EDIT: Just saw yr post, Koen.

You said,
Quote:
 Putting the code for pathfinding and AI in separate objects is an excellent idea. Those objects can then later be reused in other places.

Well, I thought so for the path finding, since I do indeed use it in other places.

Thank you for the linked article you gave, Koen. I will have to read it more carefully after I finish this post. I am interested in these differing points of view since I am trying to decide how to best organize my code. Well, off to bed, but please if anyone has anything else to add I will appreciate any other contributions to this discussion.

##### Share on other sites
Quote:
 Original post by KoenI don't think this is good advice. You are violating the single responsibility principle here. Putting the code for pathfinding and AI in separate objects is an excellent idea. Those objects can then later be reused in other places. You should use composition instead of inheritance as much as possible. see this articleYou are not improving the situation here. Throwing all functionality in one class still keeps the dependencies on xCo and yCo you are complaining about. A good design will result in very small interfaces of objects with single responsibilities, without access to another object's internals. Note that I'm not claiming this is easy to do :) The result however, will be worth it.

I did not suggest merging AI and Creature to decrease dependencies. I was only pointing out that AI, in it's current form, did not have enough responsibility to qualify being its own class. Additionally, delegating movement controls to AI does not simply the interface. Instead, it forces you to create a wide variety of public get() and set() methods that would otherwise be managed through private functions.

While having short source files is a wonderful thing, it's not always the 'OOP' way of doing things. In general, classes should be responsible for modifying themselves. But in this example, the Creature is telling the AI what its state is through get() methods, and then the AI performs some calculations and updates the creature through set() methods. Essentially, the AI has taken away Creature's responsibility of maintaining its own state. If I havent convinced you yet, there really isnt much more to say about it--it's just one of the basic guidelines of OO design.

Also, Im not trying to say to say it's never appropriate to have an AI class. For example, it would make sense to have a separate class if your AI had a complex state consisting of multiple variables, and this state changed over time. Because the AI would have a changing state, you would want the AI class to manage itself rather than put everything in the Creature class. So for every time you called Creature.update(), in that method you would call AI.think() to update the state of the AI. Then, you would poll the AI class for instructions such as the direction to travel in, and update xCo and yCo appropriately. However, the AI should not be modifying the coordinates on its own, it should only be controlling its own state.

##### Share on other sites
Quote:
 Original post by signal_What is the best way for the AI class to obtain access to these variables?
Before people get into debating which of the proposed solutions are best, it is crucial to know what these variables are and why the AI class needs them at all. It may also help to know what exactly an AI class is responsible for doing.

##### Share on other sites
Quote:
Original post by dmatter
Quote:
 Original post by signal_What is the best way for the AI class to obtain access to these variables?
Before people get into debating which of the proposed solutions are best, it is crucial to know what these variables are and why the AI class needs them at all. It may also help to know what exactly an AI class is responsible for doing.

To respond to this, I think it best to first define what the AI class is doing. It has two member functions:

void inputs();     // function to process sensory inputs.void think();      // function for the thinking (control function) to                   // define what state the entity is in.  // the two functions above are composed of other smaller functions.

So basically, the inputs() function processes the current sensory inputs, while the think() function then calculates what state the entity should be in. It's an AI that is a finite state machine (FSM).

And, the second part of yr question
Quote:
 what these variables are and why the AI class needs them at all.

These variables are mostly booleans. So far they fall into two classes:

1. self characteristics - like a boolean isRunning, an int hitPoints.
2. characteristics of other perceived objects/environment - like if another creature is perceived, its sex: bool sex, so that a creature could tell if he encountered a female, for example.

So basically, I am implementing a FSM for the AI whose state changes are precipitated by the processing of its internal states by the inputs() method and their analysis in the think() method.

I just wanted to separate the ai code away from the other creature code since the code is getting quite long.

So it seems no matter how it is sliced, I am down to two options. I can use a hierarchical approach or the component based approach....

##### Share on other sites
Quote:
 While having short source files is a wonderful thing, it's not always the 'OOP' way of doing things. In general, classes should be responsible for modifying themselves. But in this example, the Creature is telling the AI what its state is through get() methods, and then the AI performs some calculations and updates the creature through set() methods. Essentially, the AI has taken away Creature's responsibility of maintaining its own state.

No, it doesn't. Having your mind tell your body what to do does not mean that the body cannot handle its bodily functions on itself. On the contrary, it makes the maintenance of valid state the ONLY responsibily of the body, as opposed to having 2 reponsibilities, maintaining the state AND making decisions. As a previous poster said, it is always good to respect SRP. If we put the AI decision and pathfinding in the same Creature class, then I suppose we should do the same for rendering, collision detection, input, sound,etc...basically turning the Creature class into a God class or blob, a class with many responsibilities and dependancies which code we can only reuse if we derive another class out of this blob, which will result in a deep, rigid, and usually pretty messed up hierarchy.

Ideally, The Creature class only responsibilty would be to execute self-contained operations, like move,eat,fight,whatever. The CreatureAI class only responsibility would be to make decisions and forward them to the creature, or gererally any class that conforms to a certain interface. It's a pretty good design, that is more flexible and maintainable than the rigid hierarchy design you're proposing. It's possible, for instance, that you can change the controller of the creature to change its behaviour at runtime, something very common in games.

##### Share on other sites
Quote:
 These variables are mostly booleans. So far they fall into two classes:1. self characteristics - like a boolean isRunning, an int hitPoints.
Break-things-up [smile]

State like hitPoints can be abstracted by a Health class that handles the manipulation of health: taking damage, healing and maintaining any invariants therein, after all, you can only be so healthy and you can only take so much damage before you die.

Position/orientation can be encapsulated by a Transform class that represents a spatial transformation. If the creature only has a position then a vector class may do instead.

Gender is probably quite simple, an enum would suffice.

State like isRunning can be grouped into a ActionState class that models the current action being undertaken, somewhat like nodes on a state machine.

Now that we have identified the various aspects that make up what a creature is, they can themselves be gathered into a CreatureAspect class.

Your creature class then fulfils the one remaining responsibility here, it becomes the AI controller, only with a CreatureAspect member.

It would also aggregate the different components which make the the functional aspects artificial intelligence; one of which is the Pather (I would find a better name, ones made from real words are always better, like PathFinder for example). More such aspects are likely to become available as you refactor, take for instance the creature's personality which could be abstracted into a Behaviour class. These objects would probably be stateless themselves, they would take a reference to (pieces of) the creature's aspect and either mutate it or return fresh instances, depending the immutability of the data.

Quote:
 2. characteristics of other perceived objects/environment - like if another creature is perceived, its sex: bool sex, so that a creature could tell if he encountered a female, for example.
That necessitates the need for some sort of world query object to inspect the environment for the creature since it is not the creature's job to know how the environment works; e.g.
creature_query.find_n_nearest_if(1, is_female);

Quote:
 So basically, I am implementing a FSM for the AI whose state changes are precipitated by the processing of its internal states by the inputs() method and their analysis in the think() method.
Isn't processing inputs simply a type of thinking? Or are the two functions called at different times? / the exact same time?

Quote:
 I just wanted to separate the ai code away from the other creature code since the code is getting quite long.
That's admirable but, IMHO, refactoring isn't something to do because your code is long, it is however, something to do when you arrive at a situation where you find yourself giving a class/function/variable more than one responsibility.

[Edited by - dmatter on January 14, 2009 11:32:32 PM]

##### Share on other sites
Quote:
 Break-things-upState like hitPoints can be abstracted......Now that we have identified the various aspects that make up what a creature is, they can themselves be gathered into a CreatureAspect class.

You're correct. Simplify the situation. I've begun this process now.
Quote:
 That necessitates the need for some sort of world query object to inspect the environment for the creature since it is not the creature's job to know how the environment works; e.g.creature_query.find_n_nearest_if(1, is_female);

I have a perception system in place already. Seems to be working adequately, although I have a few ideas of how to improve it.
Quote:
 Isn't processing inputs simply a type of thinking? Or are the two functions called at different times? / the exact same time?

The inputs() method is basically the processing of sensory input. So, let's say a creature 'perceives' two other creatures. It then categorizes the perceptions. For example, it may perceive creature1 as a member of its own species: a male might be a potential rival, while a female a potential mate. Creature2 could be predator or prey depending on what it is. The inputs() method sets the state variables.

The think() method then performs any required state transitions. And in the creature class, there is the action() method which carries out the actions associated with the current state in the state machine.
Quote:
 Quote: I just wanted to separate the ai code away from the other creature code since the code is getting quite long.That's admirable but, IMHO, refactoring isn't something to do because your code is long, it is however, something to do when you arrive at a situation where you find yourself giving a class/function/variable more than one responsibility.

To quote Wikipedia, "Code refactoring is the process of changing a computer program's internal structure without modifying its external behavior or existing functionality. This is usually done to improve code readability, simplify code structure, change code to adhere to a given programming paradigm, improve maintainability, or improve extensibility."

I was using refactoring in the sense that I wish to improve code readability and simplify code structure. All this mess is of my own making; instead of planning out and intelligently writing code I smashed the keyboard at breakneck speed, only looking up to compile every now and then. Now I have it working, but I need to "clean it up" in order to add more to it. I thought I had rid myself of this bad habit; need to plan things out better in the future.

In any case, thank you very much for responding; I will try to implement some of yr recommendations.