Brtdku

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.

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.

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

}

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

}

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.