"Rusz głową" Pierwsze laboratorium

Witam, wczoraj skończyłem to laboratorium, zastanawiam się w jakim stanie jest mój kod, tj. chciałbym aby ktoś go sprawdził czy wszystko dobrze zrobiłem i ewentualnie dał mi jakieś uwagi/wskazówki co można było zrobić lepiej. Generalnie program działa, nie znalazłem jeszcze żadnego problemu, może ktoś z większym doświadczeniem się czegoś dopatrzy.

GitHub: https://github.com/Wheliee/Day-at-the-races.git

Z góry dziękuję.

EDIT: Fakt, program symuluje obstawianie wyścigów psów, jest 3 facetów i 4 charty. Faceci mogą obstawić tylko jeden raz na wyścig, tylko jednego charta, minimalny zakład 5 zł, maksymalny zakład 15zł, każdy zakład opiera się na zasadzie wszystko albo nic tj. jeśli facet A postawił na charta nr 1 5zł to jeśli wygra dostaje 10 zł (5zł wkładu 5 zł wygranej) jeśli przegra traci swój wkład. Psy biegną po prostym torze, wyścig jest kompletnie losowy, to że chart nr 1 wygrał poprzedni wyścig nie oznacza że ma większą szansę na zwycięstwo w następnym.

EDIT 2: Jeszcze dorzucę może cały projekt w zipie.

Wypadałoby dać polecenie. Nie każdy ma ta książkę

Podbijam bo spadł na drugą a jeszcze nikt się nie wypowiedział.

namespace Dzień_na_wyścigach

Say what?

Wiedziałem, że ktoś na to zwróci uwagę :D
Laboratorium się tak nazywa, ogólnie wiem że nie powinno się korzystać z polskich znaków, polskiego nazewnictwa, spacji przy takich rzeczach.

Mnóstwo kodu w formularzach. Od logiki "biznesowej" są klasy serwisowe, a nie formularze.
Greyhound.Run() - to ta metoda powoduje wykonanie kroku, całego wyścigu, czy tylko zwraca że pies dobiegł? Nazwa sugeruje, że chodzi o cały wyścig jednego psa; zwracany bool sugeruje, że sprawdza, czy chart dobiegł, a tymczasem kod sugeruje, że to jeden krok + sprawdzenie, czy dobiegł. Jeśli chodzi o zasady SOLID oraz KISS, to bardzo źle. Powinieneś to rozbić na osobne metody - pies tylko startuje i biegnie, czyli ma jakąś metodę Start, która resetuje jego stan (nie jest konieczna, jeśli pies bierze udział tylko w jednym wyścigu), metodę MakeStep, która przemieści psa o losową ilość pikseli czy tam jakichś innych jednostek, oraz metodę GetCompletedDistance. Może mieć jeszcze metodę Stop, ale patrząc na to biznesowo, to co nas obchodzi, że pies jeszcze biegnie, chociaż wyścig już ukończony? Nie trzeba nic po psie sprzątać (nie mam na myśli kupy, tylko wątki, timery i inne tego typu zasoby), więc Stop jest zbędny.
Moim zdaniem pies nie powinien wiedzieć, że dobiegł do końca. Z tego powodu to RaceTrack powinien trzymać odległość do przebiegnięcia i udostępniać metodę sprawdzającą, czy któryś z psów już dobiegł.
Ze względu na enkapsulację danych pola StartingPosition, RaceTrackLength, Location, Randomizer powinny być prywatne, a ich wartości powinny być nadawane w konstruktorze lub w metodzie Start. MyPictureBox powinno być prywatne, to pies wie jak wygląda, a nie ktoś mu to mówi. Randomizer powinno być statyczne, bo inaczej będziesz mieć bardzo duże prawdopodobieństwo, że wszystkie psy wylosują te same liczby w kolejnych krokach.

            if (MyPictureBox.Right >= RaceTrackLength)
                return (true);
            else
                return false;

Nie prościej return MyPictureBox.Right >= RaceTrackLength;? Ale ta metoda jest do wywalenia.

I tym podobnie, i tak dalej...

Praca domowa: KISS, SOLID (a tak naprawdę S z tego SOLIDa, bo na resztę literek masz za mało kodu). Na szczęście DRY nie łamiesz.

Hermetyzacja (enkapsulacja) zaczęła się właśnie po tym laboratorium w tej książce i już po przeczytaniu kilku stron zapaliła mi się lampka, że niektóre rzeczy w tym laboratorium mogłem zrobić z wykorzystaniem właśnie tego, konstruktory też się zaczęły po tym laboratorium (ot dziwnie skaczą w tej książce po tematach, ale mi się to nawet podoba, nie ma monotonni w trakcie czytania).

