4

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
     }
}
  • do you want a new instance of the "Rules" every time this method is called? – Caleb May 12 '15 at 9:50
  • 2
    I think your rules are stateless, create them on startup or use singletones and just return the list. – Aitch May 12 '15 at 9:51
  • @Caleb No need of new instance everytime. I could reuse the same. – Bhaskar K May 12 '15 at 9:54
  • 5
    NOOOO! Don't do it! Please don't try a singleton, for they are subtle and quick to anger! In all seriousness 99% of the time when you think you need a singleton YOU ACTUALLY DO NOT NEED A SINGLETON. There are lots of resources out there that show What's so bad about Singletons – Binary Worrier May 12 '15 at 10:21
  • 1
    Thank you BindayWorrier. Actually I want the code to be testable. I would create an interface and inject into the class where its required. Could you post it as answer, so that it will useful in future? – Bhaskar K May 12 '15 at 10:37
2

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

  • Ok, I am confused about Provider pattern here. I thought I would just go ahead create an interface with GetRules method and a class RuleProvider implementing it. Inject the interface wherever its required. Are you asking me to implement kind of this duanewingett.info/2014/05/22/CSimpleProviderPattern.aspx – Bhaskar K May 12 '15 at 11:05
  • another point for premature optimization. the OP wants to have a list returned and you're talking about caching. I don't want to know how your 'hello world'-programs looks like :D. – Aitch May 12 '15 at 11:23
  • @BhaskarK: I'm talking about a simple "Provider" which simply "provides" the rules for the rest of the application to use. I quickly scanned that article, lots of static classes etc, no that's not what I meant. – Binary Worrier May 12 '15 at 12:32
  • @Aitch: There was lots of thoughts in early answers & comments about performance & the fact the list of Rules was build once and then used through out the program. This answer started off as a comment in response to a comment along the lines of "Maybe a singleton would work here" – Binary Worrier May 12 '15 at 12:34
  • Aargh...why does everyone say it is difficult to implement singleton. While there's seldom a need, but if you do decide that singleton is the way to go then you create it at startup. Done. No difficulties. Well...unless you go singleton "happy" and singleton's start needing other singletons. However, if your design requires that then you should probably be thinking of switching to a different career because software design is obviously not your strong suit. – Dunk May 12 '15 at 19:06
3

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.

  • 1
    While this approach avoids building up the list on every single and respects the demand for an IList<T> call I'd consider it sub-optimal. You are - implicitly - exposing implementation details via the public interface. Lists are mutable and any consumer of your class could to anything with the reference to your private fields you're exposing - i.e. clear, remove, add and the like. Consider using AsReadOnly and return an IEnumerable<T> to prevent these issues. – Paul Kertscher May 12 '15 at 10:19
  • Thanks Caleb, shouldn't the class be static? – Bhaskar K May 12 '15 at 10:20
  • @Bhaskar never mind. Anyway, yes the class could be static, what does not mean that it should be. What Binary Worrier wrote about singletons does apply to static classes either. They are hard to trace and create hidden dependencies – Paul Kertscher May 12 '15 at 10:25
  • That should be the correct answer: simple, readable and enough for what the OP wants! – Aitch May 12 '15 at 11:26
1

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

  • Programmers is about conceptual questions and answers are expected to explain things. Throwing code dumps instead of explanation is like copying code from IDE to whiteboard: it may look familiar and even sometimes be understandable, but it feels weird... just weird. Whiteboard doesn't have compiler – gnat May 13 '15 at 9:34
  • 1
    edited to expand on the reasoning – Ewan May 13 '15 at 9:48
0

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.

  • I've seen this approach in a coworkers codebase lately and I still do not get the point. Why should I prefer yield return over - say - an array initializer? – Paul Kertscher May 12 '15 at 10:09
  • The goal of this function is not to create an array/list - the goal is to get the rules in order. Using a lazy enumerator is the least restrictive way to do so, because you are not forcing the creation of all the rules and the container that holds them. This means that if someone just wants to iterate over the rules with foreach, they can do so and GetRules will create the rules one by one when the user needs them. If they only need some of the rules, GetRules won't have to create all of them. And if they really need a contianer, they can always use .ToArray() or .ToList(). – Idan Arye May 12 '15 at 14:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.