Proźba o ocenę kodu i sugestie w celu jego poprawienia

Piszę sobie grę. A dokładnie przenoszę grę Manewry morskie do świata wirtualnego. (Instrukcja do gry: http://instrukcja.pl/i/gra_manewry_morskie).

Chciałbym się dowiedzieć Co można w kodzie poprawić, co wyrzucić, co dodać a co jest dobrze zrobione. Zdaje sobie sprawę z tego że kod idealny nie jest, a wręcz może zawierać masę błędów i dlatego piszę tego posta.

Kod ma 2865 linii więc cierpliwość wymagana :P Jeśli nie chce Ci się czytać kodu a masz zamiar pisać posty typu "Co to za badziew", "Nie chciało mi się czytać ale pewnie i tak jest wszystko źle" to sobie daruj bo tracisz tylko czas. Konstruktywna krytyka mile widziana.

Pisząc tą grę używałem synapse (http://www.ararat.cz/synapse/doku.php/start)
oraz WinGraph (http://math.ubbcluj.ro/~sberinde/wingraph/)

Co do grafiki tej gry to wiem że jest to archaiczna metoda i uczę się OpenGL by grafikę zrobić właśnie w OpenGL.

Czego jeszcze nie zrobiłem i co jest do poprawienia:

• Błędy podczas połączenia z drugim graczem.
• Pola neutralne działają jak każde inne pole.
• Do portu można wpłynąć przez każde jego pole.
• Grafika
• Przeniesienie z FPC IDE do Lazarusa.
• Serwer kierujący rozgrywką (matchmaking, statystyki itp.).

Kod w załącznikach.

Moduł Draw.pas, procedura DrawMiniImage:

  If PosX = 1 Then x := 810;
  If PosX = 2 Then x := 821;
  If PosX = 3 Then x := 832;
  If PosX = 4 Then x := 843;
  If PosX = 5 Then x := 854;
  If PosX = 6 Then x := 865;
  If PosX = 7 Then x := 876;
  If PosX = 8 Then x := 887;
  If PosX = 9 Then x := 898;
  If PosX = 10 Then x := 909;
  If PosX = 11 Then x := 920;
  If PosX = 12 Then x := 931;
  If PosX = 13 Then x := 942;
  If PosX = 14 Then x := 953;
  If PosX = 15 Then x := 964;
  If PosX = 16 Then x := 975;
  If PosX = 17 Then x := 986;
  If PosX = 18 Then x := 997;

  If PosY = 1 Then y := 10;
  If PosY = 2 Then y := 21;
  If PosY = 3 Then y := 32;
  If PosY = 4 Then y := 43;
  If PosY = 5 Then y := 54;
  If PosY = 6 Then y := 65;
  If PosY = 7 Then y := 76;
  If PosY = 8 Then y := 87;
  If PosY = 9 Then y := 98;
  If PosY = 10 Then y := 109;
  If PosY = 11 Then y := 120;
  If PosY = 12 Then y := 131;

czy Ty się dobrze czułeś, jak wklepywałeś tą drabinkę...? Nie wystarczy Ci prosty wzór na obliczanie tego...? Oj, ktoś tutaj nie pomyślał :P

X := 810 + (PosX * 11) - 11;
Y := 10 + (PosY * 11) - 11;

Mam nadzieję, że nie pomyliłem się; Jeżeli PosX może mieć wartość poza przedziałem [1 .. 18], oraz PosY poza [1 .. 12] - skorzystaj ze zbiorów by to zabezpieczyć:

if PosX in [1 .. 18] then
  X := 810 + (PosX * 11) - 11;

if PosY in [1 .. 12] then
  Y := 10 + (PosY * 11) - 11;

I pozbędziesz się tych koszmarnych powtórzeń... Reguła DRY się kłania :)

Uwagi cd.kodu:

1.Niepotrzebnie robisz spację pomiędzy identyfikatorem a nawiasem (foo ()), utrudnia to nieco czytanie kodu.
2.Don't Repeat Yourself! Spójrz na procedurę LoadImages czy DrawImage - praktycznie cały czas powtarzasz tam jeden fragment kodu...
3.

  If PosX = 1 Then x := 810;
  If PosX = 2 Then x := 821;
  If PosX = 3 Then x := 832;
  If PosX = 4 Then x := 843;
  If PosX = 5 Then x := 854;
  If PosX = 6 Then x := 865;
  If PosX = 7 Then x := 876;
  If PosX = 8 Then x := 887;
  If PosX = 9 Then x := 898;
  If PosX = 10 Then x := 909;
  If PosX = 11 Then x := 920;
  If PosX = 12 Then x := 931;
  If PosX = 13 Then x := 942;
  If PosX = 14 Then x := 953;
  If PosX = 15 Then x := 964;
  If PosX = 16 Then x := 975;
  If PosX = 17 Then x := 986;
  If PosX = 18 Then x := 997;

Nie mam pojęcia czy wartości x-ów biorą się z jakiegoś wzoru; jeżeli nie - lepiej jest wrzucić to do tablicy.

Const
  Pancernik   : Byte = 1;
  Rakietowy   : Byte = 2;
  Krazownik   : Byte = 3;
  Niszczyciel : Byte = 4;
  Podwodny    : Byte = 5;
  Eskortowiec : Byte = 6;
  Tralowiec   : Byte = 7;
  Desantowy   : Byte = 8;
  Bateria     : Byte = 9;
  Mina        : Byte = 10;

Po to wymyślono enum'y, aby takich udziwnień nie robić.

5.{Wersja alfa 1.0.0.0} :P
1.0 będzie gdy ukończysz większość z listy rzeczy do zrobienia...
Numerowanie zaczyna się od 0.1 wzwyż.

Uses Ships,Fields,Worlds,Connection,Draw,blcksock,WinGraph,WinCrt;

Wygodniej i estetyczniej jest, gdy moduły są pooddzielane przecinkami wraz ze spacją (Ships, Fields, Worlds {...}).
Jest to bardziej czytelne.

    Begin
      BMap[x,y].ShipId := World^.Field[x,y]^.GetShipId;
      BMap[x,y].ShipType := World^.Field[x,y]^.GetShipType;
      BMap[x,y].Player := World^.Field[x,y]^.GetFieldPlayer;
    end;

Tutaj może nie jest to jakimś specjalnie dużym błędem, ale gdy tych wartości byłoby więcej, zalecane jest użycie konstrukcji With.

Until (Key = '1') or (Key = '2');

Lepiej jest zapisać:

Until (Key in ['1', '2']);

i := i + 1;

I += 1 lub chociaż Inc(I)...

  If PosX = 1 Then x := 0;
  If PosX = 2 Then x := 100;
  If PosX = 3 Then x := 200;
  If PosX = 4 Then x := 300;
  If PosX = 5 Then x := 400;
  If PosX = 6 Then x := 500;
  If PosX = 7 Then x := 600;
  If PosX = 8 Then x := 700;

Mnożenie stało się zbyt mainstreamowe czy co?

11.W funkcji DrawInfo ładniej wyglądałoby case, niżeli drabinka if'ów imho, a najlepiej jakaś tablica.

Until (Check = 255) or (check = 254);

Patrz punkt ósmy.

Until (Key = 't') or (Key = 'T') or (Key = 'n') or (Key = 'N');

LowerCase + konstrukcja z punktu ósmego.

                   If ShipType = Pancernik Then Speed := 2;
                   If ShipType = Rakietowy Then Speed := 1;
                   If ShipType = Krazownik Then Speed := 2;
                   If ShipType = Niszczyciel Then Speed := 4;
                   If ShipType = Podwodny Then Speed := 2;
                   If ShipType = Eskortowiec Then Speed := 3;
                   If ShipType = Tralowiec Then Speed := 2;
                   If ShipType = Desantowy Then Speed := 1;
                   If ShipType = Bateria Then Speed := 0;
                   If ShipType = Mina Then Speed := 0;

Tablica, tablica, tablica...

Until (MoveDone = True)

Niepotrzebne = True.

16.ShipPuted.
No ja jestem słaby z angielskiego, ale forma przeszła put to put, nie putted :P

Moduł Draw.pas, procedura DrawImage (nie podaję całego kodu bo jest strasznie długi, trochę go sformatowałem...):

If (PosX = 1) and (PosY = 1) Then
  Begin
    X := 0;
    Y := 0;
    SetViewPort(X, Y, X + 99, Y + 99, ClipOff);
  end;

  If (PosX = 2) and (PosY = 1) Then
  Begin
    x := 100;
    y := 0;
    SetViewPort(X, Y, X + 99, Y + 99, ClipOff);
  end;

  If (PosX = 3) and (PosY = 1) Then
  Begin
    x := 200;
    y := 0;
    SetViewPort(X, Y, X + 99, Y + 99, ClipOff);
  end;

  If (PosX = 4) and (PosY = 1) Then
  Begin
    x := 300;
    y := 0;
    SetViewPort(X, Y, X + 99, Y + 99, ClipOff);
  end;

  If (PosX = 5) and (PosY = 1) Then
  Begin
    x := 400;
    y := 0;
    SetViewPort(X, Y, X + 99, Y + 99, ClipOff);
  end;

{...}

Ty naprawdę lubisz sobie utrudniać życie... Wiesz co możesz zrobić, żeby uniknąć 288 linii na rzecz dosłownie 4 (w sumie trzech, bo wolna linia nie jest wymagana)...? Kolejny wzór:

X := (PosX - 1) * 100;
Y := (PosY - 1) * 100;

SetViewPort(X, Y, X + 99, Y + 99, ClipOff);

Widzisz, jakie to proste...? Myśl, bo wiele procedur można znacznie skrócić; Nie wymienię Ci wszystkich, ale sam zauważysz pewne zależności i wykorzystasz choćby wzory, już nie pisząc o pętlach czy odpowiednich typach;

If ShipType = Pancernik Then
Begin
SName := 'Pancernik';
Speed := '2';
Range := '2';
Spec := '';
end;
If ShipType = Rakietowy Then
Begin
SName := 'Okret rakietowy';
Speed := '1';
Range := '1';
Spec := 'Niszczy baterie nabrzerzne';
end;
[...]

Tu powinno być: 1. ELSE - dzięki temu nie będziesz niepotrzebnie porównywać warunków gdy już jeden pasuje. Jeszcze lepszym rozwiązaniem jest CASE. I Czemu Speed i Range jest stringiem?

Poza tym twój protokół komunikacyjny jest beznadziejny. Ja bym to napisał na podstawie komend, czyli wysyłasz stringi w stylu: "Move 1 3"#13#10 (CRLF do oddzielania komend). Wtedy można chat dodać czy coś prosto. Już nie mówiąc o lepszym zarządzaniu połączeniem.

Dodatkowo, powinieneś kod przepisać na klasy, procedury sprawdzają się przy naprawdę prostych programach.

Nie rozumiem też tego dlaczego chcesz wszystko pisać samemu w OpenGLu. W takim wypadku dlaczego użyłeś chociażby Synapse? Lepiej jest pisać wysokopoziomowo niż w czystym OpenGLu.

Patryk27 napisał(a)

{Wersja alfa 1.0.0.0}
1.0 będzie gdy ukończysz większość z listy rzeczy do zrobienia...
Numerowanie zaczyna się od 0.1 wzwyż.

To zależy jak kto numeruje. Pozwól mu wybrać swoje numerowanie.

Connection napisał(a)

For i := 1 to 5 Do
If Server.CanRead (2000) Then
Server_cl.Socket := Server.Accept;

Serio? Napisz swój system komunikacji który będzie ci zarządzać połączeniami z wygodnym API.

Generalnie kod jest słaby ale jak na newbie jest ok, nawet ponadprzeciętnie ;) .

Poza tym, istnieje inny dział do oceniania programów.

Moduł Draw, procedura ChangePage:

Procedure ChangePage;
Begin
  If VPage = 0 Then
  Begin
    VPage := 1;
    APage := 0;
  end
  else
  Begin
    VPage := 0;
    APage := 1;
  end;
  SetVisualPage (VPage);
  SetActivePage (APage);
  ClearDevice;
end;

skrócić ten kod można do takiej postaci:

procedure ChangePage;
begin
  SetVisualPage(Integer(VPage = 0));
  SetActivePage(Integer(APage = 1));
  ClearDevice;
end;

Moduł Draw, procedura LoadImages (całego nie wklejam):

  Assign (BitMap,'models\water.bmp');
  Reset (BitMap);
  Seek (BitMap,54);
  For Color := 0 to 255 Do
  Begin
    Read (BitMap,b,g,r,tmp);
    SetRGBPalette (Color,r,g,b);
  end;
  For y := 99 downto 0 Do
    For x := 0 to 99 Do
    Begin
      Read (BitMap,Color);
      PField[2,x,y] := Color;
    end;
  Close (Bitmap);

  Assign (BitMap,'models\Land.bmp');
  Reset (BitMap);
  Seek (BitMap,1078);
  For y := 99 downto 0 Do
    For x := 0 to 99 Do
    Begin
      Read (BitMap,Color);
      PField[1,x,y] := Color;
    end;
  Close (Bitmap);

  Assign (BitMap,'models\Neutral.bmp');
  Reset (BitMap);
  Seek (BitMap,1078);
  For y := 99 downto 0 Do
    For x := 0 to 99 Do
    Begin
      Read (BitMap,Color);
      PField[3,x,y] := Color;
    end;
  Close (Bitmap);

{...}

Przede wszystkim:

• stwórz sobie macierz zawierającą nazwy plików do odczytu grafik,

• nie wczytuj bajt po bajcie w zagnieżdżonych pętlach bo będzie to strasznie wolne (każda grafika ma 10 000 bajtów...), lepiej odczytuj od razu cały blok do macierzy;
  Możesz także napisać procedurę, której argumentem będą:

• nazwa pliku,

• offset grafiki w pliku,

• macierz docelowa (przekazana przez referencję),

• pierwszy indeks w tej macierzy,
  dzięki temu będziesz mógł zaprogramować raz wczytywanie grafiki do macierzy i wywołać tyle razy, ile plików z grafikami, poza tym wczytywanie będzie szybsze niż bawienie się z każdym bajtem;

Na wstępie chcę zaznaczyć, że nie znam w ogóle Pascala, ale widząc procedurę Game, która ma 666 linijek jednak powiem, żebyś na razie dał sobie spokój. Pisząc w ten sposób za pięć dni nie będziesz wiedział co się dzieje w kodzie (o ile jeszcze wiesz :)). Poczytaj o programowaniu obiektowym i zasadach pisania dobrego kodu. Stracisz dużo czasu na pisaniu tego potwora, a można to zrobić zdecydowanie lepiej i 20 razy krócej.

