Using NDepend to analyze Mud Designer Alpha

Why analyze?

Since the latest build of the Mud Designer re-write isn't ready to ship yet, I thought I'd go back and run a code analyizer against the original Mud Designer Alpha, so that I can compare the quality of the old engine to the new (once it is ready). The goal of the re-write was to improve the code base and make it more abstract and not as tightly coupled.

I was provided a copy of the NDepend toolchain and used it to run the first report against the Alpha version of the Mud Designer and thought I'd share the results of it.

The stats

After the analysis was completed, it provided me with some basic stats.

  1. The project contained 3,852 lines of code, of which 47% was commented. Pretty good coverage, almost to much. Half of my code base shouldn't be covered in comments if the code was wrote clearly. The project
  2. The project contained 152 Types across 5 assemblies with 1,091 methods.
  3. I did not have any rules with errors, but ended up with 61 rule violations in total. I'm not sure if that is a significant number for the number of lines/methods/types the project has or if it is minor.

The violations

I had two violations for Types being to large and one for a method that was to complex. These two violations were anticipated before hand, since all three of them take place in the WinForms code-behind. Code-behind on a WinForms project tend to be ugly and cumbersome, so this was expected. It's also a large part of why I chose to build the next version in WPF with the MVVM pattern.

The report also told me that I had a total of 8 methods that needed to be refactored. With 4 of them being engine methods and 4 of them being WinForms project methods. Once again, the amount of code in violation within the engine was minor next to the WinForm project. The WinForm project just over double the number of IL instructions (117 vs 242) than the rest of the engine.

Something that I had not expected was that I had 9 Types with to many methods. With all but one of those Types living within the Engine assemblies. The bulk of the bloated Types lived under the GameObjects namespace, which was responsible for all of the player, item, world and npc code. Reviewing the code and the analysis showed that the objects could really use a good refactor. Something that I was already planning on the next version of the application. The BaseMob Type contained 81 methods by itself. It made me cringe and not want to even go back and look at the source.

I'm not going to go over every rule violation that took place in this report with this post, but there are a few interesting things to point out.

The report showed that I had a problem with cohesive methods, potential issues due to invoking virtual members within my constructors and that I was not properly disposing of Types that implemented the IDisposable interface. I'm really anxious to get the dust to settle on the new version of the Mud Engine source so that I can run the tool against it. I am planning on a series of posts as I build out the new engine and run the analysis tool against it. The goal of the series would be to demonstrate rule violations and how to address and fix them.

There's really no question that using NDepend will help make my code base much better. It is fairly complex at first when you go to use it, but their documentation seems pretty good. Using their documentation with the analysis and a bit of googling, you can quickly pick up what patterns can be applied through-out your app. It's really nice.

Stay tuned for additional posts coming soon.

Command & State management in Mud Designer

The previous version of the Mud Designer had a pretty good State system. The user would enter a command, the command would perform an action and the state would render the results to the screen. I spent a lot of time over the last couple of weekends trying to refactor the state system and add a couple new features.

Command Searching

I provided the player with a StateManager property, that the object can now access. This is used to try and find a command suitable to what the user typed, and execute it. In the new version of the Mud Designer, the delegate for receiving the user input, will call StateManager.PerformCommand which will try to find a command that can be used.

There are several ways that a Command can be found and executed. The manager makes an attempt to see if the command is stored in a collection of commands that the player owns. If it is, then the command is executed. If not, then the manager checks to see if the command has any short hand attributes. A command can be decorated with a short hand like this:

[ShorthandNameAttribute("Walk", "mv")]
public class WalkCommand : ICommand
{
}

This allows the actual command name, to vary from that of the class name. While you can provide any name, it is strongly recommended that builders implementing their own commands stick with a convention that the Mud Designer uses. That convention is to name your Command, with the command word appended. Like LookCommand, WalkCommand, TellCommand etc. With the attribute, you can shorten the name to just Look, Walk or Tell. You can shorten the name even further using the secondary argument. The three aforementioned commands can be shortend to l, w and t if you want to.

