Hey everyone,

I am trying to make sure I am writing my classes in the best manner possible (program to an interface not an implementation, using good oop practices, open-closed principle, etc). I believe I am on the right track with that I am doing, but I have question as to a specific part in my class, as I feel like I will be writing duplicate code between classes.

I have a class called MissionEncounter which implemented the IEncounter interface. The code is below.

public class MissionEncounter : IEncounter<Mission>
{
    private RandomizedList<Mission> missions = new RandomizedList<Mission>();
    private string path = "Assets/Resources/XML Files/Activities/Missions";

    public MissionEncounter()
    {
        string[] files = Reader.GetXmlFiles(path);
        Create(files, missions);
        missions.Shuffle();
    }

    public Mission GetNext()
    {
        return missions.Next();
    }

    private void Create(string[] files, RandomizedList<Mission> list)
    {
        for (int i = 0; i < files.Length; i++)
        {
            //...use XML to retrieve Encounter(Mission in this case) specific information
            //...create a new Mission(...)
            //...add new Mission to the missions list
        }
    }
}

public interface IEncounter<T>
{
    T GetNext();
    void Create(string[] files, RandomizedList<T> list);
}

I will have multiple types of encounters in my game, some are missions, some are shops, random loot, neutral/hostile ships, and more. I was thinking I had to create a new ________Encounter class for each type of encounter. (LootEncounter, ShopEncounter, etc). The reason for this is that each encounter types details are defined by an XML file structure and there are multiples of each (100 missions, 3 shops, etc). Using the XML format allows me to easily add new content without having to write new code - unless I want to create a totally new encounter type.

What is bugging me is that the _______Encounter class would all be duplicates of each other except for the code inside the method that creates the encounter type. Create(string[] files, RandomizedList<T> list).

I was trying to stay away from inheritance as much as possible, but would it be acceptable in this situation to have MissionEncounter inherit from some type of generic base class that implements IEncounter so that it takes care of class construction, the GetNext() method, and implements an overridable Create(..) method?

It seems to me like you'd want to move your mission creation to an entirely separate factory class that knows how to parse the XML data for each individual mission type and construct them by passing the relevant data directly to their constructors. That way missions know nothing about XML or how they came to be, just that they now exist and have appropriate data.

This will also allow you to remove the Create() function from your IEncounter interface, which doesn't really seem to belong there. I inferred that "encounters" handle mission sequencing from the GetNext() method, but also having them create the missions gives them multiple responsibilities and violates SRP.

@Zipster I think some confusion is a product of bad class naming on my part. The MissionEncounter class would be considered the factory for loading all the mission xml data into a list of missions and then handing one out when requested. There is an actual Mission class which only houses the data for that mission, title text, description, messages, choices, etc. Same with the shop, loot, and ship classes.

I would probably do something along the lines of this:

public class Encounter : IEncounter
{
    private RandomizedList<Encounterable> list = new RandomizedList<Encounterable>();
    private EncounterStrategy strategy;

    public Encounter(EncounterStrategy strategy)
    {
        string[] files = Reader.GetXmlFiles(strategy.getPath());
        this.strategy = strategy;
        Create(files, list);
        list.Shuffle();
    }

    public Encounterable GetNext()
    {
        return list.Next();
    }

    private void Create(string[] files, RandomizedList<Encounterable> list)
    {
        for (int i = 0; i < files.Length; i++)
        {
            //...use XML to retrieve Encounter(Mission in this case) specific information
            //the strategy creates the proper Encounterable.
            //add Encounterable to list.
        }
    }
}

public interface EncounterStrategy
{
    string getPath();
    //other methods required to create encounterable.
}

This way all of the unique functionality is in one place (MissionEncounterStrategy, LootEncounterStrategy, etc.) instead of forcing you to inherit and keep overriding create methods.

EDIT: lots of little code errors.

@SeraphLance

Interesting concept there, but I am left wondering something. Each encounterable will have differing data. Missions (title, mission text, choices, and outcomes) differs from a Shop encounterable (dealer name, type of items they sell, stock). So the lines of code in the create method for the XML portion would be drastically different as they are targeting different elements and attributes.

I'm leaning towards SiCranes response, about having the Create method choose the XML method to use to grab the information out of the file. I'm just worried I'll end up creating smelly switch statements. I understand if used properly (once and only once for a certain procedure) they are fine, but I typically over use them in some areas and they've been a pain in the butt to modify when I've had major additions, and the fact I haven't been able to grasp the polymorphic alternative to switch statements. Most examples are so simple I have a hard time translating how to apply them to my code.

It's hard to decide how to split the responsibilities of your classes. Try making a domain model (http://en.wikipedia.org/wiki/Domain_model) to define your dictionary and find relation between your abstractions, and post it.

@SeraphLance

Interesting concept there, but I am left wondering something. Each encounterable will have differing data. Missions (title, mission text, choices, and outcomes) differs from a Shop encounterable (dealer name, type of items they sell, stock). So the lines of code in the create method for the XML portion would be drastically different as they are targeting different elements and attributes.

I'm leaning towards SiCranes response, about having the Create method choose the XML method to use to grab the information out of the file. I'm just worried I'll end up creating smelly switch statements. I understand if used properly (once and only once for a certain procedure) they are fine, but I typically over use them in some areas and they've been a pain in the butt to modify when I've had major additions, and the fact I haven't been able to grasp the polymorphic alternative to switch statements. Most examples are so simple I have a hard time translating how to apply them to my code.

That's what the strategy is for. The strategy implementation stores a path and knows how to read the files from that path to create encounterables. One "could" argue that it's a violation of the SRP to do both, but I'd say they're sufficiently connected tasks. The strategy pattern basically is the polymorphic alternative to switch statements.

It's worth noting that the whole strategy.getPath() thing is kind of stupid, but I put it in there as a sort of minimum-modification example. Really, the create function in this case would just have the strategy handle all that.

If I understand the code correctly, I am seeing 2 problems here. The Encounter class does too many things and Mission is only vaguely defined.

Encounter currently handles Mission creation and Mission enumeration.

You are saying, there will be different kind of Missions with different data. Would it be possible to make Mission a base class and extend from there different Mission types? Maybe it would even be possible to model all the different scenarious by making Mission a container for "MissionObjectives".

Then you can move the creation logic out of the Encounter class and create a list of Mission objects which are filled by factories based on the xml supplied.