Permalänk
Medlem

Kodkritik - (auto)snake

Satt sent en natt och tentaflyktade med att skriva ett litet snake-spel i C, och mindes tråden om kodkritik för en vecka sen. Det skulle vara intressant att få kommentarer på koden, det var ett tag sen jag skrev C sist men jag är personligen väldigt nöjd med resultatet. Det verkar dock finnas en minnesläcka nånstans i autosnake, kanske i snake också.

Spelet finns i två versioner:
snakehttp://pastebin.ca/1750341
autosnakehttp://pastebin.ca/1750354

(Går du på Chalmers? Kör man snake eller man autosnake)

Permalänk
Medlem

är det bara för mig länkarna inte funkar eller?

Permalänk
Citat:

Ursprungligen inskrivet av Dalton Sleeper
är det bara för mig länkarna inte funkar eller?

Japp, det funkar för mig! Önskar man kunde tillräckligt med C för att kunna säga vad som var bra /dåligt

Visa signatur

Asus Striker II Extreme / XFX Geforce GTX 280 / Q9450 @ 3.6GHz/ TRUE Noctua 120/ 4x1GB Corsair TWIN3X2048-1333C9DHX / X25-M G2 80gb Velociraptor / Win 7 Ultimate x64/ Antec P190

MovieDatabase

Permalänk
Medlem

Koden är väll inte ren C iaf, då får man inte ha deklareringar mitt i koden, fast funkar ju om man kör med c++ kompilator

Permalänk
Hedersmedlem
Citat:

Ursprungligen inskrivet av Dalton Sleeper
Koden är väll inte ren C iaf, då får man inte ha deklareringar mitt i koden, fast funkar ju om man kör med c++ kompilator

Jodå, det blev tillåtet med C99.

Permalänk
Medlem

Det kanske blir snyggare/lättare att se om du gör funktioner för ett & annat, tex InitWorm(...) eller liknande.

Citat:

Ursprungligen inskrivet av Elgot
Jodå, det blev tillåtet med C99.

Okey

Permalänk
Medlem

Jag tycker det ser bra ut, inget negativt att kommentera på. Du har väldigt tight kodstil och försvinner inte iväg i massa fluff och abstraktion. Väldigt rakt på sak och löser de problem som behöver lösas.

Visa signatur

void@qnet
teeworlds, stålverk80, evil schemer, c, c++
Languages shape the way we think, or don't.

Permalänk

Detta inlägget gäller snake, autosnake läste jag inte.

Det ser väldigt bra ut. Jag gillar ditt sätt att kommentera på, som en liten rubrik för varje kodstycke.

En sak som jag funderade på var valet av unsigned int på flera ställen. Om det inte finns någon anledning att ha det tycker jag det är snyggare att bara köra på vanliga int:s även om variablen bara får positiva värden. Sen tycker jag det är nästan lite "stroppigt" att skiva "signed int" Jag blir iallafall provocerad

Det verkar som du missat return 0 på slutet... eller är jag blind?
Varför kör du "return INT_MIN" vid fel? Antar att det är smaksak med jag tror dom flesta kör return -1.

På rad 75 har du

