18
\$\begingroup\$

I've been working through the book Head First C# and this is an exercise from chapter 7, which is about interfaces and abstract classes.

Note that the class design was mandated by the authors, I'm open to suggestions to improve.

I'd like a review on any and all aspects. My code ended up quite a bit different than the book's solution, because it's a few years old and I think "holds back" on newer C#/.NET features and syntax.

In a shared code base, I would include that XML documentation, but since this is just local code for learning, I didn't. Any and all feedback appreciated.


The authors' published implementation is here on GitHub. The idea is to implement a floor plan that can be explored, with room connections and doors to the outside. It's done through a WinForms application.

Here is a picture of the plan from the book. Sorry, couldn't find a screenshot.

floorplan

The app looks like this:

app


Location.cs

using System;

namespace House
{
    abstract class Location
    {
        public Location(string name) => Name = name;

        public string Name { get; private set; }
        public Location[] Exits;

        public virtual string Description
        {
            get
            {
                string description = $"You're standing in the {Name}. You see exits to the following places: \r\n";
                foreach (Location exit in Exits)
                {
                    description += $"— {exit.Name}";
                    if (exit != Exits[Exits.Length - 1])
                    {
                        description += "\r\n";
                    }
                }
                return description;
            }
        }
    }
}

Room.cs

using System;

namespace House
{
    class Room : Location
    {
        public Room(string name, string decoration) 
            : base(name)
        {
            Decoration = decoration;
        }

        private string Decoration { get; set; }

        public override string Description => $"You see {Decoration}. {base.Description} ";
    }
}

Outside.cs

using System;

namespace House
{
    class Outside : Location
    {
        public Outside(string name, bool hot)
            : base(name)
        {
            Hot = hot;
        }

        private bool Hot { get; }

        override public string Description => Hot 
            ? "It's very hot here. " + base.Description 
            : base.Description;
    }
}

IHasInteriorDoor.cs

using System;

namespace House
{
    interface IHasExteriorDoor
    {
        string DoorDescription { get; }
        Location DoorLocation { get; }
    }
}

OutsideWithDoor.cs

using System;

namespace House
{
    class OutsideWithDoor : Outside, IHasExteriorDoor
    {
        public OutsideWithDoor(string name, bool hot, string doorDescription)
            : base(name, hot)
        {
            DoorDescription = doorDescription;
        }

        public string DoorDescription { get; private set; }

        public Location DoorLocation { get; set; }

        public override string Description => $"{base.Description}\r\n You see {DoorDescription} to go inside.";
    }
}

RoomWithDoor.cs

using System;

namespace House
{
    class RoomWithDoor : Room, IHasExteriorDoor
    {
        public RoomWithDoor(string name, string decoration, string doorDescription)
            : base(name, decoration)
        {
            DoorDescription = doorDescription;
        }

        public string DoorDescription { get; private set; }

        public Location DoorLocation { get; set; }
    }
}

And here is the WinForms to make it work. Leaving out IDE generated code.

ExploreTheHouseForm.cs

using System;
using System.Windows.Forms;

namespace House
{
    public partial class ExploreTheHouseForm : Form
    {
        Location currentLocation;

        RoomWithDoor livingRoom;
        RoomWithDoor kitchen;
        Room diningRoom;
        OutsideWithDoor frontYard;
        OutsideWithDoor backYard;
        Outside garden;

        public ExploreTheHouseForm()
        {
            InitializeComponent();
            CreateObjects();
            MoveToLocation(livingRoom);
        }

