Prośba o sprawdzenie kodu

0

Piszę sobie pewien program i mam wrażenie, że coś robię źle. To znaczy, że pewnie wszystko jest źle.
Klasa główna
Lista kontrolek
Layout - jeśli komuś miałby pomóc.

0

Zacznij od podstaw logiki i matematyki, ponieważ zamiast:

            public static void EnergyNeeded(UInt16 level)
            {
                UInt64 energy = 300000;
                UInt64 temp = 0;
                for (UInt16 i = 1; i < level; i++)
                {
                    for (UInt16 j = 1; j <= 3; j++)
                    {
                        temp += energy;
                    }
                    energy = temp;
                    temp = 0;
                }
                energyNeeded = energy;
            }

można napisać:

            public static void EnergyNeeded(UInt16 level)
            {
                energyNeeded = 300000*(UInt64)Math.Pow(3,level);
            }

Albo ci nie o to chodziło, albo nie widzisz że można tak uprościć, bo brakuje matematyki i logiki.

0
 public static string FormattedTime(UInt64 time)
            {
                StringBuilder output = new StringBuilder();
                if (time == 0)
                {
                    output.Append(time);
                    output.Append(" sekund");
                    return output.ToString();
                }
                else
                {
                    if (time > 31557600)
                    {
                        output.Append(time / 31557600);
                        output.Append(" lat  ");
                        time %= 31557600;
                    }
                    if (time > 86400)
                    {
                        output.Append(time / 86400);
                        output.Append(" dni  ");
                        time %= 86400;
                    }
                    if (time > 3600)
                    {
                        output.Append(time / 3600);
                        output.Append(" godzin  ");
                        time %= 3600;
                    }
                    if (time > 60)
                    {
                        output.Append(time / 60);
                        output.Append(" minut  ");
                        time %= 60;
                    }
                    if (time > 0)
                    {
                        output.Append(time);
                        output.Append(" sekund");
                    }
                    return output.ToString();
                }
            }

To jest bardzo złe... Po pierwsze pierwszy warunek jest zbędny, możesz uprościć do


            if (time > 31557600)
            {
                output.Append(time / 31557600);
                output.Append(" lat  ");
                time %= 31557600;
            }
            if (time > 86400)
            {
                output.Append(time / 86400);
                output.Append(" dni  ");
                time %= 86400;
            }
            if (time > 3600)
            {
                output.Append(time / 3600);
                output.Append(" godzin  ");
                time %= 3600;
            }
            if (time > 60)
            {
                output.Append(time / 60);
                output.Append(" minut  ");
                time %= 60;
            }
            if (time > 0)
            {
                output.Append(time);
                output.Append(" sekund");
            }
            return output.ToString();
        

Ale dlaczego w ogóle używać uint64 zamiast TimeSpan? Wtedy możesz skorzystać z ToString() lub uprościć warunek. Jeśli chcesz pozostawić funkcję z takim nagłówkiem jaki ma, to przynajmniej użyj TimeSpan wewnątrz i zmień nazwę argumentu na "seconds" - Twój "time" nic nie mówi.

FormattedNumber nie powinno korzystać z ToString i odpowiedniego formatu?


            public static void SolarEnergy(UInt16 level)
            {
                solarEnergy = (UInt32)(20 * level * Math.Pow(1.1, level) * engineer);
            }
 
            public static void FusionEnergy(UInt16 level, UInt16 techLevel)
            {
                fusionEnergy = (UInt32)(30 * level * Math.Pow(1.05 + (techLevel * 0.01), level) * engineer);
            }
 
            public static void SatteliteEnergy(Int16 minTemp, Int16 maxTemp)
            {
                Int16 avTemp = (Int16)((minTemp + maxTemp) / 2);
                satelliteEnergy = (UInt16)(((avTemp + 160) / 6) * engineer);
            }

Te metody powinny zwracać wynik, a nie go gdzieś ustawiać. Nazwę zmienić do CalculateSolarEnergy itd.

W obsłudze zdarzeń możesz wyizolować wiele wiele metod.

            gravCalc.SolarEnergy((UInt16)gravSolar.Value);
            gravCalc.FusionEnergy((UInt16)gravFusion.Value, (UInt16)gravEnergyResearch.Value);
            gravCalc.SatteliteEnergy((Int16)gravMinTemp.Value, (Int16)gravMaxTemp.Value);

Mógłbyś zrobić UpdateSolarEnergy(), UpdateFusionEnergy(), UpdateSatteliteEnergy() - w ten sposób jasno mówisz co robisz, a i chowasz brzydki kod.

0

