6 replies to this topic

[SolarChronus]

I have been programming for awhile now, but the one thing that seems to always bite me is the design of my code. In college we were taught how to write code, but not how to architect it. I feel as though this .cs file, along with multiple others, could do with some code reviews. I feel like I am at a point where learning concepts and applying them isn't enough anymore and I need my code put through the ringer - something it seems professors don't really have the time to do. I've already found this place really helpful with expanding my knowledge and hope that a few people would take some time to look at it and let me know if there are better/more efficient ways to go about what I am doing!

Below is my CombatUI.cs file. This is used to manage the user interface when the player is inside a combat arena with another ship. The UI will display the players ship, target ship, damage indicators for each subsystem for each ship, and the hull/shield condition for each ship. I use events to update the indicators whenever the subsystems take damage. Everything works perfectly, however, the design feels flawed and cumbersome if I want to add new subsystems. Also checking the owner of the ship (Player vs AI) seems out of place, like there is a better way to implement it.

public class CombatUI
{
	public Ship playerShip = null;
	public Ship targetShip = null;
	public GUIText player_computerCoreDamageIndicator;
	public GUIText player_engineCoreDamageIndicator;
	public GUIText player_weaponSystemDamageIndicator;
	public GUIText player_sensorGridDamageIndicator;
	public GUIText player_shieldEmittersDamageIndicator;
	public GUIText player_shieldStrengthDamageIndicator;
	public GUIText player_hullStrengthDamageIndicator;
	public GUIText target_computerCoreDamageIndicator;
	public GUIText target_engineCoreDamageIndicator;
	public GUIText target_weaponSystemDamageIndicator;
	public GUIText target_targetingComputerDamageIndicator;
	public GUIText target_shieldEmittersDamageIndicator;
	public GUIText target_shieldStrengthDamageIndicator;
	public GUIText target_hullStrengthDamageIndicator;

	public CombatUI(Ship playerShip, Ship targetShip)
	{
	 	 this.playerShip = playerShip;
	 	 this.targetShip = targetShip;
		ObserveShipSystems(playerShip);
		//ObserveShipSystems(targetShip);
	}

	private void ObserveShipSystems(Ship ship)
	{
		ship.OnShieldChangeCondition += new Ship.ConditionHandler(UpdateShieldConditionIndicator);
		ship.OnHullChangeCondition += new Ship.ConditionHandler(UpdateHullConditionIndicator);
		ObserveShipSubsystems(ship.Subsystems);
		UpdateSubsystemIndicators(ship);
		UpdateShieldConditionIndicator(ship);
		UpdateHullConditionIndicator(ship);
	}

	private void ObserveShipSubsystems(Dictionary<SubsystemType, Subsystem> subsystems)
	{
		foreach (KeyValuePair<SubsystemType, Subsystem> subsystem in subsystems)
		{
			subsystem.Value.subsystemDamaged += new Subsystem.DamageHandler(UpdateSubsystemIndicators);
		}
	}

	private void UpdateShieldConditionIndicator(Ship ship)
	{
		float shieldStrength = ((ShieldEmitter)ship.Subsystems[SubsystemType.ShieldEmitter]).ShieldStrength;
		player_shieldStrengthDamageIndicator.text = "SS: " + shieldStrength;
	}

	private void UpdateHullConditionIndicator(Ship ship)
	{
		float hullStrength = ((HullPlating)ship.Subsystems[SubsystemType.HullPlating]).HullStrength;
		player_hullStrengthDamageIndicator.text = "HS: " + hullStrength;
	}

	private void UpdateSubsystemIndicators(Ship ship)
	{
		if (ship.Controller == Controller.Player)
		{
			// Prototype display for showing damage
			// TODO: Change from GuiText to a GuiTexture
			player_computerCoreDamageIndicator.text = "CC: " + ship.Subsystems[SubsystemType.ComputerCore].DamagePercent().ToString() + "%";	  
			player_engineCoreDamageIndicator.text = "EC: " + ship.Subsystems[SubsystemType.EngineCore].DamagePercent().ToString() + "%";
			player_weaponSystemDamageIndicator.text = "WS: " + ship.Subsystems[SubsystemType.WeaponSystem].DamagePercent().ToString() + "%";
			player_shieldEmittersDamageIndicator.text = "SE: " + ship.Subsystems[SubsystemType.ShieldEmitter].DamagePercent().ToString() + "%";
			player_sensorGridDamageIndicator.text = "SG: " + ship.Subsystems[SubsystemType.SensorGrid].DamagePercent().ToString() + "%";
		}
		else
		{
			target_computerCoreDamageIndicator.text = "CC: " + ship.Subsystems[SubsystemType.ComputerCore].DamagePercent().ToString() + "%";
			target_engineCoreDamageIndicator.text = "EC: " + ship.Subsystems[SubsystemType.EngineCore].DamagePercent().ToString() + "%";
			target_weaponSystemDamageIndicator.text = "WS: " + ship.Subsystems[SubsystemType.WeaponSystem].DamagePercent().ToString() + "%";
			target_shieldEmittersDamageIndicator.text = "SE: " + ship.Subsystems[SubsystemType.ShieldEmitter].DamagePercent().ToString() + "%";
			player_sensorGridDamageIndicator.text = "SG: " + ship.Subsystems[SubsystemType.SensorGrid].DamagePercent().ToString() + "%";
		}
	}
}

