Ocena Kodu.

0

Witam, tym razem moje pytanie odnosi się do formatu kodu. I pytanie do was czy można w ten sposób zapełnić komponenty?

Metody:

 // ------------------------- Dla Postaci ---------------------
    
        public void Spadaj()
        {
            FPostac_1.Top += 7;           // Wysokość komponentu "FPostac_1" się zwiększa o 7 jednostek symulując spadanie.
        }

        public void Lec()
        {
            FPostac_1.Top -= 7;
        } 

        public void LecSpadaj()
        {
            if (gora == false)      // jeśli zmienna "gora" przyjmuje wartość "false", postać zacznie spadać.
            {
                Spadaj();           // Metoda odpowiedzialna za spadanie postaci.
            }


            if (gora == true)        // jeśli zmienna "gora" przyjmuje wartość "true", postać zacznie lecieć.
            {
                Lec();               // Metoda odpowiedzialna za wznoszenie się postaci.
            }
        }     // Warunki dla leć i spadaj.

        public void ZderzenieZObiekt1()
        {
            if (((FPostac_1.Top + FPostac_1.Height <= FObiekt_1.Top + FObiekt_1.Height + 3) && (FPostac_1.Top + FPostac_1.Height >= FObiekt_1.Top)) && ((FPostac_1.Left + FPostac_1.Width <= FObiekt_1.Left + FObiekt_1.Width + 3) && (FPostac_1.Left >= FObiekt_1.Left)))
            {
                Zycie--;
                if (Zycie < 1)
                {
                    GameOver();
                }
            }
        }

        public void GameOver()           
        {                              // Kiedy postać zderzy się z obiektem.
            KoniecGry = true;
            FObiekt_1.Top = -793;
            FObiekt_2.Top = -799993;
            MessageBox.Show("Twoje punkty to: " + Punkty.ToString(), "Koniec Gry!");       // game over. 
        }

        public void ObszarRozgrywki()
        {
            if (FPostac_1.Top <= FSwiat_1.Top)
            {
                FPostac_1.Top += 7;                      // Blokada chroniąca przed wylotem postaci po za górny obszar świata gry.
            }

            if (FPostac_1.Top + FPostac_1.Height >= FSwiat_1.Top + FSwiat_1.Height)
            {
                FPostac_1.Top -= 7;                     // Blokada chroniąca przed wylotem postaci po za dolny obszar świata gry.
            }
        }  
public void Timer1()
        {
            LecSpadaj();
            GraSieToczy();
            OdNowaObiekt1();
            WyswietlStatystyki();
            ZderzenieZObiekt1();
            ObszarRozgrywki(); 
        }

Komponent:

        private void FPostac_Tick(object sender, EventArgs e)
        {
            Timer1();
        }

Wypełnienie w ten sposób wydaje mi się dość schludne i uporządkowane, ale nawet nie wiem czy to jest poprawne czy złe myślenie.

0

Mnie tam się podoba twój kod,jest prosto napisany.Sam piszę podobnie projektując architekturę kodu,jednak najlepiej żebyś go przetestował i zobaczył czy takie działanie ci odpowiada.

0
Riw napisał(a):

Mnie tam się podoba twój kod,jest prosto napisany.Sam piszę podobnie projektując architekturę kodu,jednak najlepiej żebyś go przetestował i zobaczył czy takie działanie ci odpowiada.

Właśnie podoba, według mnie jest porządek i jeśli chcę coś zmienić wiem gdzie mam szukać.

4

