Efficient strucure for backing data in MVVM

Question

I started learning C# a few months ago (so keep in mind that this is all fairly new to me) and I am working on my first real project (WPF / MVVM). I've received conflicting advice on how to handle my backing data. Currently I have everything held in a singleton class (DataManager); some say that's okay, but others say I should use a different design. So I've come here looking for suggestions on how to do this in an efficient way that will serve me well as the project progresses.

For the sake of getting the best possible answer, I'm going to post a good bit of my code. Everything is very basic at the moment; I just want to get a good foundation before I go too much further. Once that happens I'll make this more elaborate. I don't think it's relevant, so I've decided not to post the ViewModel code. If you would like to see it, let me know. So...

My DataManager class, where master lists of all major types are contained (my ViewModel's properties reference the data contained in this class):

class DataManager
{
    private static DataManager _data;
    private ObservableCollection<Adventurer> _adventurers = new ObservableCollection<Adventurer>();
    private ObservableCollection<Expedition> _expeditions = new ObservableCollection<Expedition>();
    private ObservableCollection<Guild> _guilds = new ObservableCollection<Guild>();
    private ObservableCollection<Worker> _workers = new ObservableCollection<Worker>();

    public static DataManager Data
    {
        get
        {
            if (_data == null)
            {
                _data = new DataManager();
            }
            return _data;
        }
    }

    public Date GameDate { get; set; }

    public ObservableCollection<Adventurer> Adventurers
    {
        get { return _adventurers; }
    }

    public ObservableCollection<Expedition> Expeditions
    {
        get { return _expeditions; }
    }

    public ObservableCollection<Guild> Guilds
    {
        get { return _guilds; }
    }

    public ObservableCollection<Worker> Workers
    {
        get { return _workers; }
    }
}

My models for Workers and Adventuers:

interface IPerson
{
    Gender Gender { get; set; }

    string FirstName { get; set; }

    string LastName { get; set; }
}

class Adventurer : IPerson
{
    public Adventurer()
    {
        CreateAdventurer();
    }

    public Gender Gender { get; set; }

    public Guild Employer { get; set; }

    public int Defense { get; set; }

    public int ID { get; set; }

    public int Salary { get; set; }

    public int Strength { get; set; }

    public string FirstName { get; set; }

    public string LastName { get; set; }

    private void CreateAdventurer()
    {
        this.Gender = ModelUtilities.RandomGender();
        Employer = null;
        Defense = ModelUtilities._rand.Next(21);
        ID = DataManager.Data.Adventurers.Count;
        Salary = ModelUtilities._rand.Next(21);
        Strength = ModelUtilities._rand.Next(21);
        ModelUtilities.RandomName(this);
    }
}

class Worker : ObservableObject, IPerson
{
    public Worker()
    {
        CreateWorker();
    }

    public Gender Gender { get; set; }

    public Guild Employer { get; set; }

    public int ID { get; set; }

    public int Salary { get; set; }

    public int Skill { get; set; }

    public string FirstName { get; set; }

    public string LastName { get; set; }

    private void CreateWorker()
    {
        this.Gender = ModelUtilities.RandomGender();
        Employer = null;
        ID = DataManager.Data.Workers.Count;
        Salary = ModelUtilities._rand.Next(21);
        Skill = ModelUtilities._rand.Next(21);
        ModelUtilities.RandomName(this);
    } 
}

My Guild model:

class Guild
{
    public Guild()
    {
        CreateGuild();
        FilterAdventurers();
        FilterWorkers();
    }

    public ICollectionView Adventurers { get; set; }

    public ICollectionView Workers { get; set; }

    public int Gold { get; set; }

    public int ID { get; set; }

    public string Name { get; set; }

    private void CreateGuild()
    {
        Gold = 5000;
        ID = DataManager.Data.Guilds.Count;
        Name = "Guild1";

        Adventurers = new CollectionViewSource { Source = DataManager.Data.Adventurers }.View;
        Workers = new CollectionViewSource { Source = DataManager.Data.Workers }.View;
    }

    private void FilterAdventurers()
    {
        Adventurers.Filter = a =>
            {
                Adventurer adventurer = a as Adventurer;
                if (a == null)
                    return false;
                return adventurer.Employer == this;
            };
    }

    private void FilterWorkers()
    {
        Workers.Filter = w =>
            {
                Worker worker = w as Worker;
                if (w == null)
                    return false;
                return worker.Employer == this;
            };
    }
}

My Expedition model:

class Expedition
{
    public Expedition()
    {
        CreateExpedition();
    }

    public bool Available { get; set; }

    public Guild Owner { get; set; }

    public int Difficulty { get; set; }

    public int ID { get; set; }

    public int Reward { get; set; }

    public string Name { get; set; }

    private void CreateExpedition()
    {
        Available = true;
        Owner = null;
        Difficulty = ModelUtilities._rand.Next(21);
        ID = DataManager.Data.Expeditions.Count;
        Reward = ModelUtilities._rand.Next(21);
        Name = string.Format("Expedition {0}", ID);
    }
}

And last, the class that initializes all of the data:

static class Initialize
{
    public static void NewGame()
    {
        DataManager.Data.GameDate = new Date(1, 0);

        DataManager.Data.Guilds.Add(new Guild());

        for (int i = 0; i < 10; i++)
        {
            DataManager.Data.Adventurers.Add(new Adventurer());
        }

        for (int i = 0; i < 10; i++)
        {
            DataManager.Data.Workers.Add(new Worker());
        }

        for (int i = 0; i < 10; i++)
        {
            DataManager.Data.Expeditions.Add(new Expedition());
        }
    }
}

So this is how the data is currently being handled. As I said, I've had some people say that what I'm doing is okay and I've had others tell me to overhaul it. Right now it works, but I'm very interested in knowing what an experienced programmer would suggest I do to handle my backing data to make it more efficient/practical in the long term.

As a novice programmer, I know that I have a lot to learn, and I want to learn everything that I can. That said, any advice you could give me would be greatly appreciated. Thanks.

Answer 1

Following the lengthy commentary on the question, there's one additional point that qualifies as an actual 'answer'. Your classes are built up using the composition model which is fine. But since an Adventurer contains an instance of Guild, what happens to the Guild instance if the Adventurer is destroyed? So you'll need to test for (or be aware of) unintended side-effects that may cause an unpredictable state.

Along another line, consider this scaled-down implementation of Guild...

public class Guild
{
    public string Name { get; set; }
}

And here's a method to run using it...

    public void Scenario1()
    {
        ObservableCollection<Guild> guilds = new ObservableCollection<Guild> ();
        for (int i = 0; i < 20; i++)
        {
            Guild g1 = new Guild { Name = "Meistersingers" }; 
            if (!guilds.Contains(g1))
            {
                guilds.Add(g1);
            }
        }
        int uniqueGuilds = guilds.ToList().Distinct().Count();
        Console.WriteLine(@"Unique guilds: " + uniqueGuilds);
    }

And the output is...

Unique guilds: 20

...because the 'Contains' method for collections uses the 'Equals' method on object. And object.Equals simply checks memory addresses. Lots of developers who are starting out expect 'Contains' to use value semantics, and thus start thinking that 'Contains' is buggy when their classes start to grow and use complex relationships.

Now for an alternative, look at this scaled-down version of the Guild class...

public class Guild2 : IEquatable<Guild2>, IComparable<Guild2>
{
    public string Name { get; set; }
    public bool Equals(Guild2 other)
    {
        return Name == other.Name;
    }
    public int CompareTo(Guild2 other)
    {
        return String.Compare(Name, other.Name, System.StringComparison.Ordinal);
    }
}

And against the same function as before...

public void Scenario2()
{
    ObservableCollection<Guild2> guilds = new ObservableCollection<Guild2>();
    for (int i = 0; i < 20; i++)
    {
        Guild2 g1 = new Guild2 { Name = "Meistersingers" };
        if (!guilds.Contains(g1))
        {
            guilds.Add(g1);
        }
    }
    int uniqueGuilds = guilds.ToList().Distinct().Count();
    Console.WriteLine(@"Unique guilds: " + uniqueGuilds);
}

And the output is...

Unique guilds: 1

...this is more in line with expectations. So consider inheriting from IEquatable and IComparable so as to avoid bugs when your library gets large and complex. Why? Because the collections 'Contains' method will look to see if you have implemented IEquatable and use it instead of the same method on 'object'. Polymorphism is your friend.

Lastly, the three or four 'CreateXXX' methods are better situated in an abstract factory pattern.