ryansworld10/Escape — pull request #3 by whisperity (2013-11-10 18:56:58+00:00)
Title: Purify the way the world is handled, make stuff more OO

(I've seen your project on Facepunch and have to say it's a wonderful things. But it has some errors I'd like to fix for you.)

## Statically typed items
This was the first which caught my attention. Design principles dictate that this is a bad approach, and if it is left this way, when you reach the point that your world contains lots of elements, you'll only run into errors.

You have an `Item` class, which defines the properties and methods of a particular item. There is no need to create static definitions for each and every one. Instead, **instances** of `Item` should be used:

```c#
Item some_item = new Item();
some_item.Name = "Some item";
some_item.Description = "This is an item.";
some_item.SomeVariable = SomeValue;
// et. al.
```

So with this approach, I scruffed the types and made every item like this. You might think: _What happened to my implementation of how the items are used?_ This is where **events** come to the picture.

(I've read that you're a beginner and started this to learn (which is rather admirable :beers:), so I try to explain everything I think need to be explained.)

C# supports events and event-based execution. (Official documentation [here](http://msdn.microsoft.com/en-us/library/aa645739%28v=vs.71%29.aspx) if you wish.) This means, that using `delegate`s, you can add methods to an event-type property which get fired when the event is raised. See the example below:

```c#
public class EventHost
{
public delegate void ItDoesSomething();

public event ItDoesSomething RegisteredEvents;

public void Fire()
{
this.RegisteredEvents();
}
}

public class Program
{
public static void Main(string[] args)
{
EventHost eventcontainer = new EventHost();

eventcontainer.RegisteredEvents += new EventHost.ItDoesSomething(one);
eventcontainer.RegisteredEvents += new EventHost.ItDoesSomething(two);

eventcontainer.Fire();
}

static void one()
{
// Do something
}

static void two()
{
// Do something else
}
}
```

`one` and `two` gets executed when `Fire()` is called on the container. Of course, multiple instances of event containers can exist, and on the delegate (which is a type), you can specify arguments and different return types. I used this approach to specify what happens when an item is used.

> Note: in my commit, I've went a step further and used _anonymous_ delegates, which are simple in-line methods without a designated function name. They have the same functionality but create no needless method signatures in the IDE's method list. (They do have compiler-generated signatures in the build, though.)

This changed the was the item generator looks in `World.cs`. But there were more problems.

## Gobbled parameter list for constructors
### Or simply: overparameterized constructors

You set properties in bloated constructors. This again, while plausible and working, can lead to confusion and errors later on when the project is refactored or expanded.

Compare these two:
```c#
Map.Add(new Location(
"Room 1",
"This is a room.",
new List() { "room 2" },
new List() { "brass key", "rock" }));
```
```c#
Location room1 = new Location("Room 1");
room1.Description = "This is a room.";
room1.Exits.Add("Room 2");
room1.Items.Add("Brass Key"),
room1.Items.Add("Rock");
Map.Add(room1);
```

More lines, maybe, but a lot more easily readable and understandable (interpret as: debuggable) code. And there is no real difference between the two after compiling. Only, that you, the developer, can quickly realize what he is reading or modifying, instead of having to look at documentation or the constructor's code directly. It saves time, makes the code more readable and more comforting to the object-oriented ~~standards~~ conventions.

## Reference types
After those above were done, I saw that you are using a lot of extra methods because you store entries in simple generic lists. So there is a lot to convert between names and IDs, lookups and administration. Administration which makes the code slower to run, slower to debug, slower and harder to contribute to. And, we violate [DRY](http://en.wikipedia.org/wiki/Don%27t_repeat_yourself).

I have created a wrapper collection exactly for this purpose, which incorporates storing stuff in lists, indexed **both** with names and IDs. In commit [`d9a011d`](https://github.com/whisperity/Escape/commit/d9a011d6e65736362e2361a5aea4db805beeb3b0), this was implemented.

C# is object-oriented. The code, literally **everywhere** (sometimes to an annoying level for someone coming from non-OOP languages) relies on objects. And objects are `reference` types. It means, that you don't need to calculate IDs, pass those IDs around and then track back what object the ID belongs to, you can pass the object as it was. And even if you copy an object from a list to another, it does **not** create a new object, only its reference (the pointer) happens to be at two places.

The previous way in which you stated, for example, the exits of a location by name and after the generation, you had it converted to IDs (and in the same approach: every time you needed a location's data, it had to be looked up) and stored it that way, was dropped also. Instead, I made it so that we first store the names (and blank instances) of every world entity, and then set up their properties.

And the last step was that after changing this "middleware", the last step was to :twisted_rightwards_arrows: align the rest of the code to use this middleware.

> This sums up the five commits I'm pull requesting.

#### Where am I going with all of this?
This might seem to be a lot of bungling into someone else's project, but the problem with leaking designs is that once a project is started with them, they are here to stay. Forever. I have a saying that
> You cannot cure the illness or hide away the symptoms if the idea itself is diseased.
Pretty fitting for this situation, I guess.

I think a clean code is good for everyone, but most importantly, the core developer(s). After these changes, --as much as I was able to test-- the program works the same way as it did before.

Please take a look at the commits I am sending to you and understand what I changed. If anywhere you don't understand what something does or why I did it that way, feel free to put a line note on the commit and **ask**. Ask! There is no use in blindly accepting or denying me.

If everything is understandable or I've already talked too much, sorry. :confused: