5
\$\begingroup\$

I recently began learning C# and I picked up Head First C# as a starting point. One of the first projects they have you build, without the code provided, is a racetrack betting app. The project first gives you the requirements, as well as some suggested implementations. I did not follow the suggestions too closely though as I think I found some simpler solutions. I would like any and all feedback.

The App looks like this. The greyhounds are currently represented by empty PictureBoxs.

racetrack interface

Requirements:

  • The four white squares are the greyhounds, each in its own lane.
  • There are three RadioButtons which represent the three people at the betting parlor. The selected RadioButton sets the name for the Label on the place bet line.
  • There are two NumericUpDowns used to change the amount and dog before clicking the placeBet button.
  • Placing a Bet updates the corresponding label for the currently selected individual. A Person can only have one Bet at a time. New Bets override old ones.
  • The startRace button starts the race and closes the betting.
  • Bets are paid out automatically at the end of the race and then the track is open for betting again.

Form1.cs

using System;
using System.Windows.Forms;

namespace RacetrackApp
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
            minimumBetLabel.Text = "Minimum Bet: " + minimumBet;
            People[0].AssignRadioButtonAndLabel(personOne, personOneBetLabel);
            People[1].AssignRadioButtonAndLabel(personTwo, personTwoBetLabel);
            People[2].AssignRadioButtonAndLabel(personThree, personThreeBetLabel);
            Greyhounds[0].AssignPictureBox(dogOne);
            Greyhounds[1].AssignPictureBox(dogTwo);
            Greyhounds[2].AssignPictureBox(dogThree);
            Greyhounds[3].AssignPictureBox(dogFour);

            activePerson = People[0];
            nameLabel.Text = activePerson.Name;
        }

        private const int minimumBet = 5;
        private bool racing = false;
        private Person[] People = { new Person("Joe", 50), new Person("Bob", 75), new Person("Alice", 100) };
        private Greyhound[] Greyhounds = { new Greyhound(1), new Greyhound(2), new Greyhound(3), new Greyhound(4) };
        private Person activePerson;

        private void CheckedChanged(object sender, EventArgs e)
        {
            if (sender as RadioButton == personOne)
            {
                activePerson = People[0];
            }
            else if (sender as RadioButton == personTwo)
            {
                activePerson = People[1];
            }
            else
            {
                activePerson = People[2];
            }

            nameLabel.Text = activePerson.Name;
        }

        private void PlaceBet_Click(object sender, EventArgs e)
        {
            if (!racing)
            {
                activePerson.MakeBet((int)betNumber.Value, Greyhounds[(int)dogNumber.Value - 1]);
            }
        }

        private void StartRace_Click(object sender, EventArgs e)
        {
            if (!racing)
            {
                racing = true;
                timer1.Start();
            }
        }

        private void Timer1_Tick(object sender, EventArgs e)
        {
            foreach (Greyhound greyhound in Greyhounds)
            {
                if (greyhound.Run())
                {
                    EndRace(greyhound);
                    break;
                }
            }
        }

        private void EndRace(Greyhound winningGreyhound)
        {
            foreach (Person person in People)
            {
                person.CheckBet(winningGreyhound);
            }
            foreach (Greyhound greyhound in Greyhounds)
            {
                greyhound.ReturnToStart();
            }
            timer1.Stop();
            racing = false;
        }
    }
}

Form1 just holds the objects and maintains the Interface interactions.


Person.cs

using System.Windows.Forms;

namespace RacetrackApp
{
    public class Person
    {
        public Person(string name, int cash)
        {
            Name = name;
            Cash = cash;
        }

        public string Name { get; }
        public int Cash { get; private set; }
        public Bet Bet { get; private set; }

        public RadioButton MyRadioButton { get; private set; }
        public Label MyLabel { get; private set; }

        public void AssignRadioButtonAndLabel(RadioButton radioButton, Label label)
        {
            MyRadioButton = radioButton;
            MyLabel = label;

            MyRadioButton.Text = Name + " has $" + Cash;
            MyLabel.Text = null;
        }

        public void MakeBet(int amount, Greyhound greyhound)
        {
            if (!(Bet is null))
            {
                Cash += Bet.Amount;
            }
            Cash -= amount;
            Bet = new Bet(amount, greyhound);
            MyLabel.Text = Name + " bets $" + Bet.Amount + " on Dog " + greyhound.IDNumber;
        }

        public void CheckBet(Greyhound greyhound)
        {
            if (!(Bet is null))
            {
                if (greyhound == Bet.Greyhound)
                {
                    Cash += Bet.Amount * 2;
                }
                MyRadioButton.Text = Name + " has $" + Cash;
                Bet = null;
                MyLabel.Text = null;
            }
        }
    }
}

Person maintains private state, like name, cash, an instance of Bet and reference to the UI elements directly related to them.


Bet.cs

namespace RacetrackApp
{
    public class Bet
    {
        public Bet(int amount, Greyhound greyhound)
        {
            Amount = amount;
            Greyhound = greyhound;
        }

        public int Amount { get; }
        public Greyhound Greyhound { get; }
    }
}

Bet is a simple data struct that is declared as a class so it can be nulled


Greyhound.cs

using System;
using System.Drawing;
using System.Windows.Forms;

namespace RacetrackApp
{
    public class Greyhound
    {
        public Greyhound(int ID) => IDNumber = ID;

        public PictureBox PictureBox { get; private set; }
        public int IDNumber { get; }
        private static readonly Random random = new Random();
        private const int StartingLine = 12;
        private const int FinishLine = 875;
        private const int MinSpeed = 10;
        private const int MaxSpeed = 35;

        public void AssignPictureBox(PictureBox pictureBox) => PictureBox = pictureBox;

        public void ReturnToStart()
        {
            int YLocation = PictureBox.Location.Y;
            PictureBox.Location = new Point(StartingLine, YLocation);
        }

        public bool Run()
        {
            int XLocation = random.Next(PictureBox.Location.X + MinSpeed, PictureBox.Location.X + MaxSpeed);
            int YLocation = PictureBox.Location.Y;
            if (XLocation > FinishLine)
            {
                XLocation = FinishLine;
                PictureBox.Location = new Point(XLocation, YLocation);
                return true;
            }
            PictureBox.Location = new Point(XLocation, YLocation);
            return false;
        }
    }
}

Greyhound has a reference to it's own image (which is currently blank) and then, using Random, moves across the track. The Run() method returns true when the greyhound crosses the finish line. This is the thing I am least satisfied with my solution. If two dogs would both cross the finish line within the same tick the one that wins is the one that is indexed lower in the array. I chose this solution because I did not want to have ties and I can't imagine a tie-breaking system that wasn't too sophisticated for this simple exercise.

I look forward to anything you can tell me to help with my approach to C#.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your race only has 2^32 possible sequences. This is because of the static Random instance and all arguments to .Next() are deterministic. This is generally a good thing for testing, but not if you want truly random races every time. \$\endgroup\$ – Brad M Jan 23 at 19:23
  • \$\begingroup\$ @BradM thank you for that truly useful observation. More importantly, would this effect the probability of one dog winning the race over there others? \$\endgroup\$ – bruglesco Jan 23 at 19:42
  • 1
    \$\begingroup\$ "Probability" isn't the best word to describe this code because it is 100% deterministic based on the random seed (the default is the system timestamp). Using the Random class properly is deceptively complex for other reasons as well. \$\endgroup\$ – Brad M Jan 23 at 20:05
  • \$\begingroup\$ @BradM I was worried you would say that when you mentioned it was deterministic. Thank you. Sounds like something worthwhile for me to look into. \$\endgroup\$ – bruglesco Jan 23 at 20:10
  • \$\begingroup\$ Check out my approach is shuffling a list inside /src/core/Utilities.cs. It should give some insight into handling these scenarios. github.com/bradmarder/MSEngine \$\endgroup\$ – Brad M Jan 23 at 20:16
1
\$\begingroup\$

Some things you might like to think about. (Please don't be disheartened. Most of this is picky).

I'd like to see the people and the greyhounds more separated from the form used for display. There's no real reason for a greyhound to know anything about a picture box for example - it should know how far it's run, then the form draws the picture box using that information. This is probably not something to worry much about at this stage in your learning, but as you write more programs, you'll thank yourself for keeping business/game logic and presentation separate. (I'm not familiar with the book you're using by the way, but it sounds like it has fun examples/exercises).

However, given that you are letting the form run everything, I note that the Assign methods (particularly AssignPictureBox) are really just setters. Greyhound.PictureBox might just as well have public get and set if you're going to provide a method to set it anyway. (I like that you're trying to use encapsulation to keep the setter private, but if a method exists to set it anyway, encapsulation isn't working - let it be a public setter, if that's what it honestly is. It works much better for Person.Cash for example, where the setter is rightly private - nothing outside should be able to set how much cash a person has, only give or take cash when they win or lose a bet, so a private set is absolutely correct there).

Bet is good, I like that it's immutable. Just a thought while I'm looking at it: the usual data type for currency is decimal, rather than int (what if someone wants to bet $5.75?). No big deal, but it'd be a good habit to get into.

Personally I vastly prefer using var whenever possible (i.e. local variables), for example foreach (var person in People). There are good reasons for this (particularly making it easier to change the type of something without having to update everywhere it's used), though some people think it obscures information from the programmer and prefer not to use it. I think that misses the point, but I won't repeat the flame war about it here, just something for you to read about and make up your own mind. :-)

I really like that you're naming your controls (if I ever again have to try to work out what button1 does or what gets entered into textBox2, I'll scream) but I would recommend including what the control is in the name. personOne doesn't sound like a radio button, personOneRadioButton does, and it's easily distinguishable from personOneLabel and personOneTextBox, if such things existed.

CheckedChanged could definitely be improved by adding the Person as a Tag on the radio button. So, in your initialization code you'd do something like this:

personOneRadioButton.Tag = People[0];
personTwoRadioButton.Tag = People[1];
...

Then, CheckedChanged doesn't need all those ifs and ass, and becomes simply:

var rb = (RadioButton)sender;
activePerson = (Person)(rb.Tag);
nameLabel.Text = activePerson.Name;

Unless I'm missing something, a person can make a bet they don't actually have enough money for? :-D Also, you don't show the generated code for the controls (which is fine), but I hope the betNumber NumericUpDown has a MinValue of 0, to prevent people placing negative bets and hoping they lose, right? :-D

I like that you've thought about how to handle ties, or at least are aware of it. An alternative approach might be to let all the greyhounds run in every tick, then at the end of Timer1_Tick, look for the greyhound with the largest X position and award them the win if they're past the finish line. (Ties would still be possible, and could be resolved however you see fit, but currently the greyhounds near the top of the track get more chances to win. If this were gambling real money, punters would not like that!)

Greyhound.Run() is a little messy. There's a bit of code duplication, and the distance the greyhound ran that tick is mixed up with the picture box stuff, and two returns where there could be just one... looks like it works fine, but it could be cleaner. Something like:

var distanceRan = random.Next(MinSpeed, MaxSpeed);
var newX = Math.Max(PictureBox.Location.X + distanceRan, FinishLine);
PictureBox.Location = new Point (newX, PictureBox.Location.Y);
return newX >= FinishLine;

(Given the suggestion above about deciding who wins, the Math.Max didn't ought to be necessary, nor returning a bool of whether the greyhound has won or not; but I wanted to demonstrate changing only the way the method is laid out (neater IMO), not what it does.)

On the use of Random (as it was mentioned in the comments): personally I think what you've done is fine as-is. The clock is used as a seed in new Random() so it's not like every race will be the same, and there's no way to predict the outcome. It is very common for games to make sure a different seed is used each playthrough, but for everything after that to be deterministic - it helps with testing, replays, online multiplayer synchronization, etc. So I think it's fine. However, you may decide that for fairness or other reasons, that's not enough for you. In that case, you could use a new random seed each race, or for each greyhound, instead of just once.

Hope this helps.

\$\endgroup\$

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.