Dzięki za wskazówki spróbuję wrócić do programu jakoś w weekend i napisać go czytelniej.

Napisałem już pierwszą klasę greyhound (mam nadzieje lepiej niż wcześniej):

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DayAtTheRaces
{
    public class Greyhound
    {
        private int StartingPosition { get; set; }
        private int RaceTrackLength { get; set; }
        private int Location { get; set; }
        private static Random Randomizer=new Random();
        public PictureBox MyPictureBox;

        public void Start(Random myRandom)
        {
            myRandom = Randomizer;
        }

        public void TakeStartingPosition()
        {
            MyPictureBox.Left = StartingPosition;
        }

        public int Move()
        {
            Location += MyPictureBox.Left;
            return Location;
        }

        public int Run()
        {
            MyPictureBox.Left += Randomizer.Next(1, 4);
            return MyPictureBox.Left;
        }
    }
} 

A tak wygląda formularz w tym momencie:

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DayAtTheRaces
{
    public partial class Form1 : Form
    {
        Greyhound[] GreyhoundArray = new Greyhound[4];
        Random randomizer = new Random();

        public Form1()
        {
            InitializeComponent();
            GreyhoundArray[0] = new Greyhound() { MyPictureBox = dogPicture1 };
            GreyhoundArray[1] = new Greyhound() { MyPictureBox = dogPicture2 };
            GreyhoundArray[2] = new Greyhound() { MyPictureBox = dogPicture3 };
            GreyhoundArray[3] = new Greyhound() { MyPictureBox = dogPicture4 };
        }
        private void startButton_Click(object sender, EventArgs e)
        {
            timer1.Start();
            groupBox1.Enabled = false;
        }

        private void timer1_Tick(object sender, EventArgs e)
        {
            for (int i = 0; i < GreyhoundArray.Length; i++)
            {
                GreyhoundArray[i].Start(randomizer);
                GreyhoundArray[i].Run();
                GreyhoundArray[i].Move();
                if (GreyhoundArray[i].MyPictureBox.Right>=raceTrackPictureBox.Width)
                {
                    timer1.Stop();
                    groupBox1.Enabled = true;
                    MessageBox.Show("Pies numer " + (i + 1) + " zwyciężył.", "Mamy zwycięzce!");
                    for (int j = 0; j < GreyhoundArray.Length; j++)
                    {
                        GreyhoundArray[j].TakeStartingPosition();
                    }
                }
            }
        }
    }
} 

Prosiłbym o opinię czy w dobrym kierunku poszedłem.