If no command is found matching our convention (user enters Walk, so we look for WalkCommand), then the manager will check all of the commands for a ShorthandNameAttribute. The ones that contain this attribute are checked. If any of them have a command name or shortened command that match the users input, then we have a match. Otherwise we continue our search.

The Stack.

The manager now has a Stack for commands. If a command is not completed, it pushes the command on to the stack and resumes control to the players individual game loop. If the command is completed, the command is popped off of the stack.

Once the manager has failed to find a command that matches the input provided by the user, it hits up the stack. If the stack contains a command, it pops it off and executes the command.

When a command's .Execute() method is finished, it can set a IsIncomplete property to true. If the property is set to true, it lets the State Managerk now that the command still has more steps to perform before it is really finished. Something like a User Login might require multiple steps (username and password entry). While it is possible to fetch the data directly from the user while in the command, this is now frowned on as it holds the thread up. It is better to return control to the caller and wait for the user to enter data in to the main player loop. When the new data is entered, the State Manager will pass that data back to the same command, which can then finish off what it needs to.

When a command is marked as incomplete, it is pushed on to the stack. When the command is executed, it is popped off of the stack. Only commands that must be preserved for additional actions are placed in the stack.

Example Command:

The following is an example of a single action command.

using System;
using MudEngine.Engine.Core;
using MudEngine.Engine.Networking;

namespace MudEngine.Engine.Commands
{
    [ShorthandName("Disconnect", "/dc")]
    public class DisconnectCommand : ICommand
    {
        public string CommandInput { get; private set; }

        public bool IsIncomplete { get; }

        public void Execute(GameObjects.Mob.IMob mob, string input)
        {
            if (mob is ServerPlayer)
            {
                var player = mob as ServerPlayer;

                mob.Send(new InformationalMessage("Disconnecting."));
                player.Disconnect();
            }
        }
    }
}

Pretty straight forward. You can see how we set our shorthand names, send a message to the player that we are disconnecting and then disconnect them. The player can type Disconnect or /DC and the game will disconnect them from the server.

Example Command with State

Now we want to maintain state in our command. Let's create a simple "Provide your name and age" command used during character creation.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using MudEngine.Engine.Core;
using MudEngine.Engine.GameObjects.Mob.States;
using MudEngine.Engine.Networking;

namespace MudEngine.Engine.Commands
{
    public class CreateCharacterCommand : ICommand
    {
        int state = 0; // 0 = uninitialized; 1 = username entered, 2 = age entered.
        public string CommandInput { get; private set; }

        public bool IsIncomplete { get; private set; }

        public void Execute(GameObjects.Mob.IMob mob, string input)
        {
            // First execution of the method.
            if (!this.IsIncomplete)
            {
                state = 0;
                mob.Send(new InputMessage("Please enter your name"));
                this.IsIncomplete = true;
                mob.StateManager.SwitchState< ReceivingInputState>();
            }
            else if (this.state == 0)
            {
                this.CommandInput = input;
                mob.Send(new InformationalMessage(string.Format("Hello {0}", this.CommandInput)));
                mob.Send(new InputMessage("Please enter your age"));
                this.IsIncomplete = true;
                this.state = 1;
                mob.StateManager.SwitchState< ReceivingInputState>();
            }
            else
            {
                // Out put the age from current input along with name from previous command input.
                mob.Send(new InformationalMessage(string.Format("You are {0}s old {1}", input, this.CommandInput)));
                this.IsIncomplete = false;
                this.state = 2;
                mob.StateManager.SwitchState< LookState>();
            }
        }
    }
}

As you can see in here, we maintain some simple state to determine where in our user data input state we are. The state manager will execute this command once, then determine that it is not completed and push it to the stack. When the user enters "Bob" for a user name, the State Manager will not find a command matching "Bob" and will pop the previous command off the stack and execute it.

