Poker app in C#

Question

I am developing a poker application using C#. It is almost done, and I'm looking for improvements. One of the things I wonder is whether I should change my method that checks if the player has a flush. It's quite big compared to the others but it works just fine, however, something can be improved. Maybe put some of the stuff in collections, but I am not sure if that's going to hurt the overall performance.

In the following code, I'm using the % operator and dividing by 4 because there are 4 cards of the same type but with different colors. The deck of cards for playing poker consists of 52 cards. I start to count them from 0. If we take the cards starting from ace up to king we will have 0-51 what we can do from here is assume that each Ace (0,1,2,3) has result when divided by 4 , 0 (0 /4 = 0, 1 / 4 = 0) so on.

But in the current method, I am using the % operator. This helps me determine what's the current color of the card because the clubs are 0-4-8-12-16. When we divide 0,4,8,12,16 by 4 (4 % 4) we get result 0. These are the Clubs and Diamonds: 1-5-9-13-17 and 1 % 4 = 1. That's what we are getting from the f1,f2,f3,f4 arrays. How many cards of each color we have on the table.

//straight1[] contains info about the five cards on the table

    private UsersProperties RFlush(UsersProperties user, int[] straight1)
    {
        const int localType = (int)Type.Combinations.Flush;
        int[] currentColors = {-1, -1, -1, -1, -1};
        int maxColor;
        int[] f1 = straight1.Where(o => o%4 == (int) Cards.Colors.Club).ToArray();
        int[] f2 = straight1.Where(o => o%4 == (int) Cards.Colors.Diamond).ToArray();
        int[] f3 = straight1.Where(o => o%4 == (int) Cards.Colors.Heart).ToArray();
        int[] f4 = straight1.Where(o => o%4 == (int) Cards.Colors.Spade).ToArray();

        bool[] conditions3 = {f1.Length == 3, f2.Length == 3, f3.Length == 3, f4.Length == 3};
        bool[] conditions4 = {f1.Length == 4, f2.Length == 4, f3.Length == 4, f4.Length == 4};
        bool[] conditions5 = {f1.Length == 5, f2.Length == 5, f3.Length == 5, f4.Length == 5};

        int correct3 = Array.IndexOf(conditions3, true);
        int correct4 = Array.IndexOf(conditions4, true);
        int correct5 = Array.IndexOf(conditions5, true);

        List<int[]> places = new List<int[]> {f1, f2, f3, f4};

        for (int j = 0; j < places.ToArray().Length; j++)
        {
            if (correct3 != j && correct4 != j && correct5 != j) continue;
            currentColors = places[j];
            break;
        }

        if (_reserve[_i]%4 == _reserve[_i + 1]%4)
        {
            if (_reserve[_i]/4 > _reserve[_i + 1]/4)
            {
                maxColor = _reserve[_i + 1]/4 == (int) Cards.Numbers.Ace ? 13 : _reserve[_i];
            }
            else
            {
                maxColor = _reserve[_i + 1];
            }
        }
        else
        {
            if (_reserve[_i]%4 == currentColors[0]%4)
            {
                maxColor = _reserve[_i];
            }
            else if (_reserve[_i + 1]%4 == currentColors[0]%4)
            {
                maxColor = _reserve[_i + 1];
            }
            else
            {
                maxColor = -1;
            }
        }
        if (maxColor == -1) return user;
        if (currentColors.Length == 3)
        {
            if (_reserve[_i]%4 == _reserve[_i + 1]%4 && _reserve[_i]%4 == currentColors[0]%4)
            {
                int abc;
                if (_reserve[_i]/4 > _reserve[_i + 1]/4)
                {
                    abc = _reserve[_i]/4;
                }
                else
                {
                    abc = _reserve[_i + 1]/4;
                }
                UpdateHand(user, localType, abc);
            }
        }
        if (currentColors.Length == 4)
        {
            if (maxColor%4 == currentColors[0]%4)
            {
                UpdateHand(user, localType, maxColor);
            }
        }
        if (currentColors.Length != 5 || currentColors.Max() <= 0 || maxColor%4 != currentColors[0]%4)
            return user;
        if (currentColors[0]/4 == (int) Cards.Numbers.Ace && maxColor/4 > currentColors[1]/4)
        {
            UpdateHand(user, localType, maxColor);
        }
        else if (currentColors[0]/4 < maxColor/4)
        {
            UpdateHand(user, localType, maxColor);
        }
        return user;
    }

Here's the enum Colors. Can this be made into static variables?

public static class Cards
{
    public enum Colors
    {
        Club = 0,
        Diamond = 1,
        Heart = 2,
        Spade = 3
    }
}

Here's the UpdateHand method and the User class needed:

    private void UpdateHand(UsersProperties user, int combinationType, int powerType)
    {
        user.Type = combinationType;
        user.Power = powerType + user.Type * 100;
        _win.Add(new Type { Power = user.Power, Current = user.Type });
    }