        private void CreateObjects()
        {
            // Configure the locations
            livingRoom = new RoomWithDoor("living room", "an antique carpet", "an oak door with a brass knob");
            kitchen = new RoomWithDoor("kitchen", "stainless steel appliances", "a screen door");
            diningRoom = new Room("dining room", "a crystal chandelier");
            frontYard = new OutsideWithDoor("front yard", false, livingRoom.DoorDescription);
            backYard = new OutsideWithDoor("back yard", true, kitchen.DoorDescription);
            garden = new Outside("garden", false);

            // Configure the exits
            livingRoom.Exits = new Location[] { diningRoom };
            kitchen.Exits = new Location[] { diningRoom };
            diningRoom.Exits = new Location[] { livingRoom, kitchen };
            frontYard.Exits = new Location[] { backYard, garden };
            backYard.Exits = new Location[] { frontYard, garden };
            garden.Exits = new Location[] { frontYard, backYard };

            // Configure exterior doors
            livingRoom.DoorLocation = frontYard;
            frontYard.DoorLocation = livingRoom;
            kitchen.DoorLocation = backYard;
            backYard.DoorLocation = kitchen;
        }

        private void MoveToLocation(Location location)
        {
            currentLocation = location;
            ExitsComboBox.Items.Clear();
            foreach (Location exit in location.Exits)
            {
                ExitsComboBox.Items.Add(exit.Name);
            }
            ExitsComboBox.SelectedIndex = 0;
            DescriptionTextBox.Text = currentLocation.Description;
            ShowGoThroughExteriorDoorButton(currentLocation);
        }

        private void ShowGoThroughExteriorDoorButton(Location location)
        {
            if (location is IHasExteriorDoor)
            {
                GoThroughExteriorDoorButton.Visible = true;
                return;
            }
            GoThroughExteriorDoorButton.Visible = false;
        }

        private void GoHereButton_Click(object sender, EventArgs e)
        {
            MoveToLocation(currentLocation.Exits[ExitsComboBox.SelectedIndex]);
        }