The benefit here is that the player can type "Help", which the state manager will find. So the user can be in the middle of a command, and piggy back on top of it to execute another. This will work really well going forward and allow for some complex command chaining as well.

Conclusion

While commands at the moment can be executed without a ShothandNameAttribute decorated on the class, I am considering making this a requirement. By requiring all commands be given at least a name (shorthand version optional), I can allow the engine (and the commands wrote for it) to execute internal commands. A "CreateCharacter" command would not be executable from a user, since there are now ShorthandNameAttributes associated with it. Yet, your Login command could request that the players StateManager perform the command.

mob.StateManager.PerformCommand(new ReceivedInputMessage("CreateCharacter"));

This isn't wrote in to the engine yet, but this is something I am considering before shipping.

Messaging in Mud Designer

One of the things that bugged me with the previous version of the Mud Designer was how I had essentially hard-coded the encoding of the text directly in to the flow of transfering the data back and forth between the user and the server. The old version of the engine would send content directly to the user like this:

player.SendMessage("What is your username, adventurer? ", false);

and the SendMessage looked like:

public override void SendMessage(string message, bool newLine = true)
{
    // When printing properties that don't have values, they'll
    // be null.
    if (message == null)
        return;

    if (newLine && !lastMessageHadNewLine)
        message = message.Insert(0, System.Environment.NewLine);

    if (newLine)
    {
        message += System.Environment.NewLine;
        lastMessageHadNewLine = true;
    }
    else
        this.lastMessageHadNewLine = false;

    // Make sure we are still connected
    try
    {
        if (IsConnected)
            Connection.Send(new ASCIIEncoding().GetBytes(message));
    }
    catch (Exception ex)
    {
        // No connection was made, so make sure we clean up
        if (!IsConnected)
            Disconnect();
    }
}

So, as you can see the formatting of the content sent to the player was pretty static. If you wanted to change how the text was formatted, you had to modify this one method. If you wanted to have multiple types of formatting it was almost impossible because you would not know what the content of the message was once it was within the SendMessage method. You only know that there is text that must be sent. This was changed in the re-write. Now, all messages are wrapped within an IMessage object. The object must have a property called Message which contains the actual message and a method called FormatMessage(). The interface looks like this:

public interface IMessage
{
    /// < summary>
    /// Gets or sets the message.
    /// < /summary>
    string Message { get; set; }

    /// < summary>
    /// Formats the message.
    /// < /summary>
    /// < returns>Returns a formatted message.</returns>
    string FormatMessage();
}

Why is this a good thing? Now you can customize how you want your message to be sent to the player. For instance, I have an IState object that handles the users login. The state renders both a welcome message to the user along with a prompt to enter the users name. I can now format both messages differently, by creating two different IMessage objects. One for showing information and one for asking for input.

this.connectedPlayer.Send(new InformationalMessage("Invalid username/password specified."));
this.currentState = CurrentState.FetchUserName;
this.connectedPlayer.Send(new InputMessage("Please enter your user name"));

The above code outputs the following to the players console:

Invalid username/password specified.
Please enter your user name >:

The first IMessage type is implemented with out any formatting. It just takes the message and appends a new line to the end of it.

public class InformationalMessage : IMessage
{
    public InformationalMessage(string message)
    {
        this.Message = message;
    }

    /// < summary>
    /// Gets or sets the message.
    /// < /summary>
    public string Message { get; set; }

    /// < summary>
    /// Formats the message.
    /// < /summary>
    public string FormatMessage()
    {
        return this.Message += Environment.NewLine;
    }
}

The second one, InputMessage formats the message by appending a >: to the end of the message. This indicates that the user must enter a command in order to continue.

public class InputMessage : IMessage
{
    /// < summary>
    /// Initializes a new instance of the <see data-preserve-html-node="true" cref="InputMessage"/> class.
    /// < /summary>
    /// < param name="message">The message.</param>
    public InputMessage(string message)
    {
        this.Message = message;
    }