public class UsersProperties
{
    public int LeftCard;
    public int RightCard;
    public string Name;
    public int? Chips;
    public int Type;
    public double Power;
    public bool Turn;
    public bool FoldTurn;
    public int PreviousCall { get; set; }
    public int EnumCasted;

    public Point CardsLocation;
    public AnchorStyles CardsAnchor;

    public readonly Panel Panel = new Panel();
    public Point PanelLocation;
    public Size PanelSize = new Size((Settings.Width + 10) * 2, Settings.Height + 20);
    public int IndentationPanelXy = 10;

    public Label UsernameLabel = new Label();
    public Point UsernameLabelLocation;
    public Size UsernameLabelSize = new Size(Settings.Width * 2, 20);

    public TextBox ChipsTextBox = new TextBox();
    public Label StatusLabel = new Label();
}
public class Player : UsersProperties
    {
        public Player(int? chips,Point cardsLocation)
        {
            LeftCard = 1;
            RightCard = 0;
            Name = "Player";
            Chips = chips;
            Type = -1;
            Power = 0;
            Turn = true;
            FoldTurn = false;
            CardsAnchor = AnchorStyles.Bottom;
            PreviousCall = 0;
            EnumCasted = (int)CUser.Player;
            CardsLocation = cardsLocation;
            UsernameLabelLocation = new Point(CardsLocation.X, CardsLocation.Y + Settings.Height);
            PanelLocation = new Point(CardsLocation.X - IndentationPanelXy, CardsLocation.Y - IndentationPanelXy);
        }
        public Player(int? chips, bool turn, bool foldTurn, AnchorStyles style, Point cardsLocation)
        {
            LeftCard = 1;
            RightCard = 0;
            Name = "Player";
            Chips = chips;
            Type = -1;
            Power = 0;
            Turn = turn;
            FoldTurn = foldTurn;
            CardsAnchor = style;
            PreviousCall = 0;
            EnumCasted = (int)CUser.Player;
            CardsLocation = cardsLocation;
            UsernameLabelLocation = new Point(CardsLocation.X, CardsLocation.Y + Settings.Height);
            PanelLocation = new Point(CardsLocation.X - IndentationPanelXy, CardsLocation.Y - IndentationPanelXy);
        }
    }

Answer 1

Encapsulation.

Your UserProperties class actually contains exactly ONE so-called property.

public int PreviousCall { get; set; }

This member looks something like this at run-time:

private int _previousCall;

public int get_PreviousCall() { return _previousCall; }
public void set_PreviousCall(int value) { _previousCall = value; }

Everything else you have in that class is called a field. And public fields break encapsulation, one of the four pillars of Object-Oriented Programming.

---

Separation of Concerns.

There's another major problem with UserProperties. It knows about way too many things. It seems it should be concerned with data, not with presentation yet...

public Point CardsLocation;
public AnchorStyles CardsAnchor;

public readonly Panel Panel = new Panel();
public Point PanelLocation;
public Size PanelSize = new Size((Settings.Width + 10) * 2, Settings.Height + 20);
public int IndentationPanelXy = 10;

public Label UsernameLabel = new Label();
public Point UsernameLabelLocation;
public Size UsernameLabelSize = new Size(Settings.Width * 2, 20);

public TextBox ChipsTextBox = new TextBox();
public Label StatusLabel = new Label();

Labels and TextBox controls belong in the UI, not in your data.

The Player class has literally no reason to not be the UserProperties class - this inheritance tree is completely artificial and superfluous. I think my first refactoring would be to move the UserProperties members into the Player class, and then get rid of UserProperties.

The dependency on the UserProperties type is a false abstraction; you're really depending on a specific implementation. This is a misuse of inheritance.

---

I need to say something about this snippet:

int abc;
if (_reserve[_i]/4 > _reserve[_i + 1]/4)
{
    abc = _reserve[_i]/4;
}
else
{
    abc = _reserve[_i + 1]/4;
}
UpdateHand(user, localType, abc);

What's up with the magic 4 here? If that stands for the number of suits in a deck of cards, then it should be clearer than that. Also, what's the meaning of abc? And what is localType?

const int localType = (int)Type.Combinations.Flush;

That's utterly confusing. Type is a .NET class; if it's yet another static class that's just another container for the Combinations enum, it's wrong.

Here's the enum Cards.Colors maybe this can be made into static variables?

There's nothing wrong with using an enum for this. Except enum types shouldn't be pluralized, so that would be Color, and if you're using the Cards class as a container for the enum, note that you can get rid of it - enums can perfectly live inside a namespace, they're full-fledged types too!

That said, I'd go with Suit instead of Color, and drop the redundant explicit numbering:

public enum Suit
{
    Club,
    Diamond,
    Heart,
    Spade
}

Similarly, the Combinations enum should be Combination, and again the semantic value of the enum member is the member itself, not its underlying int.

Same with Cards.Numbers, which would probably be better off as FaceValue. When I see a pluralized class name (like Cards), I expect there's another class (singular) with a similar name (e.g. Card), and that the plural one represents some kind of collection of the singular one, e.g. Cards : ICollection<Card>.

You're abusing static classes here.