        private void GoThroughExteriorDoorButton_Click(object sender, EventArgs e)
        {
            IHasExteriorDoor locationWithExteriorDoor = currentLocation as IHasExteriorDoor;
            MoveToLocation(locationWithExteriorDoor.DoorLocation);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ The winforms screen gives the option to go through the exterior door, but whatever is behind the exterior door doesn't get listed in the available places? What happens if the kitchen had multiple exterior doors to multiple places? What if the entire house was just a kitchen? \$\endgroup\$ – Mast Jan 17 at 15:15
  • 4
    \$\begingroup\$ If you are interested in the general problem of how to write text-based adventures, I encourage you to look into Inform7, an exceedingly clever language for writing such adventures. If you're interested in learning about virtual machines for implementing such adventures, I did a series on my blog a few years ago about the Z-machine. \$\endgroup\$ – Eric Lippert Jan 17 at 22:21
  • 3
    \$\begingroup\$ As a bonus, on a Z-machine code for this problem will occupy less than a kilobyte. You won't be able to compile the above code into a .NET assembly that takes up that much. (This is another way of saying "in Ye Olde Days we didn't have no interfaces and abstract classes". However, we were also much more likely to be eaten by a grue, so it all evens out in the end.) \$\endgroup\$ – Jeroen Mostert Jan 18 at 13:36
9
\$\begingroup\$

RobH's review covers syntax and style well so I won't go into that. Instead I'd like to give my take on feedback given by Svek and BittermanAndy.

Separation Of Concerns

I think Svek's commentary about the CreateObjects method is spot on, but I don't think it goes far enough. The need for such a method in the first place hints that the ExploreTheHouseForm class is doing to much. With the current implementation, each of the rooms is a field on the form. This effectively makes the ExploreTheHouseForm the house itself. As such it would be more aptly named ExplorableHouseForm.

In general (and this becomes ever more important as you get into more complex projects) we want to separate the presentation of the data from the data itself.

The form is the UI it already has the duty to present the data to the user. It shouldn't also be the data. I would much rather the house be constructed elsewhere and passed to the form's constructor:

    public ExploreTheHouseForm(Location initialRoom)
    {
        InitializeComponent();
        MoveToLocation(initialRoom);
    }

With this simple change, you can remove all the individual Location fields from ExploreTheHouseForm with the exception of currentLocation. Also, if you desire, you can use the same form to explore any number of different houses without further modification.

Immutability

A fair amount of BittermanAndy's advice (as of the time of this writing, his post was updated at least once since I started) was to try to make your Location class immutable. Since with the overall design as it is, locations need to reference each other, you run into a chicken & egg scenario preventing immutability where each Location needs their neighbors to be created before them. I don't see a way around this, however if you have your locations implement an interface, and write your form to consume the interface rather than Location you can get much of the same benefit of actual immutability.

public interface ILocation
{
    public string Name { get; }
    public IList<ILocation> Exits {get;}
    public string Description { get;}
}

In ILocation we only specify the get portions of the properties. This means to consumers of ILocation the properties are effectively read-only even if the implementing classes implement the set. We also declare Exits as a collection of ILocation rather than Location so that accessed members are also read-only to consumers.

You don't have to change much about Location itself:

public abstract class Location: ILocation
{
    ...
    //private field to back Exits property. 
    private IList<ILocation> _exits;

    public IList<ILocation> Exits {
       get
       {
           // AsReadOnly so that consumers are not allowed to modify contents.
           // there are other ways of accomplishing this that may be better overall, but ExploreTheHouseForm accesses Exits by index so we can only change it so much. 
           return _exits?.AsReadOnly();
       }
       set{ _exits = value;}
    }
}

Updating ExploreTheHouseForm is also straight forward, simply change the type of the field currentLocation the Location parameters of ExploreTheHouseForm, MoveToLocation, and ShowGoThroughExteriorDoorButton to ILocation:

    ...
    private ILocation _currentLocation;
    ...
    public ExploreTheHouseForm(ILocation initialRoom)
    {
        InitializeComponent();
        MoveToLocation(initialRoom);
    }
    ...
    private void MoveToLocation(ILocation location)
    ... 
    private void ShowGoThroughExteriorDoorButton(ILocation location)

The overall impact of this is that the locations are mutable during construction (by some factory) but once construction is complete, all you work with the read-only ILocation

// GenerateHouse returns the entry point of the house.

public ILocation GenerateHouse()
{
    // Configure the locations
    var livingRoom = new RoomWithDoor("living room", "an antique carpet", "an oak door with a brass knob");
    var kitchen = new RoomWithDoor("kitchen", "stainless steel appliances", "a screen door");
    var diningRoom = new Room("dining room", "a crystal chandelier");
    var frontYard = new OutsideWithDoor("front yard", false, livingRoom.DoorDescription);
    var backYard = new OutsideWithDoor("back yard", true, kitchen.DoorDescription);
    var garden = new Outside("garden", false);

    // Configure the exits
    livingRoom.Exits = new List<ILocation>() { diningRoom };
    kitchen.Exits = new List<ILocation>() { diningRoom };
    diningRoom.Exits = new List<ILocation> { livingRoom, kitchen };
    frontYard.Exits = new List<ILocation> { backYard, garden };
    backYard.Exits = new List<ILocation> { frontYard, garden };
    garden.Exits = new List<ILocation> { frontYard, backYard };

    // Configure exterior doors
    livingRoom.DoorLocation = frontYard;
    frontYard.DoorLocation = livingRoom;
    kitchen.DoorLocation = backYard;
    backYard.DoorLocation = kitchen;
    // return entry point.
    return livingRoom;

}

Location Connectivity

I agree with the other reviews and comments that pulling the concept of the location connection into a separate class/set of classes would allow for a better design. Locations can have any number of exits, and it is a property of the exit, not the location if that exit is an open archway, a door, or just an abstract dividing line (outdoor location to outdoor location) Comintern does a good job of covering this in their review so I won't go into it further.

\$\endgroup\$
  • 1
    \$\begingroup\$ Excellent answer. Separating the creation of the house, and presenting the form only with the interface it needs for navigation, solves the creation / immutability problem very nicely. (My edits were fairly minor, by the way). \$\endgroup\$ – BittermanAndy Jan 18 at 8:26
  • \$\begingroup\$ "there are other ways of accomplishing this that may be better overall, but ExploreTheHouseForm accesses Exits by index so we can only change it so much.". At the very least I don't see any reason to not simply change the return value to IReadOnlyList which makes the intent much clearer. Admittedly the better fix for the consumer would be to use IImmutableList from the immutable collections package which really guarantees that the content of the list won't change. \$\endgroup\$ – Voo Jan 19 at 13:25
23
\$\begingroup\$

I have a general rule: if I see string concatenation with a loop I assume it's not the best way of doing it. Let's look at this:

foreach (Location exit in Exits)
{
    description += $"— {exit.Name}";
    if (exit != Exits[Exits.Length - 1])
    {
        description += "\r\n";
    }
}

I see this pattern a lot. What you need is Select with string.Join:

var exitList = string.Join(Environment.NewLine, Exits.Select(exit => $"— {exit.Name}"));

I've used Environment.NewLine because I like using well-named constants. Random aside: \r is a Carriage Return and \n is a Line Feed. The terminology comes from physical printers. Another reason I prefer Environment.NewLine is that it means you don't have to know and remember that.

Edit

A comment notes that Environment.NewLine is different on Windows vs Linux/Unix. I think that's important to know and I really should have mentioned it the first time. As the comment exchange shows, it's really easy to get platforms and line-endings mixed up which I think illustrates the usefulness of the constant.


The Name property of Location is only set in the constructor. You can use a read-only auto property instead of a private set one:

public string Name { get; }

Outside.Description could be using string interpolation like the other Description properties.


You're missing your access modifiers on your class declarations. MS guidelines state that they should always be specified.


I'm not aware of a convention for it but override public string seems to be an odd order to me. I would expect the access modifier first: public override string. The main thing is to keep that consistent though.

\$\endgroup\$
  • 2
    \$\begingroup\$ Environment.NewLine is better for working with Windows or Linux because it contains a different value depending on the OS. For Windows, Environment.NewLine is "\r\n" and on Linux, it is "\r", but by using Environment.NewLine you do not need to worry about those details. \$\endgroup\$ – Rick Davin Jan 17 at 15:26
  • 1
    \$\begingroup\$ @RickDavin - yes, that's true. I think that \r was old mac though, no? Unix/linux use \n AFAIK. \$\endgroup\$ – RobH Jan 17 at 15:34
  • \$\begingroup\$ My mistake. Yes Linux is "\n". See stackoverflow.com/questions/1015766/… \$\endgroup\$ – Rick Davin Jan 17 at 16:59
10
\$\begingroup\$

You've already made a decent effort to make your types immutable by setting its properties in a constructor and providing get-only access, not set - like you have with Location.Name for example. This is good practice whenever it is reasonable to do so (because, among other things, this means you can pass objects around without ever worrying that something else will change their state unexpectedly). However, I note that Location.Exits is a public field, which means it could be replaced with another array while the program is running - but the house is supposed to be fixed in structure. It would be better as a get-only public property, passed as another parameter in the constructor.

More subtly, not only could someone do something like currentLocation.Exits = ..., which the above advice would prevent; someone could also do currentLocation.Exits[0] = ..., and again they are changing something that is supposed to be fixed. (When I say "someone", understand that that could mean "you, by mistake", especially in a larger more complex program). Since you mentioned you have been learning about interfaces, the public get accessor for Location.Exits should be an IEnumerable<Location>, which lets things enumerate through the exits array to see what they are, but not change them. (If you've not used generics yet, don't worry too much about this for now).

So it would end up like this:

abstract class Location
{
    public Location(string name, Location[] exits)
    {
        Name = name;
        _exits = exits;
    }

    public string Name { get; }

    private Location[] _exits;
    public IEnumerable<Location> Exits => _exits;

    // ...
}

(Secretly, I'm not super-happy about passing in exits as an array, either. Again, someone could change that array after creating the Location. I'd rather pass in exits as an IEnumerable<Location> and copy them into a new array or other container type private to the Location. But that raises some design and object ownership questions that aren't too relevant here so let's not worry about that).


Digging deeper on locations/exits - a couple of things here, which are probably too large a change to worry about for this exercise, but something to think about in future.

Firstly, OutsideWithDoor inherits from Outside and implements the IHasExteriorDoor interface. This works for your purposes, but means the question of whether or not an exit from one location to the next has a door depends on the type of the locations, whereas logically speaking it's a property of the connection between them. (It's also limited to only one door per location, and a few other tricky bits - prefer to avoid unnecessary inheritance, and prefer composition over inheritance). So, I would suggest a LocationConnection type, where Locations are joined by LocationConnections rather than directly to other Locations, and a LocationConnection can either have a door or not (a boolean property).

Secondly, Location exits are bi-directional, that is, if you can go from one location to another, you can always go back, too. That makes sense (if you go from the kitchen to the dining room, it'd be very odd to be unable to go back to the kitchen!) but depends on your initialization code always getting that right, which is a common source of bugs. (What if the building was a mansion, with a hundred rooms!?) This problem could go away if that LocationConnection type were well-implemented, someone could travel along it in either direction and it only needs to be coded once. Something to bear in mind in future: whenever you have to remember to write BA if you write AB, someone's going to forget to do it. Make their (your) lives easier!

Introducing that new type may be a bigger change than is really justified for this code review, but it could solve a couple of potential problems.


A couple of very minor comments on ShowGoThroughExteriorDoorButton. Firstly, the name of the method is OK, but it sounds like it's always going to show that button. I'd call it ShowOrHide..., though that's just my personal preference. Also, the if-statement in that method is a bit inelegant. I'd write simply:

GoThroughExteriorDoorButton.Visible = (location is IHasExteriorDoor);

...which gets rid of those naked true and false values, and also gets rid of that ugly return halfway through the method. In general, prefer methods to have a single point of exit, at the end, though this isn't always practical, especially when you start to use exceptions.


Always specify access modifiers, especially private for all those fields on the ExploreTheHouseForm. Also, a common convention for private fields is to prefix them with an underscore, e.g. private Location _currentLocation;, though this is not universally followed - I like it because it helps make obvious what's a parameter or local variable, and what's a member variable.

\$\endgroup\$
  • 2
    \$\begingroup\$ The desire to have the Location immutable and take the exits in the constructor is great, but it seems incompatible with two-way travel. The kitchen has an exit to the dining room, and the dining room has an exit to to the kitchen. In order to follow your implementation advice, both would have to be created before the other. The proposed LocationConnection class seems to have the same chicken and egg problem preventing immutability. It will have to reference the locations it connects, and they will have to reference it. How would you resolve this conflict? \$\endgroup\$ – Mr.Mindor Jan 17 at 21:45
  • 2
    \$\begingroup\$ Fair point. One possible solution would be to create the Locations without exit information, then create a list/LUT of LocationConnections. The Location then wouldn't itself hold references to its exits - every time it wanted to know, it would have to check the LocationConnection list. Is that better or worse than just allowing the Location class to be mutable? It becomes a judgement call at that point and depends on how the classes will be used. Mutable is great "whenever it is reasonable to do so"... it isn't always. \$\endgroup\$ – BittermanAndy Jan 17 at 22:34
  • 1
    \$\begingroup\$ Very interesting answer, thank you! I'll be taking this into consideration in the future! \$\endgroup\$ – Phrancis Jan 18 at 2:36
  • \$\begingroup\$ Comintern's (good) answer provides some examples of what the exit/connection types might look like. \$\endgroup\$ – BittermanAndy Jan 18 at 8:21
  • 1
    \$\begingroup\$ @MrMindor The usual way around this (apart from redesigning to avoid circular references) is to use a builder pattern during creation which is mutable and generate immutable classes from that one. \$\endgroup\$ – Voo Jan 19 at 13:08
7
\$\begingroup\$

The other reviews covered many of the main points I'd raise, but there are a handful of others.


Constructors in abstract classes should be protected, not public:

public Location(string name) => Name = name;    

You can't create a new instance of the abstract class, so it is for all intents and purposes protected anyway. Make the access modifier match the behavior.


I'm not sure that I like some of the naming. For example, CreateObjects() gives only the slightest clue as to what it is doing. I'd probably go with something more along the lines of GenerateMap(). A couple of the member names are also a little ambiguous as to how they function - for example, which room is IHasExteriorDoor.DoorLocation relative to?


Speaking of doors (keeping in mind that I'm not sure how much was proscribed by the exercise), I also think I find it a bit more natural to have the exits be the primary motive object. It's more natural to me to think of "using a door" than it is to think of "leaving a room". To me, a room is a static thing - it has exits, but you don't really use a room by leaving it. I'd consider building your map from the standpoint of the connections between the locations instead of the locations themselves. Something more like this:

public interface ILocation
{
    string Name { get; }
    string Description { get; }
}

public interface IExit
{
    // TODO: my naming sucks too.
    ILocation LocationOne { get; }
    ILocation LocationTwo { get; }
    // Takes the room you're exiting as a parameter, returns where you end up.
    ILocation Traverse(ILocation from);
}

public abstract class Location : ILocation
{
    private readonly IReadOnlyList<IExit> _exits;

    protected Location(string name, string description, IEnumerable<IExit> exits)
    {
        _exits = exits.ToList();
    }

    public IEnumerable<IExit> Exits => _exits;

    // ...other properties...
}

That lets you concentrate on the spatial relationships between the locations from a more natural direction (IMO and no pun intended). You'll likely find this to be more easily extensible down the road when you need to, say close or lock a door:

public interface Door : IExit
{
    bool IsOpen { get; }
    bool IsLocked { get; }
}
\$\endgroup\$
  • \$\begingroup\$ Why do you have both a ILocation interface and a Location abstract class? Since you have the Location abstract class I'd prefer to move Name and Description to Location as abstract properties. \$\endgroup\$ – inwenis Feb 10 at 20:18
  • 1
    \$\begingroup\$ @inwenis That would fall under the // ...other properties... comment. The example Location was intended to highlight the Exits. \$\endgroup\$ – Comintern Feb 10 at 20:34
  • \$\begingroup\$ I've read other reviwes and seems the ILocation can be used to pass locations as immutable objects. It's nice but without any comments I would recommennd mering them. OK, maybe now after I have seen what it can be used for I would have my doubts. \$\endgroup\$ – inwenis Feb 10 at 21:38
6
\$\begingroup\$

In this code block here:

...
OutsideWithDoor backYard;
Outside garden;

public ExploreTheHouseForm()
{
    InitializeComponent();
    CreateObjects(); // <--- bleh
    MoveToLocation(livingRoom);
}

This is call to the CreateObject() method, is something I don't like to see in code (it could be a personal style issue) but if you are constructing an object, then all code related to the construction of an object should just stay in the constructor...

I would prefer that it ended up looking like

...
private readonly OutsideWithDoor _backYard;  // now it can be readonly
private readonly Outside _garden;

public ExploreTheHouseForm()
{
    InitializeComponent();

    ...
    _backYard = new OutsideWithDoor("back yard", true, kitchen.DoorDescription);
    _garden = new Outside("garden", false);

    MoveToLocation(livingRoom);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ "all code related to the construction of an object should just stay in the constructor" I have the disagree with that. If it's a one-liner? Do it inline - no point creating helper methods. But for more complex initializations separating logically independent parts into their own methods keeps methods small, simple and self-documenting. Otherwise we end up with a giant constructor method that requires comments to separate the different parts. Now you certainly do want to keep things readonly, but the usual solution is to simply return the created object and assign it in the constructor. \$\endgroup\$ – Voo Jan 19 at 13:20

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.