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 application is a simple ROGUE LIKE game I built in a few hours. For the original article that demonstrates how I built it click here. I have kept all the code in the one single file for the moment mainly to demonstrate the code review process. By being able to compare just 2 files I feel this is the easiest way to observe the review and the implementation 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.
Code
The code is available on codeplex at http://rrrsroguelike.codeplex.com/. If you are interested you will also see the development in progress including: bugs, issues, tasks and releases.
The original program RRRSRogueLike0.1.cs I built is included as well as the version that includes the implementation of the review RRRSRogueLike0.2.cs.
The Review
The following are the individual items that were raised during the review:
- The field Tiles on Dungeon is public, should be private?
- The field Player on Dungeon is public, should be private?
- The modifier for the various fields is not defined, so implicitly private.
- 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 type at the definition. This enables, down the track, the use of inversion of control and dependency injection to insert 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?
- I think that rather than scanning any child to see if they are still alive, the dungeon should have an 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 IsGameActive 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.
-
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 accommodated 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.
- I'm no longer a fan of if(!IsInvalidValidMove()). I think it is much more readable to say if(IsInvalidValidMove()==false).
- 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 because the player couldn't hold it.
- I think the IsInvalidValidMove method name is very strange.
- I think this method should use less than/greater than rather than equality because of the case of the super hero player that can move two or more tiles at once.
- I note you have some evil comments.
- I don't 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.
- 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.
- Mainly you've got all the logic in the Dungeon class but that is really just the gametime container. It should be 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.
However, I did not implement number 8 because I basically ran out of time and given the limitation of item functionality. I will be looking to implement some changes in the next version based on this recommendation.
Viewing the Changes in Code
It's 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 that refers to the Review Item that Xhalent raised in his review. You should see these for each of Xhalent's original review items with the exception of no 8.
Running the Code
You can download the source from codeplex as described above or you can paste either of the code files into a C# console application and just add a reference to System.Drawing.