Dzięki ale nie skorzystam i będę pisał dalej.

Przyznam @Oak rację, powinieneś podzielić kod na mniejsze procedury, klasy itp. Natomiast nie widzę powodów abyś dał sobie spokój, bo kod można poprawić. I właśnie ponieważ pytasz to jesteśmy w stanie Cię trochę nakierować i pomóc w przeciwdziałaniu temu co się z twoim kodem dzieje.
Jeżeli chcesz poprawiać wszystko w kodzie, to poprawiaj. Natomiast ja zazwyczaj kończę kod 'w stary fason' doświadczenia zachowując na przyszły kod (no chyba że chodzi o coś co nie wymaga większych zmian). Tobie też polecę nieprzejmowanie się zbytnio tą grą natomiast używanie dobrych praktyk w przyszłości (nie wypracujesz tego od razu, ja ciągle uczę się pisać jeszcze lepszy kod).

nie wczytuj bajt po bajcie w zagnieżdżonych pętlach bo będzie to strasznie wolne (każda grafika ma 10 000 bajtów...), lepiej odczytuj od razu cały blok do macierzy;

Kolega miał na myśli BlockRead. Warto poczytać bo dzięki temu przyśpieszysz czytanie tysiące razy co najmniej.

Tak jak pisałem w pierwszym poście założenie jest takie by przenieść to wszystko na Lazarusa. Dlatego chce się dowiedzieć co mogę poprawić, a co napisać inaczej ponieważ kolejną wersję gry będę pisał od początku i chcę by pisanie tego nie było takie ciężkie jak tej wersji i by kod był mniej skomplikowany oraz bardziej przejrzysty.

