Language: C#
API: XNA

Hi, I have a game that i worked on a while back that I wish to re-write/fix up. The code for it is ugly and definitely has major design flaws. One of the major flaws I found is my use of declaring objects as static. Here's an example of what I'm talking about

Original design
GameplayScreen snippet

class GameplayScreen : GameScreen
{
#region Fields

ContentManager content;
SpriteFont gameFont;

Random random = new Random();

//Code I've added
public Block player;
BlockManager blockManager = new BlockManager();
public static ItemManager itemManager;
public static Texture2D redBlock;
public static Texture2D yellowBlock;
public static Texture2D greenBlock;
public static Texture2D plusOrb;
public static Texture2D minusOrb;
public static Texture2D growBiggerArrow;
public static Texture2D growSmallerArrow;
public static Texture2D invincibility;
public static SpriteFont itemFont;
public static SoundEffect death;
public static SoundEffect spawn;
public static SoundEffect plusSound;
public static SoundEffect minusSound;
public static SoundEffect shrinkSound;
public static SoundEffect growSound;
public static SoundEffect invincibilitySound;
public static bool isPlayerAlive = true;
public static int score = 0;

GrowBiggerArrow Snippet

public override void Update(GameTime gameTime)
{
/*if (this.HitBounds.Intersects(GameplayScreen.player.HitBounds))
{
//do specific item powerup. in this case it is +size
if (GameplayScreen.player.Size.X < GameplayScreen.player.MaxSize.X || GameplayScreen.player.Size.Y < GameplayScreen.player.MaxSize.Y)
GameplayScreen.player.Size += new Vector2(16,16);
GameplayScreen.itemManager.RemoveItem(this);
}*/

if (0 == triggered)
{
itemTimer += gameTime.ElapsedGameTime;
if (itemTimer > TimeSpan.FromSeconds(3))
{
GameplayScreen.itemManager.RemoveItem(this);
return;
}
if (this.HitBounds.Intersects(GameplayScreen.player.HitBounds))
{
//do specific item powerup. in this case it is grow bigger
//start timer
if (OptionsMenuScreen.sound)
GameplayScreen.growSound.Play();
if (GameplayScreen.player.Size.X <= GameplayScreen.player.MaxSize.X || GameplayScreen.player.Size.Y <= GameplayScreen.player.MaxSize.Y)
{
GameplayScreen.player.Size += new Vector2(16, 16);
text = "You grew!";
}
else
text = "You're too big to grow!";
triggered = 1;

//GameplayScreen.itemManager.RemoveItem(this);
}
}
//otherwise we picked it up at some point so check the timer and see if its time to take away the buff
else
{
displayTimer += gameTime.ElapsedGameTime;
if (displayTimer > TimeSpan.FromSeconds(3))
{
GameplayScreen.itemManager.RemoveItem(this);

displayTimer = TimeSpan.Zero;
}
}

}


As you can see, I have statics running all over the place. I want to keep my Update function to only take gametime as a parameter, but I need a way of getting rid of that call to access the player via the gamescreen class. I'm not sure how to implement this but these are the ideas I came up with.

Idea 1: On a collision, call the Grow method. I would have to take the collision detection out of the grow bigger class and move it to the gameplay screen (maybe? or is there a better way?)

Idea fix for grow bigger class

//All game objects (players, cars, items) are derived from GameObject
//gameObjectType would be an enum with all the different object types. (This could get extremely large and would
//have to be maintained manually (Haven't figured out how to do this part automatically)
//By taking these two parameters, I would be able to make all the public statics in gameplayscreen into private members
void Grow(GameObject obj, enum gameObjectType)
{
switch (gameObjectType)
{
case: Player:
//Handle player specific growth
break;
case: Vehicle
//Handle vehicle specific growth
break
}
}


Idea 2: Similar to idea 1 except I would have the grow bigger item fire off an event on the object that it collided with. The object that it collided with would then get the onCollide event and it would have the onCollide event handled by itself.
Example: GrowBiggerItem collides with player. GrowBiggerItem fires off the collide event to player. Player gets the event and has a method "onCollide" and handles the collide event.


These are the ideas that I came up with to get rid of the public statics and hopefully provide a better OOP design to my classes. Any feedback on what I can do to fix this would be appreciated.

Here is the full code for the growbigger item. Please point out any issues you see that I can improve on, I'm still trying to grasp OOP design in my code. Thank you.

If you want to go with the Grow() method solution, you don't have to 'enum' your gameObjectTypes manually. You can take the enum parameter out from your Grow() method and make it like this:

void Grow(GameObject obj)
{
if(obj is Player)
{
Player playerObj = obj as Player;
// Do player-related stuff
}
if(obj is Vehice)
{
Vehicle vehicleObj = obj as Vehicle;
// Do vehicle-related stuff
}
}


... Although, I don't think this is the best approach to your problem. Why not make the Grow() method in the GameObject class? Then you could override it for each of the specialized classes (Player, Vehicle etc.) to do their own Grow() stuff.

About removing the statics, I usually make this kind of a structure for my game projects:
- GameController class, which is a DrawableGameComponent (and is added to the Components list)
- ScreenController class, which handles all the drawing (the Draw() method is called from the GameController)
- Player class, which handles all the player-related stuff (Updated from GameController, drawn from ScreenController)

... and so on. I have this structure shown in this video:
[media][/media]
It's a bit long, but I think it could help you out while trying to figure out the best structure for your game.

Although, I don't think this is the best approach to your problem. Why not make the Grow() method in the GameObject class? Then you could override it for each of the specialized classes (Player, Vehicle etc.) to do their own Grow() stuff.

I thought about doing that but then my GameObject class would contain every single possible effect that could happen. Is this really a good way of doing it?

I haven't had time to watch your video yet, when I get home today I'm going to watch it. Sorry if you answered my question in that video.

Thank you.

I thought about doing that but then my GameObject class would contain every single possible effect that could happen. Is this really a good way of doing it?

Actually, as I said, you can overload the Grow() method in every inherited version, thus making separating the effects for each of the classes.
A bit like this:


public class GameObject
{
public virtual void Grow()
{
// Do nothing here.
}
}

public class Player : GameObject
{
// Place all the player's effect stuff here (For example:)
private float playerGrowRate = 1.0f;

public override void Grow()
{
// Do the player's grow stuff here (For example like this:)
playerSize += playerGrowRate;
}
}

public class Vehice : GameObject
{
// Again, place all the vehicle's stuff here
private float vehicleGrowRate = 5.4f;

public override void Grow()
{
// Again, do the vehicle's grow code here
}
}


Hopefully this clears my point.

I understood that, after looking at this for a while, I'm thinking that GameObject will have to contain all the functions (or event declarations if I go that route) and I'll have to ovveride those functions depending on my objects (which is what you suggested) I'm going to try to rework my classes like this and hopefully after I change everything, I'll re-post some of my classes for further critique. Thank you very much for your help!

After reworking the classes a bit, this is what I have. The functionality as it pertains to my game isn't implemented yet but the concept of how I'm trying to lay out my classes now are shown. Critiques and suggestions appreciated!

Note: Ignore the blockdodge namespace at the top, It's temporary until I rework everything else
Note 2: I accidently wrote GrowSmallerItem class in the class diagram. Grow bigger and grow smaller are pretty much the same.

Class diagram


Object class

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;
using Microsoft.Xna.Framework.Input;
using Microsoft.Xna.Framework.Audio;

using BlockDodgeWithGSM;

namespace BlockDodgeWithGSMv2
{
public abstract class GameObject
{
#region Properties
/// <summary>
/// By default the object is set to false meaning it does not handle touch input.
/// </summary>

bool isTouchable = false;
bool isHuman = false;
//default size is 32,32
Vector2 size = new Vector2(32, 32);
Vector2 position;

//Have default game sounds in here or in the specific classes?
SoundEffect defaultPlayerDeathSound = null;

protected bool IsTouchable
{
get { return isTouchable; }
protected set { isTouchable = value; }
}


/// <summary>
/// The width and height of the object
/// </summary>
protected Vector2 Size
{
get { return size; }
set { size = value; }
}

protected Vector2 Position
{
get { return position; }
protected set { position = value; }
}

//TODO Consider getting rid of isHuman

protected bool IsHuman
{
get { return isHuman; }
protected set { isHuman = value; }
}

public Rectangle HitBounds
{
get
{
Rectangle r = new Rectangle(
(int)(position.X),
(int)(position.Y),
(int)(size.X),
(int)(size.Y));

return r;
}
}

#region DefaultSounds

#region PlayerSounds

protected SoundEffect DefaultPlayerDeathSound
{
get { return defaultPlayerDeathSound; }
protected set { defaultPlayerDeathSound = value; }
}
#endregion

#endregion


#endregion

protected virtual void HandleInput(InputState input) { }

protected virtual void Update(GameTime gameTime) { }

protected virtual void Draw(SpriteBatch spriteBatch) { }

//TODO this is where effects will go. Objects can ovveride this to provide their own effect

public virtual void Grow() { }

public virtual void Shrink() { }

public static List<GameObject> Objects = new List<GameObject>();
}
}



Player class

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;
using Microsoft.Xna.Framework.Input;
using Microsoft.Xna.Framework.Input.Touch;
using Microsoft.Xna.Framework.Audio;
using BlockDodgeWithGSM;

namespace BlockDodgeWithGSMv2
{
public enum PlayerState
{
Alive,
Dead,
Invincible,
Smaller,
Bigger
}

public class Player : GameObject
{
Texture2D texture;
Texture2D overlay;
Vector2 velocity;
Vector2 minSize = new Vector2(48, 48);
Vector2 maxSize = new Vector2(96, 96);
bool inDragState = false;

PlayerState playerState = PlayerState.Alive;

#region Properties
public Vector2 Velocity
{
get { return velocity; }
protected set { velocity = value; }
}

public Vector2 MaxSize
{
get { return maxSize; }
protected set { maxSize = value; }
}

public Vector2 MinSize
{
get { return minSize; }
protected set { minSize = value; }
}

public PlayerState PlayerState
{
get { return playerState; }
set { playerState = value; }
}
#endregion

public Player(Texture2D texture, Vector2 position, Vector2 size, Vector2 velocity, bool isTouchable, bool isHuman)
{
this.texture = texture;
this.Position = position;
this.Size = size;
this.velocity = velocity;
this.IsTouchable = isTouchable;
this.IsHuman = isHuman;

GameObject.Objects.Add(this);
/* remove ishuman
if (isHuman == false)
BlockManager.obstacles.Add(this);
* */
}

public override void HandleInput(InputState input)
{

foreach (GestureSample gesture in input.Gestures)
{
foreach (TouchLocation touch in input.TouchState) //need this to know if it was a press or move
{
switch (gesture.GestureType)
{
case GestureType.FreeDrag:
if (touch.State == TouchLocationState.Moved)
{
if (this.inDragState) //is the block already being dragged?
{
//TODO get rid of this graphics somehow
int ScreenWidth = BlockDodgeWithGSM.BlockDodgeWithGSM.graphics.PreferredBackBufferWidth;
int ScreenHeight = BlockDodgeWithGSM.BlockDodgeWithGSM.graphics.PreferredBackBufferHeight;
int ScreenTop = 0 + GameplayScreen.adLocation.Height;

//We want the player to be centered on the touch and not the top left corner
this.Position = new Vector2(gesture.Position.X - this.Size.X / 2, gesture.Position.Y - this.Size.Y / 2);
//make sure player is within screen limits
//X
if (this.Position.X + this.Size.X > ScreenWidth)
this.Position = new Vector2(ScreenWidth - this.Size.X, this.Position.Y);
else if (this.Position.X < 0)
this.Position = new Vector2(0, this.Position.Y);
//Y
if (this.Position.Y + this.Size.Y > ScreenHeight)
this.Position = new Vector2(this.Position.X, ScreenHeight - this.Size.Y);
else if (this.Position.Y < ScreenTop)
this.Position = new Vector2(this.Position.X, ScreenTop);
break;
}
else
{
if (this.HitBounds.Contains(new Point((int)gesture.Position.X, (int)gesture.Position.Y))) //since block is not being dragged. check if touch is on block
{
//TODO get rid of this graphics somehow
int ScreenWidth = BlockDodgeWithGSM.BlockDodgeWithGSM.graphics.PreferredBackBufferWidth;
int ScreenHeight = BlockDodgeWithGSM.BlockDodgeWithGSM.graphics.PreferredBackBufferHeight;
int ScreenTop = 0 + GameplayScreen.adLocation.Height;
//We want the player to be centered on the touch and not the top left corner
this.Position = new Vector2(gesture.Position.X - this.Size.X / 2, gesture.Position.Y - this.Size.Y / 2);
//make sure player is within screen limits
//X
if (this.Position.X + this.Size.X > ScreenWidth)
this.Position = new Vector2(ScreenWidth - this.Size.X, this.Position.Y);
else if (this.Position.X < 0)
this.Position = new Vector2(0, this.Position.Y);
//Y
if (this.Position.Y + this.Size.Y > ScreenHeight)
this.Position = new Vector2(this.Position.X, ScreenHeight - this.Size.Y);
else if (this.Position.Y < ScreenTop)
this.Position = new Vector2(this.Position.X, ScreenTop);
this.inDragState = true;
break;
}
break;
}
}
break;
case GestureType.DragComplete: //drag finished so block no longer in drag state
this.inDragState = false;
break;
}
}
}
}

public override void Update(GameTime gameTime)
{

if (this.playerState != PlayerState.Invincible)
{
foreach (GameObject o in GameObject.Objects)
{
if (o is Enemy)
{
if (o.HitBounds.Intersects(this.HitBounds))
{
//Game over human was hit
if (GameplayScreen.isPlayerAlive == true)
{
//TODO: boolean in GameController? class to set sound
if (OptionsMenuScreen.sound)
GameplayScreen.death.Play();
GameplayScreen.isPlayerAlive = false;
this.PlayerState = PlayerState.Dead;
}
}
}
}
}
}

public override void Draw(SpriteBatch spriteBatch)
{
spriteBatch.Draw(this.texture, this.HitBounds, Color.White);
if (this.playerState == PlayerState.Invincible)
spriteBatch.Draw(overlay, this.HitBounds, new Color(255, 255, 255, 0.8f));
}

public override void Grow()
{
if (OptionsMenuScreen.sound)
GameplayScreen.growSound.Play();
if (this.Size.X <= this.MaxSize.X || this.Size.Y <= this.MaxSize.Y)
{
this.Size += new Vector2(16, 16);
//text = "You grew!";
}
else
{
//text = "You're too big to grow!";
}
}

public override void Shrink()
{
if (OptionsMenuScreen.sound)
GameplayScreen.shrinkSound.Play();
if (this.Size.X >= this.MinSize.X || this.Size.Y >= this.MinSize.Y)
{
this.Size -= new Vector2(16, 16);
//text = "You Shrunk!";
}
else
{
//text = "You're too tiny to shrink!";
}
}
}


}



Item class

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;

namespace BlockDodgeWithGSMv2
{
public abstract class Item : GameObject
{
//TODO instead of Texture2D create your own animated texture type
protected Texture2D texture;
protected TimeSpan timeOnScreen;
protected bool isTimed = false;
protected bool triggered;

protected int price;
protected string name;
protected double weight;


}
}



GrowBiggerItem class

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using Microsoft.Xna.Framework;
using Microsoft.Xna.Framework.Graphics;

using BlockDodgeWithGSM;

namespace BlockDodgeWithGSMv2
{
public class GrowBiggerItem : Item
{

TimeSpan itemTimer;
TimeSpan displayTimer;
string text;

public GrowBiggerItem(Texture2D texture, Vector2 position)
{
this.texture = texture;
this.Position = position;
this.Size = new Vector2(32, 32);
itemTimer = TimeSpan.Zero;
displayTimer = TimeSpan.Zero;
text = "";
}

public GrowBiggerItem(Texture2D texture, Vector2 position, Vector2 size)
{
this.texture = texture;
this.Position = position;
this.Size = size;
itemTimer = TimeSpan.Zero;
displayTimer = TimeSpan.Zero;
text = "";
}

public override void Update(GameTime gameTime)
{
foreach (GameObject obj in GameObject.Objects)
{
if (this.HitBounds.Intersects(obj.HitBounds))
{
obj.Grow();
triggered = true;
}
}

if (false == triggered)
{
itemTimer += gameTime.ElapsedGameTime;
if (itemTimer > TimeSpan.FromSeconds(3))
{
//remove item
//GameplayScreen.itemManager.RemoveItem(this);
return;
}

}
//otherwise we picked it up at some point so check the timer and see if its time to take away the buff
else
{
displayTimer += gameTime.ElapsedGameTime;
if (displayTimer > TimeSpan.FromSeconds(3))
{
//GameplayScreen.itemManager.RemoveItem(this);

displayTimer = TimeSpan.Zero;
}
}

}

public override void Draw(SpriteBatch spriteBatch)
{
int ScreenTop = 0 + GameplayScreen.adLocation.Height;
Vector2 Center = new Vector2((BlockDodgeWithGSM.BlockDodgeWithGSM.graphics.PreferredBackBufferWidth / 2) - (GameplayScreen.itemFont.MeasureString(text).X / 2), BlockDodgeWithGSM.graphics.PreferredBackBufferHeight / 2);

if (false == triggered)
spriteBatch.Draw(this.texture, new Rectangle((int)this.Position.X, (int)this.Position.Y, (int)this.Size.X, (int)this.Size.Y), Color.White);
else
spriteBatch.DrawString(GameplayScreen.itemFont, text, Center, Color.White);
}
}
}