Niekoniecznie można uprościć. Co w sytuacji gdy time == 0? Chciałem, żeby wtedy wypisywało 0 sekund. Nie mogłem podłączyć do time >=0 bo wtedy dostawałbym 0 sekund przy pełnych minutach.
Nie jestem pewien czy TimeSpan jest dobrym rozwiązaniem, muszę odciąć lata bo mieści prawie 30 tys., a u mnie może dojść do ponad 26 mln (TimeSpan liczy w tickach, a ja sekundach - mam 10 mln razy większą pojemność). Poza tym nie ma metod do zwracania lat i formatowania takiego jak moje tylko z dwukropkami więc musiałbym wyciągać pojedyncze składniki. Czyli takie mieszanie.

Jaki format, bo "000 000 000 000 itd." dokłada mi zera na początku.

Nie rozumiem cię. Jeśli mam mieć UpdateSolarEnergy(), która ustawia wartość to druga powinna nazywać się raczej GetSolarEnergy(), a nie CalculateSolarEnergy() bo wartość jest obliczana przez Calculate....

Myślę, że powinienem zrobić jakąś hierarchię klas, bo niektóre metody przydadzą mi się w innych częściach programu np. licząca constructTime, czy surowce. W którym miejscu tworzyć obiekty? Np. Ship deathStar = new Ship(...). Czy da się zrobić tak żeby konstruktor dostawał nazwę kontrolki, zapisywał do zmiennej np. typu string, żeby później metodą Update odświeżyć zmienną z danymi? Czy raczej podawać całą kontrolkę jako object? Kontrolki z których pobierane są dane jako stałe? Jak rozwiązać sytuację, w której obiekty z różnych paneli muszą mieć dostęp do tej samej kontrolki? Statyczną klasą/zmienna jak do tej pory?

Czy można używać tego samego zdarzenia kilka razy? Np. MouseLeave dla każdego buttona jest identyczne.

0

Krótko: dobrym kodem jest taki, który czytasz jakbyś czytał zdanie.

public static void SolarEnergy(UInt16 level)
            {
                solarEnergy = (UInt32)(20 * level * Math.Pow(1.1, level) * engineer);
            }

Jeżeli spojrzysz na "SolarEnergy", to nie wiadomo co ona robi.

 public static void CalculateSolarEnergy(UInt16 level, int engineer)
        {
            return (UInt32)(20 * level * Math.Pow(1.1, level) * engineer);
        }

Jeżeli zmienisz to tak, to widać że metoda oblicza "solar energy" i zwraca ją.

        private void gravEnergyResearch_ValueChanged(object sender, EventArgs e)
        {
            gravCalc.FusionEnergy((UInt16)gravFusion.Value, (UInt16)gravEnergyResearch.Value);
            gravFusionEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.fusionEnergy);
        }

Kolejny przykład, czytam nazwę funkcji i się domyślam że dotyczy ona jakiegoś textboxa/slidera/..., ale co się dzieje w niej? Na pierwszy rzut oka trudno się dowiedzieć. Możesz wyizolować UpdateFusionEnergyLabel() itp. i wtedy wiem, że gdy zmienia się textbox/slider, to wtedy updatujemy odpowiedni label. Widzę to na pierwszy rzut oka.

        
private void gravMechanicCheckbox_CheckedChanged(object sender, EventArgs e)
        {
            if (gravEngineerCheckbox.Checked)
            {
                gravCalc.engineer = 1.1f;
                gravEngineerLabel.Visible = true;
                gravEngineerLine.Visible = true;
            }
            else
            {
                gravCalc.engineer = 1.0f;
                gravEngineerLabel.Visible = false;
                gravEngineerLine.Visible = false;
            }
            gravCalc.SolarEnergy((UInt16)gravSolar.Value);
            gravCalc.FusionEnergy((UInt16)gravFusion.Value, (UInt16)gravEnergyResearch.Value);
            gravCalc.SatteliteEnergy((Int16)gravMinTemp.Value, (Int16)gravMaxTemp.Value);
            gravSolarEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.solarEnergy);
            gravFusionEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.fusionEnergy);
            gravSatelliteEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.satelliteEnergy);
        }

        private void gravTemp_ValueChanged(object sender, EventArgs e)
        {
            if (gravMinTemp.Value > gravMaxTemp.Value) gravMinTemp.Value = gravMaxTemp.Value;
            gravCalc.SatteliteEnergy((Int16)gravMinTemp.Value, (Int16)gravMaxTemp.Value);
            gravSatelliteEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.satelliteEnergy);
        }

        private void gravLevel_ValueChanged(object sender, EventArgs e)
        {
            gravCalc.EnergyNeeded((UInt16)gravLevel.Value);
            gravNeededEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.energyNeeded);
        }

        private void gravSolar_ValueChanged(object sender, EventArgs e)
        {
            gravCalc.SolarEnergy((UInt16)gravSolar.Value);
            gravSolarEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.solarEnergy);
        }

        private void gravFusion_ValueChanged(object sender, EventArgs e)
        {
            gravCalc.FusionEnergy((UInt16)gravFusion.Value, (UInt16)gravEnergyResearch.Value);
            gravFusionEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.fusionEnergy);
        }

        private void gravEnergyResearch_ValueChanged(object sender, EventArgs e)
        {
            gravCalc.FusionEnergy((UInt16)gravFusion.Value, (UInt16)gravEnergyResearch.Value);
            gravFusionEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.fusionEnergy);
        }