[slayemin]

I find the Model-View-Controller framework to be a helpful framework to keep in mind and to design to.

Your game is the "model". This would contain all of your data structures, game objects, game states, the state of your game objects, process the next frame, turn, etc. It's the guts of your application. It doesn't care at all about rendering stuff to the screen or collecting input to change the model*. It's very abstract.

Your "view" has the sole focus of representing the state of the "model" to the user. It doesn't do game logic! I like to think of it as the artistic manifestation of your model. Since it's "artistic", that means that you could interchange one view with another to get a totally different view of the same data. Real world example: I've got a database driven application. The model is the data and how it all interacts. The view can be rendered as either a windows application or a webpage. I wouldn't have to change any code in the model because its completely unconcerned about rendering.

The "controller" is how the user interacts with your model. Sometimes, it's intertwined closely with the view (use the mouse to click on a button!), but its not necessary (press "F1" key to fire torpedos!). A controller will send a command (better to think of it as a request) to the model and the model will decide if it wants to honor it and change its state accordingly ("okay! I've created a torpedo!" or "nope, you have no torpedos left or its not ready to fire again!"). Again, we are NOT putting game logic into the controller. We're using it as a way to interact with the model which contains the logic to do the requested action.

So, just by looking at your code, I can see that you have a single class which is concerned with rendering. This would fit within a "view" portion of the MVC framework. It's not enough to tell you anything about whether or not your classes are well architected because we can't see how they're interacting with each other.
One slightly worrisome thing I'm seeing though is your liberal use of public access for internal class variables. Do you do this with your other classes? It may not be a big deal for small projects, but you will start to run into problems with larger projects. I like to assume that I'm writing my classes to be used by the biggest idiot in the world (me). If there is a way someone (me) can completely screw it up, then it's only a matter of time before it happens (sleep deprivation, forgetfulness, laziness, sloppiness, etc). When that screw up happens, I have to waste time debugging to isolate the source of the problem ("OH! I'm getting errors because this completely unrelated class thinks it should be allowed to come into my class and muck around with its variables?! He wants to divide by zero?!). So, when you're designing your class, think of yourself as a miser who will only give access to your class if and only if there is a good reason to, and that access will be on your terms rather than the users terms. If something is public, it's free game for a legion of monkeys to use. Would you like a legion of monkeys twiddling with your class methods or class variables?

[SolarChronus]

After reading up on MVC as you suggested I look into, I have totally re-designed the combat ui code. I grouped all the major UI elements into frames so they act as my views, created a an overarching HUD file that acts as the controller and let the combat arena scene act as the model. Combined with events I can easily change the hud elements without impacting the way the user interacts with the scene or re-tooling the model. Thanks for the suggestion!

As for those public data members, whoops, that was more a typo then wanting them exposed! I do have another question though on the best way to design out my subsystem installation code. Right now my design would work like this(pseudo-code):

public void InstallSubsystem(Subsystem subsystem)
{
	 switch(subsystem.Type)
	 {
		  case SubsystemType.EngineCore:
			   InstallEngineCore((EngineCore)subsystem);
		  break;
	 }
}


private void InstallEngineCore(EngineCore engineCore)
{
	 //Stuff associated with installing/replacing a subsystem here.
}

My one gripe with this is the use of the switch statement. I wanted to encapsulate the installation functionality so that the only thing that needs to be called is InstallSubsystem(...) when a subsystem is to be installed/replaced. I could get rid of that function and expose the individual methods for installing each subsystem but felt like that is just too much exposure. I am butting up against the limits of my knowledge on this one, and wonder if the switch is really the best way to go or would there be an alternative?

Edited by SolarChronus, 31 October 2012 - 06:48 PM.

[Alpha_ProgDes]

I know there's a polymorphic solution to your switch statement issue. Unfortunately, I can't think of it right now.

[SolarChronus]

I thought there was too but extracting them into classes seemed awkward.

[Washu]

I know there's a polymorphic solution to your switch statement issue. Unfortunately, I can't think of it right now.

State pattern, or strategy. Take your pick.

[SolarChronus]

Again, I made this mole hill of a problem into a mountain. Since I am keeping a dictionary of subsystems and using their type as unique keys, I don't need a switch switch statement or polymorphic solution. This function should cover any number of subsystems I intend on implementing on my ship, as the ship will never have duplicate subsystems.

public void InstallSubsystem(Subsystem subsystem)
	{
		if (subsystems.ContainsKey(subsystem.SubsystemType))
		{
			RemoveSubsystem(subsystems[subsystem.SubsystemType]);
			subsystems[subsystem.SubsystemType] = subsystem;
		}
		else
			subsystems.Add(subsystem.SubsystemType, subsystem);
	}