Vtgyjfy

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.

The app looks like this:

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: rn";

                foreach (Location exit in Exits)

                {

                    description += $"— {exit.Name}";

                    if (exit != Exits[Exits.Length - 1])

                    {

                        description += "rn";

                    }

                }

                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}rn 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);

        }

    }

}

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.

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 += "rn";

    }

}

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.