Uncategorized

A Bit of Code Review

So, I was perusing my HamQuest codebase, and looking through some of the oldest code in it. This is stuff that doesn’t really change, and is relied upon heavily.  Here’s the public interfaces:

public class CountedCollection<CountedType>
{
public Dictionary<CountedType, int>.KeyCollection Identifiers;
public void AddCount(CountedType identifier, int count);
public void Add(CountedType identifier);
public bool Has(CountedType identifier);
public int GetCount(CountedType identifier);
public int RemoveCount(CountedType identifier, int count);
public bool Remove(CountedType identifier);
}

CountedCollection is the basis for my inventory system. The implementation does some range checking, so that one cannot add or remove a negative number, and cannot remove more than what is had. It wraps a Dictionary collection internally.

Some of the things that “bother” me about this code:

  • It is using int instead of uint. Since the count can never go below 0, the range should not include negative numbers.
  • The Add/AddCount and Remove/RemoveCount should simply be overloads of Add and Remove.
  • The GetCount function should be replaced by a read only overload of the array operator.
  • There should be events that are triggered when items are added or removed, to allow other code to easily hook into and respond to changes in an added collection.

public class WeightedGenerator<GeneratedType>
{
public void Clear();
public void SetWeight(GeneratedType theValue, int theWeight);
public GeneratedType Generate();
public WeightedGenerator();
}

The WeightedGenerator is used for chests, generating door types, item drops, wandering monsters, summoned monsters, and probably a few other things. It wraps a Dictionary collection internally.

Issues with this class:

  • Weights should be uints, not ints. Negative weights don’t make sense.
  • There is no GetWeight function
  • Instead of GetWeight/SetWeight there should be overloaded array operator.

public class RandomNumberGenerator

{
public static Int32 Next(Int32 maximum);
public static Int32 Next(Int32 minimum,Int32 maximum);
}

In .NET, there is a Random class, and an instance is needed in order to generate a random number. I wanted to support the ability to use a single Random throughout the entire game. My solution was to make one statically available in the RandomNumberGenerator class. Really is just exposes the number generating functions of the Random class stored within it.

My Issues:

  • The class is not declared as static, and all fields and method are static. This would not affect anything in the slightest.
  • It would be really cool to be able to push and pop the Random class to use onto a stack to allow for overriding the random number generator.
  • Even better would be to make a class that has an interface similar to Random, and be able to supply my own random number generating function.

So, will I wind up doing any of these “enhancements”? Probably. One of the things I’m doing at this point is looking at parts of the code that I am likely to actually reuse elsewhere.  Most of this is in an assembly named PDGBoardGames, which has maze generation and utility stuff like the classes I posted here.