Introduction
The following is a real C# Code Review. While its not an enterprise scale application – it will give you an understanding of the type of things that are assessed as part of a code review.
Background
The sample appplication is a simple ROGUELIKE game I built in a few hours. I have kept all the code in the one single file for the moment mainly to demonstrate the code review process and by being able to compare just 2 files I feel this is the easiest way to observe the review and the implementatin of the review. Future versions will include a properly designed project structure. The review was performed by my good friend Xhalent and was completed in a very short period of time.
Benefits of Code Reviews
Code reviews are a really important aspect of software development. Quite often reviews are performed by team leaders or peers and provide the developer with invaluable benefits including:
-Ensure adherence to an agreed framework
-Advice and direction in terms of better use of patterns and efficient code.
-Confirmation of correct use of business rules.
-Ensure consistent coding practices and naming conventions.
-Quicker detection of bugs.
-Forcing the developer to write readable code – knowing that all code will be reviewed.
While of course this sample is not based on an enterprise framework it should give you an idea of the type of things code reviews will pick up.
Code
The code is all available on codeplex http://rrrsroguelike.codeplex.com/ if you are interested you will also see there are the bugs,issues,tasks and releases.
I have also included the two files at the bottom of this post.
The Review
The following are the individual items that were raised during the review:
1. The field Tiles on Dungeon is public, should be private?
2. The field player on Dungeon is public, should be private?
3. The modifier for the various fields is not defined, so implicitly private
4. Why use List<> and not IList<> ? If possible, you should leave the definition of the concrete implementation as long as you can, and not constrain the concrete typeat the definition. This enables, down the track, the use of inversion of control and dependency injection to insetr various different types of player classes.Actually why are these lists and not arrays – do you remove walls, swords and monsters and so need a variable length list?
5. I think that rather scanning any child to see if they are still alive, the dungeon should have a integer of say, live monsters.When the monster gets created the number of live monsters gets incremented. The dungeon subscribes to the “ImDead” event of the monster, which gets fired in the die method.The event handler decrements the number of live monsters.Similarly, the dungeon should subscribe to the “ImDead” event of the player and set a boolean to indicate if the player is dead or not.
Then your IsGameSctive property is doing a very simple evaluation:
public bool IsGameActive
{
get { return (playerALive > 0 && liveMonsterCount > 0)); }
}
This is more efficient than querying every object when the property IsGameActive is accessed.
6. I think the working out of the where the player is should be done in the player class. Can’t a player move outside of a dungeon? What happens if I’m using a superhero player? Then my move is actually +=2 in the direction of my choice but that can’t be accomodated when the dungeon is in control. So the player class should have methods: GoSouth, GoNorth etc etc which do the appropriate location modifications. The player should expose a location property to give grid coordinates. The dungeons job is to take the user input and convert to the GoNSEW call, then ask the player where it is and work out if the move is allowed.
7. I’m no longer a fan of if(!IsInvalidValidMove()). I think it is much more readable to say if(IsInvalidValidMove()==false).
8. I think the logic of what to do when an object is on a tile should be placed in the player object. There should be some feedback to the dungeon to indicate if the item
was picked up or not, or if a different item was placed on the tile becuase the player couldn’t hold it.
9. I think the IsInvalidValidMove method name is very strange.
10. I think this method should use less than/greater than rather than equality becuase of the case of the super hero player that can move two or more tiles at once.
11. I note you have some evil comments.
12. I dont think the logic around battles should live in the dungeon. You should probably look at using a strategy pattern for the battle class. The strategy might
change depending on the weapons, number of participants etc.
13. MoveMonsterToPlayer. I think the monster should be in charge of where it moves, based on the passed in location of the player. Again, the calculated location
should be tested to see if it is invalid.
14. Mainly you;ve got all the logic in the Dungeon clas,s but that is really just the gametime container. It should eb unaware of how game elements do their thing,
Implementing the Review
I implemented most changes as I reviewed them in detail and agreed they were worthwhile doing. I did not implement no 8 because I basically ran out of time but will be looking to implement some changes in the next version based on this recommendation.
Viewing the changes in code
Its worth having a look at the first version of the code RRRSRougeLike0.1.cs before you look at the new version. You will notice the new version of the game contains refactor comments like:
//Refactor 14: Add the new Game Manager Class
This indicates we are making a change the refers to the Review Item that Xhalent raised in his review. You should see one for each of Xhalent’s original review items with the exception of no 8.
In Summary
This article demonstrated the implementation of a code review. Big thanks to Xhalent for the excellent review and although the implementation of the review is a great start to the development process of this game its not the end. The next article will focus on the next iteration of the game and hopefully another review.
Code Sample – RSSSRougueLike0.1.cs
using System;
using System.Collections.Generic;
using System.Drawing;
using System.Linq;
namespace ReallyReallyReallySimpleRogueLike
{
class ReallyReallyReallySimpleRogueLike
{
static void Main(string[] args)
{
Dungeon dungeon = new Dungeon(Constants.DungeonWidth, Constants.DungeonHeight);
string displayText = Constants.IntroductionText;while (dungeon.IsGameActive)
{
dungeon.DrawToConsole();
Console.WriteLine(displayText);
Console.Write(Constants.CursorImage);
displayText = dungeon.ExecuteCommand(Console.ReadKey());
}Console.WriteLine(ConcludeGame(dungeon));
Console.ReadLine();
}private static string ConcludeGame(Dungeon dungeon)
{
return (dungeon.player.Hits > 0) ? Constants.PlayerWinsText : Constants.MonsterWinsText;
}
}class Dungeon
{
Random r;
public Player player;
List monsters;
List swords;
List walls;public Tile[,] Tiles;
private int xMax;
private int yMax;
public enum Direction
{
North,
South,
East,
West
}public bool IsGameActive
{
get
{
return (player.Hits > 0 && monsters.Any(m => m.Hits > 0));
}
}public Dungeon(int xMax, int yMax)
{
monsters = new List();
walls = new List();
swords = new List();this.xMax = xMax;
this.yMax = yMax;
Tiles = new Tile[xMax, yMax];
BuildRandomDungeon();
SetDungeonTiles();
}public string ExecuteCommand(ConsoleKeyInfo command)
{
string commandResult = ProcessCommand(command);
ProcessMonsters();
SetDungeonTiles();return commandResult;
}private void ProcessMonsters()
{
if (monsters != null && monsters.Count > 0)
{
monsters.Where(m => m.Hits >= 0).ToList().ForEach(m =>
{
MoveMonsterToPlayer(m);
});
}
}private void BuildRandomDungeon()
{
r = new Random();
SetAllDungeonSquaresToTiles();for (int i = 0; i < xMax; i++)
{
Wall top = new Wall(i, 0);
walls.Add(top);
Wall bottom = new Wall(i, yMax – 1);
walls.Add(bottom);
}for (int i = 0; i < yMax; i++)
{
Wall left = new Wall(0, i);
walls.Add(left);
Wall right = new Wall(xMax – 1, i);
walls.Add(right);
}for (int i = 0; i < Constants.NumberOfSwords; i++)
{
Sword s = new Sword(GetValidRandomPoint());
swords.Add(s);
}for (int i = 0; i 0 && monster.X player.X) ? -1 : 1;
if ((monster.Y > 0 && monster.Y player.Y) ? -1 : 1;
if (!IsInvalidValidMove(move.X, move.Y))
{
monster.X = move.X;
monster.Y = move.Y;
}if (monster.X == player.X && monster.Y == player.Y)
ResolveCombat(monster);
}private void ResolveCombat(Monster monster)
{
if (player.Inventory.Any())
monster.Die();
else
player.Die();
}public string ProcessCommand(ConsoleKeyInfo command)
{string output = string.Empty;
switch (command.Key)
{
case ConsoleKey.UpArrow:
case ConsoleKey.DownArrow:
case ConsoleKey.RightArrow:
case ConsoleKey.LeftArrow:
output = GetNewLocation(command, new Point(player.X, player.Y));
break;
case ConsoleKey.F1:
output = Constants.NoHelpText;
break;
}return output;
}private string GetNewLocation(ConsoleKeyInfo command, Point move)
{
switch (command.Key)
{
case ConsoleKey.UpArrow:
move.Y -= 1;
break;
case ConsoleKey.DownArrow:
move.Y += 1;
break;
case ConsoleKey.RightArrow:
move.X += 1;
break;
case ConsoleKey.LeftArrow:
move.X -= 1;
break;
}if (!IsInvalidValidMove(move.X, move.Y))
{
player.X = move.X;
player.Y = move.Y;
if (Tiles[move.X, move.Y] is Sword && player.Inventory.Count == 0)
{
Sword sword = (Sword)Tiles[move.X, move.Y];
player.Inventory.Add(sword);
swords.Remove(sword);
}
return Constants.OKCommandText;
}
else
return Constants.InvalidMoveText;
}public bool IsInvalidValidMove(int x, int y)
{
return (x == 0 || x == Constants.DungeonWidth – 1 || y == Constants.DungeonHeight – 1 || y == 0);
}public void SetDungeonTiles()
{
//Draw the empty dungeon
SetAllDungeonSquaresToTiles();SetAllDungeonObjectsToTiles();
}private void SetAllDungeonObjectsToTiles()
{
//Now draw each of the parts of the dungeon
walls.ForEach(w => Tiles[w.X, w.Y] = w);
swords.ForEach(s => Tiles[s.X, s.Y] = s);
monsters.ForEach(m => Tiles[m.X, m.Y] = m);
Tiles[player.X, player.Y] = player;
}private void SetAllDungeonSquaresToTiles()
{
for (int i = 0; i < yMax; i++)
{
for (int j = 0; j < xMax; j++)
{
Tiles[j, i] = new Tile(i, j);
}
}
}public void DrawToConsole()
{
Console.Clear();
for (int i = 0; i < yMax; i++)
{
for (int j = 0; j < xMax; j++)
{
Console.ForegroundColor = Tiles[j, i].Color;
Console.Write(Tiles[j, i].ImageCharacter);
}
Console.WriteLine();
}
}
}public class Tile
{
public string name { get; set; }
public string ImageCharacter { get; set; }
public ConsoleColor Color { get; set; }
public int X { get; set; }
public int Y { get; set; }public Tile() { }
public Tile(int x, int y)
: base()
{
this.X = x;
this.Y = y;
ImageCharacter = Constants.TileImage;
Color = Constants.TileColor;
}
}public class Wall : Tile
{
public Wall(int x, int y)
: base(x, y)
{
ImageCharacter = Constants.WallImage;
this.Color = Constants.WallColor;
}
}public class Sword : Tile
{
public Sword(Point p)
{
ImageCharacter = Constants.SwordImage;
this.Color = Constants.SwordColor;
X = p.X;
Y = p.Y;
}
}public class Creature : Tile
{
public int Hits { get; set; }public void Die()
{
Hits = 0;
}
}public class Player : Creature
{
public Player(Point p)
{
ImageCharacter = Constants.PlayerImage;
Color = Constants.PlayerColor;
Inventory = new List();
X = p.X;
Y = p.Y;
Hits = Constants.StartingHitPoints;
}public List Inventory { get; set; }
}public class Monster : Creature
{
public Monster(Point p)
{
ImageCharacter = Constants.MonsterImage;
Color = Constants.MonsterColor;
X = p.X;
Y = p.Y;
Hits = Constants.StartingHitPoints;
}
}public static class Constants
{
public readonly static int DungeonHeight = 20;
public readonly static int DungeonWidth = 20;
public readonly static int NumberOfSwords = 5;
public readonly static int MonsterDamage = 2;
public readonly static int NumberOfMonsters = 1;
public readonly static int StartingHitPoints = 10;public readonly static string TileImage = “.”;
public readonly static string WallImage = “#”;
public readonly static string PlayerImage = “@”;
public readonly static string SwordImage = “s”;
public readonly static string StepsImage = “S”;
public readonly static string MonsterImage = “M”;
public readonly static string CursorImage = “>”;public readonly static ConsoleColor MonsterColor = ConsoleColor.Blue;
public readonly static ConsoleColor PlayerColor = ConsoleColor.Gray;
public readonly static ConsoleColor WallColor = ConsoleColor.DarkCyan;
public readonly static ConsoleColor SwordColor = ConsoleColor.Yellow;
public readonly static ConsoleColor TileColor = ConsoleColor.White;public readonly static string InvalidCommandText = “That is not a valid command”;
public readonly static string OKCommandText = “OK”;
public readonly static string InvalidMoveText = “That is not a valid move”;
public readonly static string IntroductionText = “Welcome to the dungeon – grab a sword kill the monster(s) win the game”;
public readonly static string PlayerWinsText = “Player kills monster and wins”;
public readonly static string MonsterWinsText = “Monster kills player and wins”;
public readonly static string NoHelpText = “No help text”;
}
}
Code Sample – RSSSRougueLike0.2.cs
using System;
using System.Collections.Generic;
using System.Drawing;
using System.Linq;
using System.Text;
namespace ReallyReallyReallySimpleRogueLike
{
class ReallyReallyReallySimpleRogueLike
{
private static void Main(string[] args)
{
DungeonGameManager dungeonGameManager = new DungeonGameManager();
string displayText = Constants.IntroductionText;
while (dungeonGameManager.IsGameActive)
{
dungeonGameManager.DrawDungeon();
DrawDisplayText(displayText);
displayText = dungeonGameManager.ExecuteCommand(Console.ReadKey());
}
dungeonGameManager.DrawDungeon();
ConcludeGame(dungeonGameManager.PlayerHasWon.Value);
}private static void DrawDisplayText(string displayText)
{
Console.WriteLine(displayText);
Console.Write(Constants.CursorImage);
}private static void ConcludeGame(bool hasPlayerWon)
{
string gameConclusionText = (hasPlayerWon) ? Constants.PlayerWinsText : Constants.MonsterWinsText;
Console.WriteLine(gameConclusionText);
Console.ReadLine();
}
}//Refactor 14: Add the new Game Manager Class
class DungeonGameManager
{
//Refactor 3:Make the members explicitly private
private Dungeon dungeon;
private Player player; //Refactor 2:Player is now private
private IList monsters; //Refactor 4:IList replaces List
private IList swords; //Refactor 4:IList replaces List
private bool playerAlive;
private static Random random;public DungeonGameManager ()
{
random = new Random();
playerAlive = true;
dungeon = new Dungeon(Constants.DungeonWidth, Constants.DungeonHeight);
player = new Player(GetValidRandomPoint());
player.CreatureDeadEvent += new Creature.CreatureDeadHandler(player_CreatureDeadEvent);
monsters = new List();
swords = new List();for (int i = 0; i < Constants.NumberOfMonsters; i++)
{
Monster monster = new Monster(GetValidRandomPoint());
monster.CreatureDeadEvent += new Creature.CreatureDeadHandler(monster_CreatureDeadEvent);
monsters.Add(monster);
}for (int i = 0; i 0 && monsters.Count() > 0); }
}public string ExecuteCommand(ConsoleKeyInfo command)
{
StringBuilder commandResult = new StringBuilder(ProcessCommand(command));
commandResult.Append(ProcessMonsters());
SetAllDungeonObjectsToTiles();
return commandResult.ToString();
}private string ProcessMonsters()
{
if (monsters.Count()>0)
{
MoveMonsters();
return FightMonstersOnPlayersTile();
}
return string.Empty;
}private string FightMonstersOnPlayersTile()
{
var monstersInBattle = monsters.Where(m => m.X == player.X && m.Y == player.Y).ToList();
string outputText = string.Empty;if (monstersInBattle.Count() > 0)
{
SimpleBattle battle = new SimpleBattle(player, monstersInBattle);
outputText = battle.Fight();
}return outputText;
}private void MoveMonsters()
{
monsters.ToList().ForEach(m =>
{
Point currentPlayerLocation = new Point(player.X, player.Y);
m.MoveToPlayer(currentPlayerLocation);
});
}private void SetAllDungeonObjectsToTiles()
{
dungeon.Clear();
swords.ToList().ForEach(s=>dungeon.AddSword(s));
monsters.ToList().ForEach(m => dungeon.AddMonster(m));
dungeon.walls.ToList().ForEach(w => dungeon.AddWalls(w));
dungeon.AddPlayer(player);
}public string ProcessCommand(ConsoleKeyInfo command)
{
string output = string.Empty;
switch (command.Key)
{
case ConsoleKey.UpArrow:
case ConsoleKey.DownArrow:
case ConsoleKey.RightArrow:
case ConsoleKey.LeftArrow:
output = GetNewLocation(command, player);
break;
case ConsoleKey.F1:
output = Constants.NoHelpText;
break;
}
return output;
}private string GetNewLocation(ConsoleKeyInfo command, Player player)
{
Constants.Direction direction = GetDirectionFromConsoleKey(command);
Point newPoint = player.Move(direction);
if (dungeon.IsOKToMove(newPoint))
{
player.X = newPoint.X;
player.Y = newPoint.Y;
if (dungeon.GetTile(newPoint) is Sword && player.Inventory.Count == 0)
{
Sword sword = (Sword)dungeon.GetTile(newPoint);
player.Inventory.Add(sword);
swords.Remove(sword);
}
return Constants.OKCommandText;
}
else
return Constants.InvalidMoveText;
}public Constants.Direction GetDirectionFromConsoleKey(ConsoleKeyInfo command)
{
Constants.Direction direction = Constants.Direction.Unknown;
switch(command.Key)
{
case ConsoleKey.UpArrow:
direction = Constants.Direction.N;
break;
case ConsoleKey.DownArrow:
direction = Constants.Direction.S;
break;
case ConsoleKey.RightArrow:
direction = Constants.Direction.E;
break;
case ConsoleKey.LeftArrow:
direction = Constants.Direction.W;
break;
}
return direction;
}public void DrawToConsole()
{
Console.Clear();
for (int i = 0; i < Constants.DungeonHeight; i++)
{
for (int j = 0; j < Constants.DungeonWidth; j++)
{
Console.ForegroundColor = dungeon.Tiles[j, i].Color;
Console.Write(dungeon.Tiles[j, i].ImageCharacter);
}
Console.WriteLine();
}
}
}class Dungeon
{
public IList walls; //Refactor 3: Replace the List with IList
public Tile[,] Tiles; //Refactor 2: make Tiles privatepublic enum Direction
{
North,
South,
East,
West
}public Dungeon(int xMax, int yMax)
{
walls = new List();
Tiles = new Tile[xMax, yMax];
BuildRandomDungeon();
}private void SetAllDungeonSquaresToTiles()
{
for (int i = 0; i < Constants.DungeonHeight; i++)
{
for (int j = 0; j < Constants.DungeonWidth; j++)
{
Tiles[j, i] = new Tile(i, j);
}
}
}private void BuildRandomDungeon()
{
SetAllDungeonSquaresToTiles();for (int i = 0; i < Constants.DungeonWidth; i++)
{
Wall top = new Wall(i, 0);
walls.Add(top);
Wall bottom = new Wall(i, Constants.DungeonHeight – 1);
walls.Add(bottom);
}for (int i = 0; i 0 && p.X < Constants.DungeonWidth – 1) && (p.Y 0));
}internal void AddSword(Sword s)
{
Tiles[s.X, s.Y] = s;
}internal void AddMonster(Monster m)
{
Tiles[m.X, m.Y] = m;
}internal void AddWalls(Wall w)
{
Tiles[w.X, w.Y] = w;
}internal void AddPlayer(Player player)
{
Tiles[player.X, player.Y] = player;
}internal Tile GetTile(Point point)
{
return Tiles[point.X, point.Y];
}internal void Clear()
{
SetAllDungeonSquaresToTiles();
}
}public class Tile
{
public string name { get; set; }
public string ImageCharacter { get; set; }
public ConsoleColor Color { get; set; }
public int X { get; set; }
public int Y { get; set; }public Tile() { }
public Tile(int x, int y)
: base()
{
this.X = x;
this.Y = y;
ImageCharacter = Constants.TileImage;
Color = Constants.TileColor;
}
}public class Wall : Tile
{
public Wall(int x, int y)
: base(x, y)
{
ImageCharacter = Constants.WallImage;
this.Color = Constants.WallColor;
}
}public class Sword : Tile
{
public Sword(Point p)
{
ImageCharacter = Constants.SwordImage;
this.Color = Constants.SwordColor;
X = p.X;
Y = p.Y;
}
}public class Creature : Tile
{
private int hits;public delegate void CreatureDeadHandler(Object sender);
public event CreatureDeadHandler CreatureDeadEvent;public int Hits
{
get
{
return hits;
}
set
{
hits = value;
if (hits = 0);}
}public void Attack(Creature creature)
{
creature.Hits= 0;
}
}public class Player : Creature
{
public Player(Point p)
{
ImageCharacter = Constants.PlayerImage;
Color = Constants.PlayerColor;
Inventory = new List();
X = p.X;
Y = p.Y;
Hits = Constants.StartingHitPoints;
}//Refactor 6: Encapsulate the move action within the player class
public Point Move(Constants.Direction direction)
{
Point newPoint = new Point(X, Y);
switch (direction)
{
case Constants.Direction.N:
newPoint.Y -= 1;
break;
case Constants.Direction.S:
newPoint.Y += 1;
break;
case Constants.Direction.E:
newPoint.X += 1;
break;
case Constants.Direction.W:
newPoint.X -= 1;
break;
}
return newPoint;
}public IList Inventory { get; set; }
}public class Monster : Creature
{
public Monster(Point p)
{
ImageCharacter = Constants.MonsterImage;
Color = Constants.MonsterColor;
X = p.X;
Y = p.Y;
Hits = Constants.StartingHitPoints;
}//Refactor 13:MoveMonsterToPlayer. The monster should be in charge of where it moves.
public void MoveToPlayer(Point playerLocation)
{
Point move = new Point(X, Y);
if (X > 0 && X playerLocation.X) ? -1 : 1;if ((Y > 0 && Y playerLocation.Y) ? -1 : 1;
}
}//Refactor 12:Introduce a battle class to encapsulate the way a battle occurs – this also includes IBattle
public class SimpleBattle : IBattle
{
private Player player;
private IList monsters;public SimpleBattle(Player player,IList monsters)
{
this.player = player;
this.monsters = monsters;
}public string Fight()
{
Monster monster = monsters.OrderBy(m => m.Hits).FirstOrDefault();//get the monster with the most hits
if (player.Inventory.Any())
{
player.Attack(monster);
return Constants.PlayerKillsMonsterText;
}
else
{
monster.Attack(player);
return Constants.MonsterKillsPlayerText;
}
}
}public interface IBattle
{
string Fight();
}public static class Constants
{
public readonly static int DungeonHeight = 20;
public readonly static int DungeonWidth = 20;
public readonly static int NumberOfSwords = 10;
public readonly static int MonsterDamage = 2;
public readonly static int NumberOfMonsters = 3;
public readonly static int StartingHitPoints = 10;public readonly static string TileImage = “.”;
public readonly static string WallImage = “#”;
public readonly static string PlayerImage = “@”;
public readonly static string SwordImage = “s”;
public readonly static string StepsImage = “S”;
public readonly static string MonsterImage = “M”;
public readonly static string CursorImage = “>”;public readonly static ConsoleColor MonsterColor = ConsoleColor.Blue;
public readonly static ConsoleColor PlayerColor = ConsoleColor.Gray;
public readonly static ConsoleColor WallColor = ConsoleColor.DarkCyan;
public readonly static ConsoleColor SwordColor = ConsoleColor.Yellow;
public readonly static ConsoleColor TileColor = ConsoleColor.White;public readonly static string InvalidCommandText = “That is not a valid command”;
public readonly static string OKCommandText = “OK”;
public readonly static string InvalidMoveText = “That is not a valid move”;
public readonly static string IntroductionText = “Welcome to the dungeon – grab a sword kill the monster(s) win the game”;
public readonly static string PlayerWinsText = “Player kills monster and wins”;
public readonly static string MonsterWinsText = “Monster wins”;
public readonly static string NoHelpText = “No help text”;
public readonly static string PlayerKillsMonsterText = “Player kills a monster”;
public readonly static string MonsterKillsPlayerText = “Monster kills a player”;public enum Direction { N, S, E, W, Unknown };
}
}
Doms Code 