I pytanko dotyczące serwera. Co bardziej byście polecili:

1. Serwer tylko przechowuje dane odnośnie gry i rozsyła odpowiednim graczom a wszystkie wyliczenia odbywają się na kliencie.
2. Serwer wykonuje obliczenia i wysyła wyniki do klienta na podstawie których klient aktualizuje stan gry.

babubabu napisał(a):

I pytanko dotyczące serwera. Co bardziej byście polecili:

1. Serwer tylko przechowuje dane odnośnie gry i rozsyła odpowiednim graczom a wszystkie wyliczenia odbywają się na kliencie.
2. Serwer wykonuje obliczenia i wysyła wyniki do klienta na podstawie których klient aktualizuje stan gry.

Ja polecę 3: Zarówno serwer jak i klient znają mechanikę gry i nawzajem walidują swoje ruchy. To się chyba najlepiej sprawdza w przypadku lokalnych serwerów gdyż serwer ani klient nie mogą czitować.

Ale serwer nie ma być lokalny. Serwer ma być globalny do którego się będą podłączać gracze. Serwer będzie według odpowiednich statystyk dobierał graczy na podobnym poziomie i łączył ich do rozgrywki. Dlatego jestem za opcją numer 2 bo utrudni to czitowanie. Oczywiście dla chcącego nie ma nic trudnego i na pewno znajdzie się jakiś wariat co będzie chciał grę oszukać ale myślę, że sposób 2 trochę mu to utrudni. Komunikację Klient-Serwer mam zamiar rozwiązać w sposób komend np:
Movex1y1x2y2 i serwer sprawdza czy można, jeśli tak, przenosi co trzeba i wysyła zaktualizowaną mapę do dwóch graczy.

