Design method to return List of objects

Question

I need to create a method which will return a List of IRule objects.

This is what I have written, am I correct in the approach? Or should I follow any patterns/principles?

public IList<IRule> GetRules()
{
    return new List<IRule> {
         new Rule1(),
         new Rule2(),
         ...etc
     }
}

Answer 1

It depends on how you need to use GetRules.
If it's expensive and and you really do want to access the same set of Rules everywhere in your application, then I'd create a Cache class which stores the list of rules.

In this instance (and almost all others) a Singleton would be an ant-pattern. Instead you would have an ICache interface with a GetRules method and pass an ICache reference to every object that needs it.

While this is more work than rolling it into a Singleton (it's actually surprisingly difficult to implement the Singleton pattern correctly), it gives you correct separation of concerns and allows you to swap out the mechanism behind the ICache interface. e.g. In time you find you need to refresh the cache every 8 hours. The Cache object can do that, but the client code doesn't need to change.
Or you find you need to implement two different types of caching for two different games/clients? You simply inject a different implementation of the cache.
You find in a years time that one part of the running app needs to see live rules in a database and can't work from the cache? You inject a different cache that isn't actually a cache but returns live rules each time it's called. You'd be in trouble with a singleton on that one.

That's one way you could do it. Have a Cache that returns rules, or an IRulesProvider that knows how to get rules.
Abstract the getting of rules, inject that abstraction into the classes that need to get rules, then implement that interface.
You can still choose to create only one instance of the class that implements IRulesProvider but that's up to you, you don't need all the crap that comes along with the Singleton pattern, half of which is there to protect yourself from creating more than on instance of the class.

You may by now have guessed I've butted heads with Singletons in the past . . . it has left some scars :)

Even if it's not expensive to create the objects
Still consider the Provider pattern. It give you great flexibility and proper separation of concerns. Things that use rules can get rules, but they don't even know what gets the rules. It's a realy powerful pattern to follow and leaves you and your code extremely agile.

An example of what I mean by a provider pattern.

interface IRulesProvider
{
    IEnumerable<IRule> GetRules();
}

class RulesProvider: IRulesProvider
{
    IEnumerable<IRule> GetRules()
    {
        // return some rules;
    }
}

Are the rules cached on the RulesProvider object? are they fetched from an Xml file and cached on it, are they read live from a database each time GetRules is called? Thats up to you, that class you inject an IRulesProvider into neither knows nor care how the provider comes by the rules (unless of course it needs to know, at which point we need a different mechanism).

Answer 2

Going off your comment, where you do not need a new list every time. It would be better to build up the list once, and return the same "singleton" list in that function.

public class RuleProvider
{
    private readonly List<IRule> _theRules = 
        new List<IRule>{
         new Rule1(new Writer1()),
         new Rule2(new Writer2())
        };

    Public IList<IRule> GetRules()
    {
        return _theRules;
    }
}

Depending on how you want this list to be used it would be worth looking into:
1) Lazy Loading - defer the "intial" load until someone needs to use the Rules list.
2) Read-only collection - so that users of your "rules" cannot modify the list.

Answer 3

Consider the code:

foreach (Object o in Items)
{
    foreach(Rule r in GetRules())
    {
        //apply rule
    }
}

This will create multiple copies of your list of objects. these will eventually be collected by the garbage collector. but in the mean time they eat memory.

Now if you called it "CreateRules()" or added some xml comment, you are less likely to do this by accident. But its still somewhat risky and begs the question of what you are attempting to achieve.

Be careful about falling the other way and providing access to a mutable list, where you don't want changes in one list of GetRules() to affect a second call

Answer 4

Why not use yield?

 public System.Collections.Generic.IEnumerable<IRule> GetRules()
 {

      yield return new Rule1(new Writer1());
      yield return new Rule2(new Writer2());
      //...etc

 }

The user of the function will decide if they want all the rules at once or to get them lazily.

Update

The other solutions have mentioned caching - if you need/want caching, this solution won't work.