6
\$\begingroup\$

Based on responses to my question posted here, I've built a full implementation of a Location object. Such an object can be used to build a map for a text-based adventure game.

Feel free to review the code, leave some comments about the good and the bad of what you see.

Things to notice

Objects of this class can only be constructed via its static factory method.

This class is immutable, yet has circular references to other objects of the same type (causing a 'chicken or the egg' conundrum). The circular references are made possible via lazy evaluation: the factory method accepts a Func{Location} delegate to map those references.

Each circular reference is hidden behind a public property (North, South, East, West). Each call to the delegate is tucked within a try-catch block, just in case each delegate contains an error condition, so that a meaningful exception message can be thrown.

using System;
using System.Diagnostics.Contracts;

/// <summary>
/// Represents a location within the game. Instances of this class are immutable.
/// </summary>
public sealed class Location
{
    /// <summary>
    /// The description of this location.
    /// </summary>
    private readonly string description;

    /// <summary>
    /// The delegate that returns the location that is to the east of this location. -or- The delegate that returns <c>null</c> if there is no location to the
    /// east.
    /// </summary>
    private readonly Func<Location> east;

    /// <summary>
    /// The name of this location.
    /// </summary>
    private readonly string name;

    /// <summary>
    /// The description of the location that is displayed to the game player.
    /// </summary>
    private readonly string narration;

    /// <summary>
    /// The delegate that returns the location that is to the north of this location. -or- The delegate that returns <c>null</c> if there is no location to the
    /// north.
    /// </summary>
    private readonly Func<Location> north;

    /// <summary>
    /// The delegate that returns the location that is to the south of this location. -or- The delegate that returns <c>null</c> if there is no location to the
    /// south.
    /// </summary>
    private readonly Func<Location> south;

    /// <summary>
    /// The delegate that returns the location that is to the west of this location. -or- The delegate that returns <c>null</c> if there is no location to the
    /// west.
    /// </summary>
    private readonly Func<Location> west;

