Strategy pattern using an abstract class and an interface

Question

I have implemented some sort of the strategy pattern using an abstract class and an interface:

public interface ICombination
{
    void Check(int[] hand, UsersProperties user);
    void UpdateHand(UsersProperties user, int combinationType, int powerType);
}

public abstract class ICombinationAnalyzer : CardVariables, ICombination
{
    public abstract void Check(int[] hand, UsersProperties user);

    public void UpdateHand(UsersProperties user, int combinationType, int powerType)
    {
        user.Type = combinationType;
        user.Power = powerType + user.Type*100;
        MainPoker.Win.Add(new Hand {Power = user.Power, Current = user.Type});
    }

    protected ICombinationAnalyzer(UsersProperties user, IReadOnlyList<int> hand) : base(user, hand)
    {

    }
}

CardVariables is just a class where I keep some variables used in the calculates down below. I don't think there's point in showing it here.

There are a few derived methods I'm not going to show entirely here because they are quite similar to one another.

public class RoyalFlush : ICombinationAnalyzer
{
    public RoyalFlush(UsersProperties user, IReadOnlyList<int> hand) : base(user, hand)
    {
    }

    public override void Check(int[] hand, UsersProperties user)
    {
        const int localType = (int) Hand.Combinations.RoyalFlush;
        foreach (
            var t in
                CombinedArrays.Where(t => t.Length >= 5)
                    .Where(
                        t =>
                            t[0] == (int) Cards.CardTypes.Ace && t.Contains((int) Cards.CardTypes.Ten) &&
                            t.Contains((int) Cards.CardTypes.Jack) && t.Contains((int) Cards.CardTypes.Queen) &&
                            t.Contains((int) Cards.CardTypes.King)))
        {
            UpdateHand(user, localType, t.Max()/4);
        }
    }
}

And here's one more:

public class Straight : ICombinationAnalyzer
{
    public Straight(UsersProperties user, IReadOnlyList<int> hand) : base(user, hand)
    {
    }

    public override void Check(int[] hand, UsersProperties user)
    {
        const int localType = (int) Hand.Combinations.Straight;
        int[] op = Straight.Select(o => o/4).Distinct().ToArray();
        for (int j = 0; j < op.Length - 4; j++)
        {
            if (op[j] + 1 == op[j + 1] && op[j] + 2 == op[j + 2] && op[j] + 3 == op[j + 3] && op[j] + 4 == op[j + 4])
            {
                UpdateHand(user, localType, op.Max() - 4 == op[j] ? op.Max() : op[j + 4]);
            }
            if (op[j] != (int) Cards.CardTypes.Ace || op[j + 1] != (int) Cards.CardTypes.Ten ||
                op[j + 2] != (int) Cards.CardTypes.Jack || op[j + 3] != (int) Cards.CardTypes.Queen ||
                op[j + 4] != (int) Cards.CardTypes.King) continue;

            UpdateHand(user, localType, 13);
        }
    }
}

Now the problem is where I'm actually implementing the strategy pattern:

List<SwitchCombination> listCombinations = new List<SwitchCombination>();
                var switchCombination = new SwitchCombination( new HighCard(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new PairFromTable(user,_reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new PairInHand(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new PairHandTable(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new TwoPairFromTable(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new TwoPairHandPairTablePair(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new TwoPairHandTable(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new TwoPairTwoDifferent(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new ThreeOfAKind(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new Combinations.Straight(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new Combinations.Flush(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new Combinations.FullHouse(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new Combinations.FourOfAKind(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new Combinations.StraightFlush(user, _reserve));
                listCombinations.Add(switchCombination);
                switchCombination = new SwitchCombination(new Combinations.RoyalFlush(user, _reserve));
                listCombinations.Add(switchCombination);
                foreach (var listCombination in listCombinations)
                {
                    listCombination.CheckCombination(user, _reserve);
                }

It's my first time ever doing this pattern and I'm not sure if it can be made any better but i know that if I'm writing something similar a couple times there's probably a better way of doing it. I created list of those Combinations so i can iterate through them this way I avoid typing switchCombinations.CheckCombination(user, _reserve); all the time.

However, I can't seem to find a way to reduce the overall size of that. As I said earlier, it's my first time implementing this pattern and I'm not sure if it's even effective or if that's the right way to go.

EDIT : Added the SwitchCombination class :

    internal class SwitchCombination
{
    private readonly ICombinationAnalyzer _combinationAnalyzer;

    public SwitchCombination(ICombinationAnalyzer combination)
    {
        _combinationAnalyzer = combination;
    }

    public void CheckCombination(UsersProperties inputUser, int[] inputHand)
    {
        _combinationAnalyzer.Check(inputHand, inputUser);
    }
}

Answer 1

You have an public abstract class called ICombinationAnalyzer: that's wrong, only interfaces can use the I prefix. Instead I'd expect this class to have Base in its name at the start or the end (whatever is your preference).

---

This seems odd: user.Power = powerType + user.Type*100; To me it would make more sense for the user to have a powerType field, and for Power to simply be a get which returns powerType + Type*100;.

I'm confused by all of UpdateHand(UsersProperties user, int combinationType, int powerType), to be honest: this method updates two properties of its parameter user and then calls MainPoker.Win.Add(new Hand {Power = user.Power, Current = user.Type}); which doesn't look like it updates a Hand, but instead adds a new one.

Why are combinationType and powerType ints? Their names suggest they should be enums, but then you use them in a calculation...

This whole method throws up all kinds of warning signs to me and I can't shake the feeling you have some fundamental flaws in the design of your solution.

---

The RoyalFlush also raises some questions: Hand.Combinations.RoyalFlush is cast to an int -- so why isn't it one in the first place? Same with Cards.CardTypes.Ace and others in that namespace.

Why the need for Hand.Combinations. and Cards.CardTypes. in the first place? It seems odd to use subclasses for such fundamental values.

This whole block is just a mess:

t[0] == (int) Cards.CardTypes.Ace 
&& t.Contains((int) Cards.CardTypes.Ten) 
&& t.Contains((int) Cards.CardTypes.Jack) 
&& t.Contains((int) Cards.CardTypes.Queen) 
&& t.Contains((int) Cards.CardTypes.King)))

Even formatted like this I feel there's too much going on: the casts to int don't help, plus there's the odd check that the first item is an Ace while you use .Contains() for the other types.

---

But Straight is even worse than RoyalFlush and just is incomprehensible. What is op? Why do you need to execute int[] op = Straight.Select(o => o/4).Distinct().ToArray(); each time -- it's not likely to change, now is it?

op[j] + 1 == op[j + 1] && op[j] + 2 == op[j + 2] && op[j] + 3 == op[j + 3] && op[j] + 4 == op[j + 4] suggests that op is sorted, but there's no evidence of this being the case.

This next bit looks a lot like a similar block in RoyalFlush above and I'm wondering whether they shouldn't be the same.

if (op[j] != (int) Cards.CardTypes.Ace 
    || op[j + 1] != (int) Cards.CardTypes.Ten 
    || op[j + 2] != (int) Cards.CardTypes.Jack 
    || op[j + 3] != (int) Cards.CardTypes.Queen 
    || op[j + 4] != (int) Cards.CardTypes.King) 
{
    continue;
}

---

The "problem" you identify seems trivial. You can easily reduce it to this:

var listCombinations = new List<SwitchCombination>
{
    new SwitchCombination(new HighCard(user, _reserve)),
    new SwitchCombination(new PairFromTable(user,_reserve)),
    new SwitchCombination(new PairInHand(user, _reserve)),
    new SwitchCombination(new PairHandTable(user, _reserve)),
    new SwitchCombination(new TwoPairFromTable(user, _reserve)),
    new SwitchCombination(new TwoPairHandPairTablePair(user, _reserve)),
    new SwitchCombination(new TwoPairHandTable(user, _reserve)),
    new SwitchCombination(new TwoPairTwoDifferent(user, _reserve)),
    new SwitchCombination(new ThreeOfAKind(user, _reserve)),
    new SwitchCombination(new Combinations.Straight(user, _reserve)),
    new SwitchCombination(new Combinations.Flush(user, _reserve)),
    new SwitchCombination(new Combinations.FullHouse(user, _reserve)),
    new SwitchCombination(new Combinations.FourOfAKind(user, _reserve)),
    new SwitchCombination(new Combinations.StraightFlush(user, _reserve)),
    new SwitchCombination(new Combinations.RoyalFlush(user, _reserve)),
}

I'd probably refactor that even more, but right now you only posted fragments of your code and it's too much work to fill in the gaps.

---

UPDATE: The SwitchCombination you posted is a completely superfluous wrapper class, if that's all of its code. var listCombinations = new List<SwitchCombination> should instead be something like var combinationAnalyzers = new List<BaseCombinationAnalyzer> and be filled with classes that implement BaseCombinationAnalyzer, e.g. new PairFromTable(user,_reserve) instead of new SwitchCombination(new PairFromTable(user,_reserve)).

Answer 2

One note on naming: ICombinationAnalyzer is a class—albeit abstract—not an interface, so its name should not start with I.

This boilerplate initialization code can be made much shorter and easier to read if you dispense with that local variable, which is doing nothing for you.

This:

List<SwitchCombination> listCombinations = new List<SwitchCombination>();
var switchCombination = new SwitchCombination( new HighCard(user, _reserve));
listCombinations.Add(switchCombination);
switchCombination = new SwitchCombination(new PairFromTable(user,_reserve));
listCombinations.Add(switchCombination);

can become

List<SwitchCombination> listCombinations = new List<SwitchCombination>();
listCombinations.Add(new SwitchCombination(new HighCard(user, _reserve)));
listCombinations.Add(new SwitchCombination(new PairFromTable(user,_reserve));

Just as you say

I created list of those Combinations so i can iterate through them this way I avoid typing switchCombinations.CheckCombination(user, _reserve); all the time.

which is good thinking, you could also change to creating a list of CombinationAnalyzer objects and map over it to create the list of SwitchCombination objects that you need, rather than making all those new SwitchCombination(… constructor calls manually.

More generally, I wonder if you are really using Strategy. Without the code of SwitchCombination it is difficult to know. But the motivation of Strategy is to allow a choice at runtime of one possibility at a time out of several different implementations of an algorithm. If I understand what you are doing here seems more like the intention of the Chain of Command pattern.

The local variable localType in your implementations of the Check method isn't doing much for you, either. I suggest you inline it.