    /// < summary>
    /// Gets or sets the message.
    /// < /summary>
    public string Message { get; set; }


    /// < summary>
    /// Formats the message.
    /// < /summary>
    public string FormatMessage()
    {
        return string.Format("{0}>: ", this.Message);
    }
}

By requiring all of the methods within the engine that pass messages around, to be supplied with an IMessage implementation, the engine can display data differently to the user based off of what the context of that data is. This should make presenting content more flexible in the engine going forward.

Fetch Factories based on Generic Constraint Requirements in C#

While trying to solve my Data Persistence issues, I discovered another small problem that would need to be worked out. Since the IPeristedStorage objects are generic, it does not expose anyway for you to fetch the factory associated with objects you are trying to load. So invoking the following can be problematic.

this.Worlds = this.StorageSource.Load< IWorld>().ToList();

Since the method signature is:

public IEnumerable< T> Load< T>() where T : class

We can provide any Type in the Load method, but the Load method won't know what factory to use to fetch the Type. How does the storage source know what factory to use? I could possibly force it with a secondary generic parameter:

this.Worlds = this.StorageSource.Load< IWorld, WorldFactory>().ToList();

That tightly couples the engines restore code to that factory. What happens if someone wants to create their own factory? Out of luck chuck. We can't have that can we? So what do we do? Well, we create a factory object that can be used to fetch other factories of course! Lets start with what we want to get back, we know we want to get back a collection of IWorld objects right? The StorageSource doesn't know what T (IWorld) is ahead of time (only at run-time) so we have to take T and find a factory that supports it.

Our Factory method would look like this:

Type factoryType = EngineFactory.FindFactory< T>();

Creating the initial Factory

To start out, we need to force all of our Factorys to implement a new interface, IFactory. This is what the interface looks like.

/// < summary>
/// Defines a contract for creating a Factory object.
/// < /summary>
/// < typeparam name="T">An interface that the factory is targeting.< /typeparam>
public interface IFactory< T>
{
    /// < summary>
    /// Gets a collection of Types matching < T>.
    /// < /summary>
    /// < param name="fromAssemblies">From assemblies.< /param>
    /// < returns>Returns a collection of objects matching T< /returns>
    List< T> GetObjects(Assembly[] fromAssemblies = null, string[] compatibleTypes = null);

    /// < summary>
    /// Gets a single Type matching < T>.
    /// < /summary>
    /// < typeparam name="UTypeToFetch">The type of the type to fetch.< /typeparam>
    /// < param name="fromAssemblies">From assemblies.< /param>
    /// < returns>Returns an instance of UTypeToFetch casted as T.< /returns>
    T GetObject< UTypeToFetch>(Assembly[] fromAssemblies = null, string compatibleType = null);
}

Now that we have an interface defined we can start writing our factories. We have two methods that they must implement, one that fetches all Types that match < T> and a method that fetches one Type that matches < T>. It is important to note that there are no constraints set on this interface. < T> can be anything at this point. We will set the constraints on the actual Factory implementations instead.

For our first Factory, we will create the WorldFactory object. This factory will fetch all objects implementing IWorld and return them to the caller.

/// < summary>
/// Provides a means to fetch objects that implement the IWorld interface
/// < /summary>
public class WorldFactory < T> : IFactory< T> where T : class, IWorld
{
    /// < summary>
    /// Gets a collection objects implementing IWorld.
    /// < /summary>
    /// < returns>A collection of objects in memory implementing IWorld< /returns>
    public List< T> GetObjects(Assembly[] fromAssemblies = null, string[] compatibleTypes = null) 
    {
        var types = new List< Type>();

        if (compatibleTypes == null)
        {
            compatibleTypes = new string[0];
        }

        // Loop through each assembly in our current app domain
        // generating a collection of Types that implement IWorld
        // If we are not provided with assemblies, we fetch all of them from the current domain.
        foreach (Assembly assembly in fromAssemblies ?? AppDomain.CurrentDomain.GetAssemblies())
        {
            types.AddRange(assembly.GetTypes().Where(
                type => (type.GetInterface(typeof(T).Name) != null || compatibleTypes.Contains(type.Name)) &&
                !type.IsAbstract && // Do not add abstract classes
                !type.IsInterface)); // Do not add interfaces. Concrete Types only.
        }

        // Convert our collection or Types into instances of IWorld
        // then return the IWorld collection.
        return new List< T>(
            (from type in types
             select Activator.CreateInstance(type) as T));
    }