Kierunek dobry, acz zrealizowałeś może 1/3 z tego, co napisałem.
Gdzie się podziała klasa RaceTrack?
Czym się różni Move od Run? Jak pies biegnie, to się przemieszcza. A tu w jednym miejscu coś zwiększasz, w drugim coś sprawdzasz...
Start(Random myRandom) { myRandom = Randomizer; - to chyba coś Ci się mocno pomyliło. Przypisujesz nie w tą stronę co trzeba, poza tym przecież już masz w klasie prywatne, zainicjowane pole z obiektem Random, po co Ci kolejne z konstruktora?
MyPictureBox.Left - nie używaj tego jako miernika odległości, użyj dedykowanej zmiennej.

Klasa Guy:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DayAtTheRaces
{
    public class Guy
    {
        private int Cash;
        private string Name;
        private Label MyLabel;
        public RadioButton MyRadioButton;
        public Bet MyBet;

        public Guy(int cash, string name, Label myLabel, RadioButton myRadioButton)
        {
            Cash = cash;
            Name = name;
            MyLabel = myLabel;
            MyRadioButton = myRadioButton;
        }

        public bool PlaceBet(int amount, int dogToWin)
        {
            MyBet = new Bet() { Amount = amount, Dog = dogToWin, Bettor = this };
            if (this.MyBet.Bettor.Cash > 5)
                return true;
            else
                return false;
        }

        public void ClearBet()
        {
            MyBet = null;
        }

        public void UpdateLabels()
        {
            if (this.MyBet == null)
                this.MyLabel.Text = Name + " nie postawił zakładu.";
            else
                this.MyLabel.Text = Name + " postawił " + MyBet.Amount + " na psa numer " + MyBet.Dog + ".";
        }

        public void Collect(int Winner)
        {
            this.Cash += MyBet.PayOut(Winner);
        }

        public void UpdateRadios()
        {
            this.MyRadioButton.Text = Name + " ma " + Cash + " zł.";
        }
    }
}

Klasa Greyhound:

 using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DayAtTheRaces
{
    public class Greyhound
    {
        private PictureBox MyPictureBox;
        public PictureBox RaceTrackLength;
        private static Random Randomizer = new Random();

        public Greyhound(PictureBox myPictureBox,PictureBox raceTrackLength)
        {
            MyPictureBox = myPictureBox;
            RaceTrackLength = raceTrackLength;
        }

        public void Run()
        {
            MyPictureBox.Left += Randomizer.Next(1, 5);
        }

        public void TakeStartingPosition()
        {
            MyPictureBox.Left = RaceTrackLength.Left;
        }

        public bool IsTheRaceFinished()
        {
            return MyPictureBox.Right >= RaceTrackLength.Width;
        }
    }
}

Klasa Bet:

 using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DayAtTheRaces
{
    public class Bet
    {
        public int Amount;
        public int Dog;
        public Guy Bettor;

        public int PayOut(int Winner)
        {
            if (Bettor.MyBet.Dog == Winner)
                return Amount;
            else
                return -Amount;
        }
    }
}

I cały formularz:

 using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace DayAtTheRaces
{
    public partial class Form1 : Form
    {
        Greyhound[] GreyhoundArray = new Greyhound[4];
        Guy[] GuyArray = new Guy[3];
        
        public Form1()
        {
            InitializeComponent();
            GreyhoundArray[0] = new Greyhound(dogPicture1, raceTrackPictureBox);
            GreyhoundArray[1] = new Greyhound(dogPicture2, raceTrackPictureBox);
            GreyhoundArray[2] = new Greyhound(dogPicture3, raceTrackPictureBox);
            GreyhoundArray[3] = new Greyhound(dogPicture4, raceTrackPictureBox);
            GuyArray[0] = new Guy(100, "Janek", joeBetLabel, joeRadioButton);
            GuyArray[1] = new Guy(150, "Bartek", bobBetLabel, bobRadioButton);
            GuyArray[2] = new Guy(50, "Arek", alBetLabel, alRadioButton);
            foreach (Guy Guy in GuyArray)
            {
                Guy.UpdateLabels();
                Guy.UpdateRadios();
            }
        }
        private void startButton_Click(object sender, EventArgs e)
        {
            groupBox1.Enabled = false;
            timer1.Start();
        }

        private void timer1_Tick(object sender, EventArgs e)
        {
            for (int i = 0; i < GreyhoundArray.Length; i++)
            {
                GreyhoundArray[i].Run();
                if(GreyhoundArray[i].IsTheRaceFinished())
                {
                    timer1.Stop();
                    i++;
                    MessageBox.Show("Pies numer " + i + " zwycięzył.", "Mamy zwycięzcę.");
                    groupBox1.Enabled = true;
                    for (int j = 0; j < GuyArray.Length; j++)
                    {
                        if (GuyArray[j].MyBet == null)
                            break;
                        else
                        {
                            GuyArray[j].Collect(i);
                            GuyArray[j].UpdateRadios();
                            GuyArray[j].ClearBet();
                            GuyArray[j].MyRadioButton.Enabled = true;
                            GuyArray[j].MyRadioButton.Checked = false;
                            nameLabel.Text = "Imię";
                        }
                        GuyArray[j].UpdateLabels();
                    }
                }
            }
        }

        private void joeRadioButton_CheckedChanged(object sender, EventArgs e)
        {
            nameLabel.Text = "Janek";
        }

        private void bobRadioButton_CheckedChanged(object sender, EventArgs e)
        {
            nameLabel.Text = "Bartek";
        }

        private void alRadioButton_CheckedChanged(object sender, EventArgs e)
        {
            nameLabel.Text = "Arek";
        }

        private void betButton_Click(object sender, EventArgs e)
        {
            int guyNumber = 3;
            if (joeRadioButton.Checked) guyNumber = 0;
            if (bobRadioButton.Checked) guyNumber = 1;
            if (alRadioButton.Checked) guyNumber = 2;
            GuyArray[guyNumber].PlaceBet((int)betNumericUpdAndDown.Value, (int)dogNumericUpAndDown.Value);
            GuyArray[guyNumber].UpdateLabels();
            GuyArray[guyNumber].MyRadioButton.Enabled = false;
        }

        private void resetButton_Click(object sender, EventArgs e)
        {
            foreach (Greyhound Dog in GreyhoundArray)
            {
                Dog.TakeStartingPosition();
            }
        }
    }
}

Tyle co mi się udało wymyślić przez 1 dzień kod w formularzu zmniejszył się o 6 linijek, może o trochę więcej bo dodałem przycisk do resetu toru, ponieważ po resecie z pętli psy jeszcze przeskakiwały na formularzu o wartość Random która im pozostała po skończonym wyścigu co powodowało, że nie ustawiały się na starcie w jednej pozycji.