Skip to main content
added 458 characters in body
Source Link
BCdotWEB
  • 11.3k
  • 2
  • 27
  • 45

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


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

Source Link
BCdotWEB
  • 11.3k
  • 2
  • 27
  • 45

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.