Jednak na razie muszę jeszcze dokładnie ogarnąć OpenGL (przezroczystość tekstury,tekst i czcionki) oraz wielowątkowość (modyfikowanie jednej zmiennej przez kilka wątków). Jeszcze z OpenGL nie powinienem mieć problemu (przynajmniej tak sądzę) tak z wątkami już mam bo przeczytałem kilka tutoriali i kilka przykładowych programów a i tak mam problem :/

I przepraszam za zły dział.

Komunikację Klient-Serwer mam zamiar rozwiązać w sposób komend np:
Movex1y1x2y2 i serwer sprawdza czy można, jeśli tak, przenosi co trzeba i wysyła zaktualizowaną mapę do dwóch graczy.

???

Nie tędy droga... Serwer nie może działać w ten sposób, to gra zainstalowana na dysku użytkownika ma znać reguły gry zakres ruchów, nie serwer; Serwer ma służyć jedynie za "pośrednika" w transportowaniu pakietów z danymi ruchów; Jeśli użytkownik poda zły ruch - to aplikacja na dysku ma mu to uświadomić, nie serwer;

Na serwerze możesz przechowywać statystyki graczy, ale to aplikacja kliencka ma pobierać z niej informacje o graczach i według nich ustalać sobie przeciwnika (tak działają gry np. z WP, gdzie na dysku instalujesz klienta, który pobiera informacje z bazy i przedstawia Ci w programie); Dzięki temu zakładając stół masz możliwość przeglądania listy graczy i stołów w danym pokoju;