Powyższy kod wtedy byś zastąpił na coś w stylu...

    private void gravMechanicCheckbox_CheckedChanged(object sender, EventArgs e)
        {
            if (gravEngineerCheckbox.Checked)
				ShowEngineer();
            else
				HideEngineer();
            
            UpdateSolarEnergyLabel();
            UpdateFusionEnergyLabel()
            UpdateSatelliteEnergyLabel();
        }

        private void gravTemp_ValueChanged(object sender, EventArgs e)
        {
            UpdateSatelliteEnergyLabel();
        }

        private void gravLevel_ValueChanged(object sender, EventArgs e)
        {
            gravCalc.EnergyNeeded((UInt16)gravLevel.Value);
            gravNeededEnergyLabel.Text = gravCalc.FormattedNumber(gravCalc.energyNeeded);
        }

        private void gravSolar_ValueChanged(object sender, EventArgs e)
        {
            UpdateSolarEnergyLabel();
        }

        private void gravFusion_ValueChanged(object sender, EventArgs e)
        {
            UpdateFusionEnergyLabel();
        }

        private void gravEnergyResearch_ValueChanged(object sender, EventArgs e)
        {
            UpdateFusionEnergyLabel();
        }

... zauważ, że w kilku miejscach użyjesz tych nowych metod. Na dodatek podkreślą one "co auto miał na myśli". Na pierwszy rzut oka widać co dana akcja robi - nie wiemy "jak" to robi, ale zazwyczaj tego nie potrzeba.

Co FormattedTime() - nie będę się spierał, potrzebujesz wyświetlać większą wartość niż pomieści TimeSpan, to ok...

if (time == 0)
{
    output.Append(time);
    output.Append(" sekund");
    return output.ToString();
}

Ale mimo wszystko, to powino wyglądać po prostu

if ( time == 0 )
	return "0 sekund";

Kilka linii kodu zaoszczędzone, a przy okazji od razu widać wynik.

0
Przeszczep napisał(a):

Jaki format, bo "000 000 000 000 itd." dokłada mi zera na początku.

        static void Main(string[] args)
        {
            Console.WriteLine(10000000.ToString("N0", new NumberFormatInfo { NumberGroupSeparator = " " }));
            Console.ReadLine();
        }

Myślę, że powinienem zrobić jakąś hierarchię klas, bo niektóre metody przydadzą mi się w innych częściach programu np. licząca constructTime, czy surowce. W którym miejscu tworzyć obiekty? Np. Ship deathStar = new Ship(...). Czy da się zrobić tak żeby konstruktor dostawał nazwę kontrolki, zapisywał do zmiennej np. typu string, żeby później metodą Update odświeżyć zmienną z danymi? Czy raczej podawać całą kontrolkę jako object? Kontrolki z których pobierane są dane jako stałe? Jak rozwiązać sytuację, w której obiekty z różnych paneli muszą mieć dostęp do tej samej kontrolki? Statyczną klasą/zmienna jak do tej pory?

Wpierw wyizoluj sobie metody, a potem pomyśl gdzie one pasują. Popatrz na typy danych czy gdzieś nie powinieneś zastąpić np. "int" na coś innego. Może zamiast uint64 własną klasę Time w której będzie Formatted...

Czy można używać tego samego zdarzenia kilka razy? Np. MouseLeave dla każdego buttona jest identyczne.

Możesz.

Button1.Click += click;
Button2.Click += click

0

Jest problem. Jeśli metody są statyczne i nie podaję im wartości przez argument, tylko pobieram z kontrolki, to nie będzie działać, bo kontrolki nie są statyczne. Co z tym zrobić?

EDIT:
Zrobić metody instancyjne i utworzyć instancję klasy GravCalc? Co wtedy ze zmiennymi? Statyczne czy nie?

1 użytkowników online, w tym zalogowanych: 0, gości: 1