if(mvlt == 0 && event.type == SDL_KEYDOWN) {

Jag hade bytt det till

if(mvlt != 0 || event.type != SDL_KEYDOWN) continue;

Bara för att slippa den extra indenteringen.

Som sagt allt detta är ju bara smaksaker men det kan ju vara intressant att höra hur andra tänker...

Permalänk
Hedersmedlem
Citat:

Ursprungligen inskrivet av You
Det verkar dock finnas en minnesläcka nånstans i autosnake, kanske i snake också.

Hur allvarlig är läckan?

Citat:

Ursprungligen inskrivet av jop_the_jopsan
Jag hade bytt det till

if(mvlt != 0 || event.type != SDL_KEYDOWN) continue;

Bara för att slippa den extra indenteringen.

Det här kan dock vara känsligt; ungefär som att rekommendera "goto".

Permalänk
Medlem

Jag tror inte att jag såg någon free nånstans, nån annan som gjorde det?
Dock såg jag någon dynamisk minnesallokering.

Permalänk
Hedersmedlem
Citat:

Ursprungligen inskrivet av Dalton Sleeper
Jag tror inte att jag såg någon free nånstans, nån annan som gjorde det?
Dock såg jag någon dynamisk minnesallokering.

Under körningen används dock realloc, så det borde inte gå åt mer minne än det hade gjort annars (och så många byte rör det sig inte om antar jag).

Permalänk
Medlem

Tack för alla kommentarer!

Citat:

Ursprungligen inskrivet av jop_the_jopsan
Det verkar som du missat return 0 på slutet... eller är jag blind?
Varför kör du "return INT_MIN" vid fel? Antar att det är smaksak med jag tror dom flesta kör return -1.

Har missat return 0 på slutet men det ska nog inte spela nån roll eftersom det är omöjligt att komma dit. Att jag returnerar INT_MIN vid fel beror på att jag i övriga fall returnerar spelarens poäng, så om någon skriptar nån highscorelista så hamnar man längst ner vid fel.

Citat:

Ursprungligen inskrivet av Elgot
Hur allvarlig är läckan?

Det verkar inte finnas nån läcka alls när jag kompilerar på skolans linuxboxar, men på min mac drog den upp från ~10kB till 110MB när jag lät den köra över natten.

Citat:

Ursprungligen inskrivet av Dalton Sleeper
Jag tror inte att jag såg någon free nånstans, nån annan som gjorde det?

Finns ingen free. Borde kanske egentligen försöka köra free med lite andra saker till atexit, som det är just nu så kör den bara SDL_Quit, som bör rensa upp allt som har med SDL att göra.

Permalänk
Medlem
Citat:

Ursprungligen inskrivet av You
Finns ingen free. Borde kanske egentligen försöka köra free med lite andra saker till atexit, som det är just nu så kör den bara SDL_Quit, som bör rensa upp allt som har med SDL att göra.

Om jag inte missminner mig så måste du själv frigöra allokerat minne för surfaces, fonts, ljus osv. SDL_Quit() frigör enbart minnet för den surface som används som huvudfönster.

Sen om jag ska påpeka något på koden så tycker jag om att ha lite tomrum mellan slingor, kontrollstrukturer, funktioner osv. Men det är ju en smaksak.

Permalänk
Medlem

- rader som inte får plats ens på 1920 pixlar
- 8 space för indentering (autosnake)
- är inte en inline vara snyggare än #define för avstånds-funktionen?
- när samma siffra används flera gånger, vore det inte smidigare o ha den i en konstant? typ 16 för allokeringen.
- samla spellogik-variablar på ett ställe (snake speed, start length osv)
- finns en enum för direction, man skulle kunna tänka sig en enum/define för X=0,Y=1 oxå, att använda i tex applepos[X]

jag brukar inte koda C, och vissa av mina kommentarer är mer för diskussion om hur man bör göra än min fasta övertygelse

Visa signatur

AK47s for everyone! - Angry mob
Since NaN /= NaN, I think, we should decipher 'NaN' as 'Not a NaN' - Miguel Mitrofanov
(Varför är människan så benägen att tro på Gud?) Antagligen har det lönat sig och evolutionen har drivit fram sådana hjärnor. - Anon

Permalänk
Medlem
Citat:

Ursprungligen inskrivet av vb
- finns en enum för direction, man skulle kunna tänka sig en enum/define för X=0,Y=1 oxå, att använda i tex applepos[X]

struct coord {int x, y};

annars.

Permalänk
Medlem
Citat:

Ursprungligen inskrivet av jop_the_jopsan
...

En sak som jag funderade på var valet av unsigned int på flera ställen. Om det inte finns någon anledning att ha det tycker jag det är snyggare att bara köra på vanliga int:s även om variablen bara får positiva värden. Sen tycker jag det är nästan lite "stroppigt" att skiva "signed int" Jag blir iallafall provocerad

...

Skriver han endast int så blir det väl ändå en signed int per default?
Då blir det inte bara positiva värden, utan det är unsigned int som endast ger positiva värden.

Visa signatur
Permalänk
Citat:

Ursprungligen inskrivet av Softnux
Skriver han endast int så blir det väl ändå en signed int per default?
Då blir det inte bara positiva värden, utan det är unsigned int som endast ger positiva värden.

Utan att titta i koden så menar väl jopsan att han tycker att det är onödigt att skriva signed int eftersom, precis som du säger, så blir det ju samma sak som bara int.
Sen med unsigned tyckte han att det var onödigt att skriva det då en vanlig int kan användas även om variabeln enbart har positiva värden. Så ni är ju egentligen överens

EDIT: håller dock inte med jopsan. Det finns föredelar med att skriva en int som signed. Då blir det tydligare för den som läser att särskilja. Sen att man skriver unsigned är ju inte onödigt enligt mig. En int som man bara vill ska ha positiva värden ska ju aldrig kunna gå under 0 och om den gör det är det ju fel och då ska programmet signalera det. Annars kan man få mer felsökta problem när konstiga saker man inte tänkt sig händer eftersom att integern är under 0

Visa signatur

Asus Striker II Extreme / XFX Geforce GTX 280 / Q9450 @ 3.6GHz/ TRUE Noctua 120/ 4x1GB Corsair TWIN3X2048-1333C9DHX / X25-M G2 80gb Velociraptor / Win 7 Ultimate x64/ Antec P190

MovieDatabase

Permalänk
Medlem
Citat:

Ursprungligen inskrivet av Softnux
Skriver han endast int så blir det väl ändå en signed int per default?
Då blir det inte bara positiva värden, utan det är unsigned int som endast ger positiva värden.

Det stämmer. Tilläggas kan dock att unsigned är ekvivalent med unsigned int (så man slipper skriva int om man så vill). Vidare anser jag att det är tydligare att använda unsigned när variabeln bara ska anta positiva värden.

Permalänk
Medlem

Anledningen till att jag gjort en så explicit skillnad mellan signed och unsigned är främst för att det ska vara väldigt tydligt vilka värden variabeln i fråga ska kunna ha. Sen kan jag ha ett dubbelt så stort spelfält med unsigned, men det är inte riktigt relevant (snake på INT_MAX*INT_MAX kan nog bli något enformigt).

Permalänk
Medlem

Gällande snake. Nu är ju inte det så mycket kod det handlar om, men jag tycker att det är tydligare med mer uppdelning i funktioner. Så istället för vissa kommentarer du skrivit så kan skapa en funktion. main-metoden skulle då kunna se ut i stil med:

int main() { init(); bool running = false; while (running) { running = key_handler(); if (running) { game_logic(); draw(); } } return 0; }

Fördelen jag ser med en sådan struktur är att man lätt i main-metoden ser flödet i programmet.