    /// <summary>
    /// Initializes a new instance of the <see cref="Location"/> class.
    /// </summary>
    private Location()
        : this(name: "[Empty]", description: "[Empty]", narration: "[Empty]", north: () => null, south: () => null, east: () => null, west: () => null)
    {
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="Location"/> class.
    /// </summary>
    /// <param name="name">
    /// The name of the location.
    /// </param>
    /// <param name="description">
    /// The description of the location.
    /// </param>
    /// <param name="narration">
    /// The description of the location that is displayed to the game player.
    /// </param>
    /// <param name="north">
    /// A delegate that returns the location that is to the north of this location. -or- A delegate that returns <c>null</c> if there is no location to the
    /// north.
    /// </param>
    /// <param name="south">
    /// A delegate that returns the location that is to the south of this location. -or- A delegate that returns <c>null</c> if there is no location to the
    /// south.
    /// </param>
    /// <param name="east">
    /// A delegate that returns the location that is to the east of this location. -or- A delegate that returns <c>null</c> if there is no location to the east.
    /// </param>
    /// <param name="west">
    /// A delegate that returns the location that is to the west of this location. -or- A delegate that returns <c>null</c> if there is no location to the west.
    /// </param>
    private Location(
        string name, string description, string narration, Func<Location> north, Func<Location> south, Func<Location> east, Func<Location> west)
    {
        this.name = name;
        this.description = description;
        this.narration = narration;
        this.north = north;
        this.south = south;
        this.east = east;
        this.west = west;
    }

    /// <summary>
    /// Gets an object that represents an empty location. This property is a pure function. Never returns <c>null</c>.
    /// </summary>
    [Pure]
    public static Location Empty { get; } = new Location();

    /// <summary>
    /// Gets the description of the location that is displayed to the game player. This property is a pure function. Never returns <c>null</c>.
    /// </summary>
    [Pure]
    public string Narration
    {
        get
        {
            Contract.Ensures(null != Contract.Result<string>());
            return this.narration;
        }
    }

    /// <summary>
    /// Gets the location that is to the north of the this location.
    /// </summary>
    public Location North
    {
        get
        {
            return this.north();
        }
    }

    /// <summary>
    /// Gets the location that is to the south of this location.
    /// </summary>
    public Location South
    {
        get
        {
            return this.south();
        }
    }

    /// <summary>
    /// Gets the location that is to the east of this location.
    public Location East
    {
        get
        {
            return this.east();
        }
    }

    /// <summary>
    /// Gets the location that is to the west of this location.
    /// </summary>
    public Location West
    {
        get
        {
            return this.west();
        }
    }

    [Pure]
    public static bool operator !=(Location x, Location y)
    {
        return !(x == y);
    }

    [Pure]
    public static bool operator ==(Location x, Location y)
    {
        if (Object.ReferenceEquals(x, y))
            return true;

        if (((object)x == null) || ((object)y == null))
            return false;

        return x.Equals(y);
    }

    /// <summary>
    /// Creates a new instance of the <see cref="Location"/> class. Never returns <c>null</c>.
    /// </summary>
    /// <param name="name">
    /// The name of the location.
    /// </param>
    /// <param name="description">
    /// The description of the location.
    /// </param>
    /// <param name="north">
    /// A delegate that returns the location that is to the north of this location.
    /// </param>
    /// <param name="south">
    /// A delegate that returns the location that is to the south of this location.
    /// </param>
    /// <param name="east">
    /// A delegate that returns the location that is to the east of this location.
    /// </param>
    /// <param name="west">
    /// A delegate that returns the location that is to the west of this location.
    /// </param>
    /// <exception cref="ArgumentNullException">
    /// <paramref name="name"/> or <paramref name="description"/> is <c>null</c>.
    /// </exception>
    /// <returns>
    /// A new instance of <see cref="Location"/>.
    /// </returns>
    public static Location Create(
        string name, string description, Func<Location> north = null, Func<Location> south = null, Func<Location> east = null, Func<Location> west = null)
    {
        if (null == name)
        {
            throw new ArgumentNullException(
                message: $"Context: Creating a new instance of {nameof(Location)}.{Environment.NewLine}" +
                         $"Problem: Attempted to create a new instance of {nameof(Location)} with a {nameof(name)} that is a null reference.",
                paramName: nameof(name));
        }

        Contract.Ensures(null != Contract.Result<Location>());

        try
        {
            north?.Invoke();
        }
        catch (Exception e)
        {
            throw new GameException(
                message: $"Context: Creating a new instance of {nameof(Location)}.{Environment.NewLine}" +
                         $"Problem: Attempted to create a new instance of {nameof(Location)} by passing via the {nameof(north)} parameter a delegate that " +
                         $"contains an error condition.{Environment.NewLine}" +
                         $"Possible Solution: Examine the initialization of the game map and ensure that delegates used to create {nameof(Location)} " +
                         $"instances do not contain error conditions.{Environment.NewLine}" +
                         $"Remarks: See inner exception.",
                inner: e);
        }

        try
        {
            south?.Invoke();
        }
        catch (Exception e)
        {
            throw new GameException(
                message: $"Context: Creating a new instance of {nameof(Location)}.{Environment.NewLine}" +
                         $"Problem: Attempted to create a new instance of {nameof(Location)} by passing via the {nameof(south)} parameter a delegate that " +
                         $"contains an error condition.{Environment.NewLine}" +
                         $"Possible Solution: Examine the initialization of the game map and ensure that delegates used to create {nameof(Location)} " +
                         $"instances do not contain error conditions.{Environment.NewLine}" +
                         $"Remarks: See inner exception.",
                inner: e);
        }

        try
        {
            east?.Invoke();
        }
        catch (Exception e)
        {
            throw new GameException(
                message: $"Context: Creating a new instance of {nameof(Location)}.{Environment.NewLine}" +
                         $"Problem: Attempted to create a new instance of {nameof(Location)} by passing via the {nameof(east)} parameter a delegate that " +
                         $"contains an error condition.{Environment.NewLine}" +
                         $"Possible Solution: Examine the initialization of the game map and ensure that delegates used to create {nameof(Location)} " +
                         $"instances do not contain error conditions.{Environment.NewLine}" +
                         $"Remarks: See inner exception.",
                inner: e);
        }

        try
        {
            west?.Invoke();
        }
        catch (Exception e)
        {
            throw new GameException(
                message: $"Context: Creating a new instance of {nameof(Location)}.{Environment.NewLine}" +
                         $"Problem: Attempted to create a new instance of {nameof(Location)} by passing via the {nameof(west)} parameter a delegate that " +
                         $"contains an error condition.{Environment.NewLine}" +
                         $"Possible Solution: Examine the initialization of the game map and ensure that delegates used to create {nameof(Location)} " +
                         $"instances do not contain error conditions.{Environment.NewLine}" +
                         $"Remarks: See inner exception.",
                inner: e);
        }

        string northExit, southExit, eastExit, westExit, exits, separator, narration;

        northExit =
            null != north
            ? " North"
            : string.Empty;

        southExit =
            null != south
            ? " South"
            : string.Empty;

        eastExit =
            null != east
            ? " East"
            : string.Empty;

        westExit =
            null != west
            ? " West"
            : string.Empty;

        exits =
            string.Empty != string.Concat(northExit, southExit, eastExit, westExit)
            ? $"{Environment.NewLine}Exits:{northExit}{southExit}{eastExit}{westExit}"
            : string.Empty;

        separator = new string('-', name.Length);

        narration =
            $"{name}{Environment.NewLine}" +
            $"{separator}{Environment.NewLine}" +
            $"{description}{Environment.NewLine}" +
            $"{separator}" +
            $"{exits}";

        Func<Location> emptyLocationDelegate = () => Location.Empty;

        return new Location(
            name: name,
            description: description ?? string.Empty,
            narration: narration,
            north: north ?? emptyLocationDelegate,
            south: south ?? emptyLocationDelegate,
            east: east ?? emptyLocationDelegate,
            west: west ?? emptyLocationDelegate
         );
    }

    [Pure]
    public override bool Equals(object obj)
    {
        var other = obj as Location;

        if (ReferenceEquals(other, null))
            return false;

        return other.name == this.name
            && other.description == this.description
            && other.narration == this.narration
            && other.North == this.North
            && other.South == this.South
            && other.East == this.East
            && other.West == this.West;
    }

    [Pure]
    public override int GetHashCode()
    {
        int hashCode = new {
            Name = this.name,
            Description = this.description,
            Narration = this.narration,
            North = this.North,
            South = this.South,
            East = this.East,
            West = this.West
        }.GetHashCode();

        return hashCode;
    }

    /// <summary>
    /// Returns a <see cref="string"/> that represents the current object. This method is a pure function. Never returns <c>null</c>.
    /// </summary>
    /// <returns>
    /// A <see cref="string"/> that represents the current object.
    /// </returns>
    [Pure]
    public override string ToString()
    {
        Contract.Ensures(null != Contract.Result<string>());
        return this.name;
    }

    /// <summary>
    /// Used by Microsoft Code Contracts to verify object invariants.
    /// </summary>
    [ContractInvariantMethod]
    private void ObjectInvariants()
    {
        Contract.Invariant(null != this.name);
        Contract.Invariant(null != this.description);
        Contract.Invariant(null != this.narration);
        Contract.Invariant(null != this.north);
        Contract.Invariant(null != this.south);
        Contract.Invariant(null != this.east);
        Contract.Invariant(null != this.west);
    }
}

And here's how client code would initialize Location objects:

public static class GameMap
{
    /// <summary>
    /// Creates the game map and returns the starting <see cref="Location"/>.
    /// </summary>
    /// <returns>
    /// The <see cref="Location"/> that is the start of the map.
    /// </returns>
    [Pure]
    public static Location Initialize()
    {
        Location kitchen = null;
        Location library = null;
        Location office = null;

        kitchen = Location.Create(
            name: "The kitchen", 
            description: "You are in a messy kitchen.",
            north: () => library
         );

        library = Location.Create(
            name: "The old library",
            description: "You are in a large library. The walls are lined with old, dusty books.",
            north: () => office,
            south: () => kitchen
        );

        office = Location.Create(
            name: "The office",
            description: "You are in an office",                
            south: () => library
        );

        return kitchen;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I find that I'm not liking this implementation. The problem is that there is so much code to accommodate exceptions that could throw from the delegates passed into the constructor. Immutability is supposed to simplify code, not 'complexify' it. LOL \$\endgroup\$ – Rock Anthony Johnson Oct 25 '16 at 6:29
  • 1
    \$\begingroup\$ You can edit your original code to the updated one. There is no harm in changing it as long as there are no reviews yet. With multiple versions it's not clear which one should be reviewed. \$\endgroup\$ – t3chb0t Oct 26 '16 at 19:59
3
+100
\$\begingroup\$

It's been since awhile I've done .NET dev but I'll take a stab at it.

Style

  • Line length. Some of the lines are over 150 characters in length. While I can't find a concrete guideline in Microsoft's Coding Conventions, I find this hard to read on a 13" screen. I suggest limiting lines to 100 character width by splitting where possible. Example:

    private Location()
        : this(
            name: "[Empty]",
            description: "[Empty]",
            narration: "[Empty]",
            north: () => null,
            south: () => null,
            east: () => null,
            west: () => null
    )
    
  • Minor style nitpick. Throughout you have null checking in the form: if (null == name). This sticks out as odd and distracting. Prefer the more common if (name == null).

OO/Design

  • Unnecessary member? The following:

    /// <param name="description">
    /// The description of the location.
    /// </param>
    /// <param name="narration">
    /// The description of the location that is displayed to the game player.
    

    is suspect. If narration is just a variation of description then why is it needed as a member? What you've submitted doesn't contain usage context to better understand but it seems that narration is more suited as some kind of getter method that operates on description to return a string.

  • Too many arguments. Your constructor signature is immediately suspect:

    private Location(
        string name,
        string description,
        string narration,
        Func<Location> north,
        Func<Location> south,
        Func<Location> east,
        Func<Location> west)
    

From Robert Martin's book Clean Code:

"Functions should have a small number of arguments. No argument is best, followed by one, two, and three. More than three is very questionable and should be avoided with prejudice." (Martin, 288)

So how to fix? Wrap them in a new class (or some other structure). Assuming we can remove narration from the previous suggestion, this leaves us with name, description, and the four directions. The directions really stick out here, especially with all the repetition going on in Create(). Wrap it up.

  • Don't Repeat Yourself (DRY). As previously mentioned, Create() has a ton of repetition and is way too long. Repetitive code is a smell and should indicate that improvements can be made. All of your direction related code does the same thing - wrap it in a class and eliminate all the duplication.

  • Methods should be short and do one thing. Create() is too long. Extract helper methods (ref) as it appropriate. There's a lot of cruft in setting strings; this should probably be abstracted somewhere with default initializers.

Misc

  • Code Contracts. I wasn't aware of this feature prior to this. I'm probably not experienced enough to have a full view but my initial impression is one of skepticism. They seem likely to introduce much clutter as well as possible performance issues at higher scale. Everything related to how you've used them currently can be handled by unit tests. I would recommend being mindful of potential future consequences when deciding to use such a feature. I'm not saying don't use them, but rather make sure you have a good understanding before going down that path.

There's more to be said but unfortunately I'm out of time.

\$\endgroup\$
  • \$\begingroup\$ You've done a fine job!!! I take exception with only two of your critiques: 1. I think to have seven or fewer arguments is acceptable; 2. Code Contracts provides static analysis, something that unit tests can't achieve. Everything else is spot on, I'm in total agreement with you! I'm hoping that you'll review my future postings!!!! \$\endgroup\$ – Rock Anthony Johnson Oct 26 '16 at 21:25
4
\$\begingroup\$

The circular references are made possible via lazy evaluation

The evaluation is not really lazy yet.

To make it truly lazy you need to use the Lazy<T> Class.

Use lazy initialization to defer the creation of a large or resource-intensive object, or the execution of a resource-intensive task, particularly when such creation or execution might not occur during the lifetime of the program.

This will give you a much better performance for large maps because it will create the nested objects only once unlike in your current solution where it creates each object everytime you call any of the factory Funcs via the East/West etc. properties.

If you have any loops that traverse the map it will be created over and over again.

Here's shortened example:

public sealed class Location
{
    private readonly Lazy<Location> _lazyEast;
    private readonly Lazy<Location> _lazyWest;

    private Location(string name, Func<Location> eastFactory, Func<Location> westFactory)
    {
        Name = name;
        _lazyEast = eastFactory == null ? null : new Lazy<Location>(eastFactory);
        _lazyWest = westFactory == null ? null : new Lazy<Location>(westFactory);
    }

    public string Name { get; }
    public Location East => _lazyEast?.Value;
    public Location West => _lazyWest?.Value;

    public static Location Create(string name, Func<Location> eastFactory = null, Func<Location> westFactory = null)
    {
        return new Location(name, eastFactory, westFactory);
    }

    public override string ToString() => Name;
}

You would use it the same way you do now:

Location kitchen = null;
Location library = null;
Location office = null;

kitchen = Location.Create(
    name: "The kitchen", 
    eastFactory: () => library
 );

library = Location.Create(
    name: "The old library",
    eastFactory: () => office,
    westFactory: () => kitchen
);

office = Location.Create(
    name: "The office",
    westFactory: () => library
);

Produces:

kitchen.Dump();
library.Dump();
office.Dump();

kitchen library office

Other issues have already been mentioned by @jsuth.

\$\endgroup\$
  • \$\begingroup\$ Nice, very nice! What is this Dump() method that provides visual representation? \$\endgroup\$ – Rock Anthony Johnson Oct 28 '16 at 18:44
  • 1
    \$\begingroup\$ @RockAnthonyJohnson oh, Dump() comes with LINQPad ;-) and the screenshots are from it too \$\endgroup\$ – t3chb0t Oct 28 '16 at 18:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.