Po skończonej grze aplikacja z dysku będzie aktualizować stan bazy danych, by zmienić tym samym ranking;

Oczywiście dla chcącego nie ma nic trudnego i na pewno znajdzie się jakiś wariat co będzie chciał grę oszukać ale myślę, że sposób 2 trochę mu to utrudni.

Daj spokój, każdą grę da się oszukać; Łatwiej czy trudniej, ale się da;

I przepraszam za zły dział.

To nie do końca zły dział; Co z sprawdzenia i poprawienia to dobry, co do oceny programu już nie bardzo - kwestia sporna;

Nie tędy droga... Serwer nie może działać w ten sposób, to gra zainstalowana na dysku użytkownika ma znać reguły gry, nie serwer; Serwer ma służyć jedynie za "pośrednika" w transportowaniu pakietów z danymi ruchów; Jeśli użytkownik poda zły ruch - to aplikacja na dysku ma mu to uświadomić, nie serwer;

Na serwerze możesz przechowywać statystyki graczy, ale to aplikacja kliencka ma pobierać z niej informacje o graczach i według nich ustalać sobie przeciwnika (tak działają gry np. z WP, gdzie na dysku instalujesz klienta, który pobiera informacje z bazy i przedstawia Ci w programie); Dzięki temu zakładając stół masz możliwość przeglądania listy graczy i stołów w danym pokoju;