if (gora == false) // jeśli zmienna "gora" przyjmuje wartość "false", postać zacznie spadać.
{
Spadaj(); // Metoda odpowiedzialna za spadanie postaci.

o, rly? Komentarze w stylu captain obvious. Do usunięcia.

5
if (((FPostac_1.Top + FPostac_1.Height <= FObiekt_1.Top + FObiekt_1.Height + 3) && (FPostac_1.Top + FPostac_1.Height >= FObiekt_1.Top)) && ((FPostac_1.Left + FPostac_1.Width <= FObiekt_1.Left + FObiekt_1.Width + 3) && (FPostac_1.Left >= FObiekt_1.Left)))

Mnie się nie podoba ten if ja bym tam sumował te wartości przed ifem żeby je ciągle nie powtarzać było by czytelniej i w razie zmiany była by mniejsza szansa że człowiek o czymś zapomni, a tak to po dwa razy masz np.: FPostac_1.Top + FPostac_1.Height lub FObiekt_1.Left + FObiekt_1.Width. Do tego jak jest tak długi if przed każdym && dałbym enter jak dla mnie wtedy też jest dużo czytelniej. Do tego nie lubię jak w nazwach zmiennych/obiektów są jakieś cyfry.

7
  1. Notacja węgierska (wszystko zaczyna się od "F").
  2. Podkreślniki w nazwach zmiennych (a może to właściwości?).
  3. Pomieszanie polskich i angielskich nazw.
  4. Nazwy metod praktycznie nie zawierają czasowników, więc nie jest jasne, co one robią. A nazwa Timer1 to majstersztyk.
  5. Kompletnie nieczytelny fragment: if (((FPostac_1.Top + FPostac_1.Height <= FObiekt_1.Top + FObiekt_1.Height + 3) && (FPostac_1.Top + FPostac_1.Height >= FObiekt_1.Top)) && ((FPostac_1.Left + FPostac_1.Width <= FObiekt_1.Left + FObiekt_1.Width + 3) && (FPostac_1.Left >= FObiekt_1.Left))) - to powinno być w oddzielnej metodzie, np. w klasie Postać.
7

Magiczne liczby - 7, 793 i takie tam. Zmień sobie na stałe, obecnie niezbyt wiadomo dlaczego używasz akurat takich wartości.

5
 
public void LecSpadaj()
        {
            if (gora == false)      // jeśli zmienna "gora" przyjmuje wartość "false", postać zacznie spadać.
            {
                Spadaj();           // Metoda odpowiedzialna za spadanie postaci.
            }
 
 
            if (gora == true)        // jeśli zmienna "gora" przyjmuje wartość "true", postać zacznie lecieć.
            {
                Lec();               // Metoda odpowiedzialna za wznoszenie się postaci.
            }
        } 

moze zamiast tego to RuchPionowy? czy jezeli chcesz po angielsku to masz VerticalMove?
na cholere Ci takie ify?
nie lepiej

if (gora)
{
  Spadaj();
}
else
{
  Lec();
} 

a dla niektorych to jest za duzo lini wiec mozesz napisac nawet tak (i nadal to jest czytelne, ale nie polecam tego sposobu, ze wzgledu na debugowanie)

if (gora)  Spadaj();
else Lec(); 

baaa mozesz to zrobic operatorem trojargumentowym (i bedziesz mial w jednej linii) jednak nadal poleacm pierwszy sposob ktory napisalem

0
fasadin napisał(a):

baaa mozesz to zrobic operatorem trojargumentowym

No raczej nie moze - operator trojargumentowy wymaga aby drugi i trzeci argument byly wartosciami (typow, miedzy ktorymi istnieje mozliwosc konwersji) a jego metody Lec i Spadaj nic nie zwracaja :)

0

Co do ilości kodu ta wersja:

if (gora)
{
  Spadaj();
}
else
{
  Lec();
} 

jest dużo lepiej czytelna od tej:

if (gora)  Spadaj();
else Lec(); 
0
wojas666 napisał(a):

Co do ilości kodu ta wersja:

if (gora)
{
  Spadaj();
}
else
{
  Lec();
} 

jest dużo lepiej czytelna od tej:

if (gora)  Spadaj();
else Lec(); 

A ja zapisuje tak:

if (gora)
	Spadaj();
else 
	Lec(); 

u już lepiej. Przy jednej linijce nigdy nie daje klamerek. zamiast plusów w messagebox mozesz uzyc string.Fromat albo Concat i juz lepiej wyglada. Nazwy metod, zmiennych i wszystkiego najlepiej po angielsku, i jak juz to trzymaj sie tego, ze np zaczynasz wszystko z duzych liter. Przed zmiennymi/metodami mozesz uzyc this./base. , zalezy. Jak komentujesz metode, to przez metoda wpisz /// i bedziesz mial miejsce do opisania dzialania.

0

No jeszcze można od innej strony:

enum VerticalMove
{
   Up = -7,
   Down = 7
}

var verticalMove = VerticalMove.Up;
character.Top += (int)verticalMove;
3

Oprócz tego, co wymieniono wyżej: mam wrażenie, że próbujesz pisać w c# strukturalnie (jak w czystym c). Prawdopodobnie masz jedną wielką klasę-worek, do którego wrzuciłeś wszystkie metody (leć, spadaj, gameover, timer - zupełnie rózne rzeczy). Oprócz tego, masz jednocześnie publiczne metody Leć, Spadaj i LećLubSpadaj (inne nazwy, ale wiadomo, o co chodzi). Powinieneś mieć albo te dwie pierwsze - i zewnętrzny obiekt, który je wywołuje, wybiera kierunek; albo tylko tę drugą - i obiekt sam decyduje, czy ma to być góra czy dół. Podsumowując, podziel to wszystko jakoś logicznie na klasy (Postać, Plansza, Gra itp.).

0

Ja bym zamiast VerticalMove dał VerticalPosition, bo zmiana współrzędnych (+7 lub -7) bardziej kojarzy mi się ze zmianą pozycji niż z ruchem.

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