    /// < summary>
    /// Gets the World specified.
    /// < /summary>
    /// < typeparam name="T">The Type implementing IWorld that you want to find< /typeparam>
    /// < returns>Returns the World specified.< /returns>
    public T GetObject< UTypeToFetch>(Assembly[] fromAssemblies = null, string compatibleType = null)
    {
        // This isn't the most efficient.
        // Considering that GetWorlds should really only be returning a few (1-5?) IWorld objects
        // Filtering the list a second time (filtered once in GetWorlds) shouldn't hurt much.
        // If users start loading dozens of IWorlds (bad practice!) in their scripts folder, then
        // this needs to be revisited.
        List< T> results = this.GetObjects(fromAssemblies);
        T selectedType = null;

        if (compatibleType != null)
        {
            selectedType = results.FirstOrDefault(World => World.GetType().Name == compatibleType);
        }
        else
        {
            selectedType = results.FirstOrDefault(World => World.GetType() == typeof(UTypeToFetch));
        }

        return selectedType;
    }
}

The meat of this factory is in this chunk of code:

foreach (Assembly assembly in fromAssemblies ?? AppDomain.CurrentDomain.GetAssemblies())
{
    types.AddRange(assembly.GetTypes().Where(
        type => (type.GetInterface(typeof(T).Name) != null || compatibleTypes.Contains(type.Name)) &&
        !type.IsAbstract && // Do not add abstract classes
        !type.IsInterface)); // Do not add interfaces. Concrete Types only.
}

In this code, we loop through each assembly loaded in memory and check the Types they contain for any that are concrete classes implementing < T>. Since we have set a constraint at the class level, requiring < T> to be an IWorld based object, we are guaranteed that any object that comes back will be an instance of an IWorld based object.

We add the onjects that are found to a collection of objects and then return the list. At this point, we have a full collection of all instances objects that implement IWorld. The next method, GetObject< T> allows us to specify just a single Type instead of every Type.

public T GetObject< UTypeToFetch>(Assembly[] fromAssemblies = null, string compatibleType = null)
{
    // This isn't the most efficient.
    // Considering that GetWorlds should really only be returning a few (1-5?) IWorld objects
    // Filtering the list a second time (filtered once in GetWorlds) shouldn't hurt much.
    // If users start loading dozens of IWorlds (bad practice!) in their scripts folder, then
    // this needs to be revisited.
    List< T> results = this.GetObjects(fromAssemblies);
    T selectedType = null;

    if (compatibleType != null)
    {
        selectedType = results.FirstOrDefault(World => World.GetType().Name == compatibleType);
    }
    else
    {
        selectedType = results.FirstOrDefault(World => World.GetType() == typeof(UTypeToFetch));
    }

    return selectedType;
}

With this method, we accept a secondary Type (such as DefaultWorld) and we will scan all of the Types that implement IWorld to find DefaultWorld (UTypeToFetch). We also provide an overload called compatibleType that can be used to specify what Type we want using a string. This is handy for when we want to search for a Type via a user interface of some kind.

With that, our first IFactory implementation is completed!

Creating a Factory to Fetch our Factory

Now remember our initial problem? We have an IDataPersisted object that has a method signature of Load< T> with no constraints. How does the IDataPersisted object know what factory to fetch < T> from? That's what we are going to solve next. We will create an EngineFactory object that accepts < T>, which will be the interface that the Factory we are looking for must support.