Po skończonej grze aplikacja z dysku będzie aktualizować stan bazy danych, by zmienić tym samym ranking;

Nie wiem czy ten post jest pełen ironii, ale jeżeli nie to jest to największa głupota jaką dzisiaj przeczytałem.

Nie wiem czy ten post jest pełen ironii, ale jeżeli nie to jest to największa głupota jaką dzisiaj przeczytałem.

Po części się zgodzę. Nie mogę wyjść ze zdziwienia że takie głupoty usłyszałem od tak szanowanego użyszkodnika forum.

Po skończonej grze aplikacja z dysku będzie aktualizować stan bazy danych, by zmienić tym samym ranking;

No cheats possible! Lets send "Stats-update 9999 9999 9999".

Daj spokój, każdą grę da się oszukać; Łatwie czy trudniej, ale się da;

Jeżeli bym napisał protokół do gry w kamień papier nożyce gdzie literki: K/M/P - ruch W-czekaj na odpowiedź serwera +/- - wygrana/przegrana to jak byś go złamał. Klient jest zawodną stroną, serwer w teorii niezawodną. Są różne ataki, ale ich sukces zależy od błędów w serwerze. A błędy na serwerze wynikają najczęściej z głupoty adminów.

gra zainstalowana na dysku użytkownika ma znać reguły gry zakres ruchów, nie serwer;

A czemu nie obaj.

Serwer ma służyć jedynie za "pośrednika" w transportowaniu pakietów z danymi ruchów

I co ma zrobić drugi użytkownik jak inny klient mu wysyła niepoprawny ruch? Płakać do serwera że przeciwnik oszukuje? To wtedy co jak klient zacznie płakać mimo poprawnych ruchów?

Jeśli użytkownik poda zły ruch - to aplikacja na dysku ma mu to uświadomić, nie serwer;

A co jak ulepszę aplikację bo mi reguły nie pasują?
Tak działa SAMP, klient sprawdza czy go pocisk uderzył. Wiesz jak łatwo jest zrobić godmode? Właściwie tak łatwo jak do single player. Już o pływaniu w powietrzu, przenikaniu przez ściany, dostawaniu broni z powietrza, samochodami zachowującymi się jak helikoptery albo pływającymi w ziemi nie wspomnę. Gdybyś tworzył grę online to z całą pewnością nie miałbyś żadnych cheaterów.

Na serwerze możesz przechowywać statystyki graczy, ale to aplikacja kliencka ma pobierać z niej informacje o graczach i według nich ustalać sobie przeciwnika

Pobierzmy informację o 10000000 graczach, przecież to szybciej niż serwer wyszukujący przeciwnika. Już nie mówiąc o możliwościach manipulacji.

Dzięki temu zakładając stół masz możliwość przeglądania listy graczy i stołów w danym pokoju;

To nie wystarczy pobrać listę nicków i stołów wraz z odpowiadającymi nickami powiedzmy że na danym "pokoju"? A potem gdy zechcemy żeby nam wyszukało wroga, żeby zajął się tym serwer? Tak to musisz pobrać statystyki wszystkich graczy.

Aby wyjaśnić wszelkie nieścisłości: serwer jest bezpieczny, klient nie. Klient powinien być traktowany tak, jakby pisał go scrippting kiddie który na pewno coś wymyśli. Serwer powinien walidować dane z klienta i wysyłać do drugiego klienta. Jeżeli serwer wykryje nieprawidłowość to najlepiej od razu się rozłączyć i zbanować IP (masa ataków bazuje na niezdefiniowanym statusie serwera po błędzie). Obowiązują tutaj zasady podobne przy pisaniu stron WWW, czyli traktowanie wszystkich użytkowników jako potencjalnych chakierów i walidowaniu wszystkiego co się da.