The EngineFactory object will only contain a single method, FindFactory< T>. If < T> in this case is IWorld then we must search all Types in memory and find one that both implements IFactory and has a Type constraint of IWorld.

/// < summary>
/// A simple factory that is used to fetch other factories
/// < /summary>
public class EngineFactory
{
    /// < summary>
    /// Finds a Factory that can be used with T
    /// < /summary>
    /// < typeparam name="T">The interface that the Factory found must support.< /typeparam>
    /// < returns>Returns a Factory that can fetch objects matching T< /returns>
    public static Type FindFactory< T>() where T : class
    {
        var supportedTypes = new List< Type>();

        // Loop through each assembly in memory, fetching the Types they contain.
        foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies())
        {
            // We only want Types that satisfy the following criteria:
            // 1: Must be Generic (as it must support being given < T> specified
            // 2: Must implelemt IFactory, so we have guaranteed method signatures
            Type[] types = assembly.GetTypes()
                .Where(t => t.IsGenericType && t.GetInterfaces()
                    .FirstOrDefault(i => i.IsGenericType && i.GetGenericTypeDefinition() ==  typeof(IFactory< >)) != null)
                    .ToArray();

            // Now that we have the Types meeting our criteria, we loop
            // through them to determine which have Constraints matching
            // the < T> supplied to us.
            foreach (Type type in types)
            {
                foreach (var arg in type.GetGenericArguments())
                {
                    Type constraint = arg.GetGenericParameterConstraints().FirstOrDefault();

                    // Does this constraint match T?
                    if (constraint == typeof(T))
                    {
                        // If so, then we have found a Factory object that 
                        // can take < T> and return a concrete Type matching it.
                        return type;
                    }
                }

                continue;
            }
        }

        return null;
    }
}

The end result of the above method, will give us back a Factory that can take < T> and return a concrete object already instanced.

Putting it to use.

Now we are ready to go, we can implement a Generic load method using the following code.

public IEnumerable< T> Load< T>() where T : class
{
    // Attempt to find a factory that supports < T>
    Type factoryType = EngineFactory.FindFactory< T>();

    // Create an instance of the Factory found, and provide it with < T> as its constraint.
    IFactory< T> instance = Activator.CreateInstance(factoryType.MakeGenericType(typeof(T))) as IFactory< T>;

    // Instance the objects associated with < T>
    var result = instance.GetObjects();

    // Return our collection of < T>.
    return result as IEnumerable< T>;
}

Our load method is all wired up, factory fetcher is created and our WorldFactory implemented. Now we can call Load< T> and pass in any Type and it will return all of the objects we want.

this.Worlds = this.StorageSource.Load< IWorld>().ToList();

We can also call

this.Players = this.StorageSource.Load< IPlayer>().ToList();

The exact same object and method can be used, and we will get back a collection of IPlayer objects (assuming a PlayerFactory object implementing IFactory exists).

Now you can create Factories to your hearts content, make sure and set their constraints properly and the engine will fetch and use them for you.

Routing messages between objects

One of the things I really want to address and fix with this version of the Mud Designer is message routing. If the user sends a command, I want the command to be routed through a central location that knows what to do with it. Is it a single player game or multiplayer? Based on the kind of game it is, the engine needs to route the information sent differently.

For offline games, all commands should just execute locally and spit back to the user the results. For online games the command must go through the server and all other server based objects that are affected must be notified. So coming up with a routing system is important.

User input isn't the only thing routing affects. The world environment and it's objects must be able to communicate to the user. If it is an online game, the environment must distribute it's messages to all users on the server. If it is a single player game, then the messages must go directly to the user locally and not over the network. The latest build of the engine allows for this. Essentially, all communication is routed through the IGame object, which can decode/encode messages before passing to and from the player object.

It'll be a much needed improvement.