Black Jack-spel

[Neonii]

Hej!

Bad om hjälp för ett par dagar om att få till en bra slumpgenerator för mitt Black Jack-spel som jag nu har gjort i C++.
Även om slumpgeneratorn inte verkar vara helt 100% bra, så är nu programmet i vilket fall klart, och det vore skitskoj med lite kritik på hur det är gjort, hur jag har kodat det och allmänt vad jag kan förbättra.

Det var ett par år sedan jag höll på med C++, så det mesta är gjort i while-for-if-else-satser, med struct för att lagra all data. (har glömt bort vector och list helt

Anyway, enjoy!

Kod: Markera allt

http://www.filefactory.com/file/a01882d/n/Black_Jack_txt

Ps. Borde ha gjort en funktion av att läsa in korten när dom slumpats, blev ett jäkla stök där : /


Ps.2 Slumpgenerator-snutten:
slump = Min_Varde + ((double)(Max_Varde - Min_Varde + 1.0) * ((double)rand()) / ((double)RAND_MAX + ((double)1.0)));

Vad kan vara fel?

MVH. Neonii

Edit: Typo

[DrMegahertz]

2347 rader, för ett blackjack?

Den största förbättringen du skulle kunna göra är att inte upprepa dig i källkoden som du gör nu(se DRY). Det första jag lägger märke till är att du har 12 switch-satser som gör exakt samma sak lite här och var i koden. Det du borde göra här är att du bryter ut switch-satsen till en funktion istället, dit du kan skicka ett kort och få en "mänsklig" representation utskriven. På så sätt så gör du din kod mycket enklare att underhålla och du slipper skriva lika mycket kod för att utföra uppgiften. Samma sak gäller för funktionerna enSpelare() och tvaSpelare(), det borde bara krävas en funktion, som är oberoende av antalet spelare.

Sammanfattat; försök att undvika upprepning genom att dela upp koden i mindre funktioner. Håll funktionerna korta(!) och enkla, de ska utföra en enda uppgift, så bra som möjligt!

[Jarulf]

Du "måste" även ha ett default-fall i switch-satser. God sed är det iallafall.

Kod: Markera allt

switch(a)
{
    case 1:
        cout <<"hej";
        break;

    case 2:
       cout <<"hej2";
       break;

    default:
       cout <<"Fel";
       break;
}

[Neonii]

Okej, så hellre en massa funktioner som utför alla de saker som måste hända många gånger än upprepningar?
Det låter logiskt nu när ni säger det. Man blir nog lite egenblind, i och med att man själv har full koll på vart allt ligger och vad som händer!

Någonting annat som behövs tilläggas?

[DrMegahertz]

Okej, så hellre en massa funktioner som utför alla de saker som måste hända många gånger än upprepningar?
Det låter logiskt nu när ni säger det. Man blir nog lite egenblind, i och med att man själv har full koll på vart allt ligger och vad som händer!

Någonting annat som behövs tilläggas?

Just precis, på så sätt blir det mycket enklare att underhålla och förstå programmet. Det blir dessutom lite mindre, och du får det klart snabbare. Bara fördelar alltså.

Låt oss ponera att du skulle vilja använda engelska beteckningar på korten, alltså; "Jack, Queen, King, Ace" istället för "Knekt, Dam, Kung, Ess". Som det ser ut nu så skulle behöva ändra i alla 12 switchsatserna, men om du hade haft en funktion med den här switchsatsen i så hade du bara behövt ändra på ett enda ställe för att ändringen skulle få effekt på alla platser i koden.

Därefter är det bara lite smågrejjer som behöver påpekas;

• Du har en struktur för varje spelare, alltså två olika typer för samma sak, borde inte behövas.
• Du skickar för många funktionsargument som referenser, enkla typer såsom char och int tjänar du inget på att skicka in som referenser(tvärt om), såvida du inte måste returera fler värden än ett.
• Globala variabler är generellt ett stort "no-no", jag syftar på variablerna 'kort1' och 'kort2'.
• kort1[999], kort2[999], overSkrivs[999] etc. This is madness! Försök att inte använda mer minne än vad du behöver, kika på dynamisk minnesallokering, alternativt std::vector för att undvika huvudvärk!
• cout << "\e[H\e[2J"; Detta har jag aldrig sett förut, men jag antar att den tömmer skärmen? '\e' följer inte standarden och bör undvikas.
• På vissa ställen ser jag att du gör dessa jämförelser; kort1.namn == "Knekt", kort1.namn == "Dam" osv. Borde du inte kunna jämföra mot ett heltalsvärde istället? t.ex. kort1.varde == 11, kort1.varde == 12, såna jämförelser är mycket, mycket snabbare!

När dina projekt växer sig över några hundra rader kan det också vara rätt najs att dela upp projektet i flera källfiler, för att dra ner kompileringstiden efter att du ändrat på något. Det blir också lite enklare att överblicka vilka delar som ditt projekt består av.

Happy coding!

[Neonii]

WoW, konstruktiv kritik! <3

- Jag kommer definitivt att göra fler funktioner i framtiden, som ni säger så är det ju mycket lättare att göra ändringar då vid behov, dels för mig själv och dels för andra!

Du har en struktur för varje spelare, alltså två olika typer för samma sak, borde inte behövas.

Mja, vet inte riktigt hur jag ska komma runt det här faktiskt, iofs så är jag inte jätteduktig kodare (än ), så jag antar att alternativa lösningar kommer fram då jag uppdaterar mig på vector och list igen. (Och allt bortom det för den delen!)

Du skickar för många funktionsargument som referenser, enkla typer såsom char och int tjänar du inget på att skicka in som referenser(tvärt om), såvida du inte måste returera fler värden än ett.

Förstår inte riktigt vad du menar här De referenser jag skickar med till de olika funktionerna behövs (såvitt jag har tänkt i alla fall, antar du syftar på funktionen nyttKort() för att slumpa fram ett nytt kort) för att hålla räkning på:
a) vilken spelare det är som anropar ett kort för att kunna tilldela korten rätt. (spelareId)
b) hur många kort respektive spelare har dragit. (antalKort1 & antalKort2)

Globala variabler är generellt ett stort "no-no", jag syftar på variablerna 'kort1' och 'kort2'.

Varför? Har det med minnesanvändning att göra?

kort1[999], kort2[999], overSkrivs[999] etc. This is madness! Försök att inte använda mer minne än vad du behöver, kika på dynamisk minnesallokering, alternativt std::vector för att undvika huvudvärk!

Alright, ska tänka på detta i framtiden! I större program kan jag tänka mig att det är onödigt att göra plats för mer variabler än det möjligtvis kan behövas plats för. Det där med dynamisk minnesallokering hanterar ju vector och list väldigt bra, får se till att jag läser upp dom igen

cout << "\e[H\e[2J"; Detta har jag aldrig sett förut, men jag antar att den tömmer skärmen? '\e' följer inte standarden och bör undvikas.

Återigen, what? Är det själva principen att inte tömma skärmen på text, eller är det själva funktionen jag använt för att göra det som är fel? I så fall, vad kan jag skriva istället?

Utanför ämnet: Fick tipset att tömma skärmen på detta sätt av den här posten som jag startade.

På vissa ställen ser jag att du gör dessa jämförelser; kort1.namn == "Knekt", kort1.namn == "Dam" osv. Borde du inte kunna jämföra mot ett heltalsvärde istället? t.ex. kort1.varde == 11, kort1.varde == 12, såna jämförelser är mycket, mycket snabbare!

Mja, i och med att knektar, kungar och damer (och till och med 10:or) har samma värde, alltså 10 poäng, så funkar det inte skulle jag tro

och slutligen:

När dina projekt växer sig över några hundra rader kan det också vara rätt najs att dela upp projektet i flera källfiler, för att dra ner kompileringstiden efter att du ändrat på något. Det blir också lite enklare att överblicka vilka delar som ditt projekt består av.

Flera källfiler? Menar du att till exempel använda "ofstream" för att skriva data till en separat textfil som jag sedan anropar inifrån programmet? Borde inte det i så fall ta upp mer minnesanvändning av att behöva göra det hela tiden?

Tackar för all konstruktiv kritik, man lär sig av sina misstag

/Kristoffer

[Inhuman Soul]

Nu har jag inte läst igenom hela källkoden men jag ska försöka svara lite ändå.

Du skickar för många funktionsargument som referenser, enkla typer såsom char och int tjänar du inget på att skicka in som referenser(tvärt om), såvida du inte måste returera fler värden än ett.

Förstår inte riktigt vad du menar här De referenser jag skickar med till de olika funktionerna behövs (såvitt jag har tänkt i alla fall, antar du syftar på funktionen nyttKort() för att slumpa fram ett nytt kort) för att hålla räkning på:
a) vilken spelare det är som anropar ett kort för att kunna tilldela korten rätt. (spelareId)
b) hur många kort respektive spelare har dragit. (antalKort1 & antalKort2)

Jag tror att DrMegahertz menar att du ska skicka värdet på variablerna istället för referenserna till dem.

På vissa ställen ser jag att du gör dessa jämförelser; kort1.namn == "Knekt", kort1.namn == "Dam" osv. Borde du inte kunna jämföra mot ett heltalsvärde istället? t.ex. kort1.varde == 11, kort1.varde == 12, såna jämförelser är mycket, mycket snabbare!

Mja, i och med att knektar, kungar och damer (och till och med 10:or) har samma värde, alltså 10 poäng, så funkar det inte skulle jag tro

DrMegahertz menar att du istället för string namn borde ha int namn (eller kanske något mer passande namn, typ int valor). Detsamma borde du göra med färg, istället för att ha en sträng med ett namn så kan du har ett färgid (t ex hjärter = 1, spader = 2, ruter = 3 och klöver = 4).

När dina projekt växer sig över några hundra rader kan det också vara rätt najs att dela upp projektet i flera källfiler, för att dra ner kompileringstiden efter att du ändrat på något. Det blir också lite enklare att överblicka vilka delar som ditt projekt består av.

Flera källfiler? Menar du att till exempel använda "ofstream" för att skriva data till en separat textfil som jag sedan anropar inifrån programmet? Borde inte det i så fall ta upp mer minnesanvändning av att behöva göra det hela tiden?

DrMegahertz menar källkodsfiler.

Exempel:
Du vill separera själva spelet från användargränssnittet så när du har lärt dig att göra ett grafiskt gränssnitt lätt kan byta ut det gamla.
Då kan du lägga allt som är specifikt för användargränssnittet (t ex alla cin och cout) i en fil och allt annat (alltså själva spelet) i en annan. Vips så har du två filer som:
1. Ger dig en bättre överblick
2. Gör det lättare att byta ut vissa delar
3. Enligt DrMegahertz ger kortare kompileringstid

[DrMegahertz]

Du skickar för många funktionsargument som referenser, enkla typer såsom char och int tjänar du inget på att skicka in som referenser(tvärt om), såvida du inte måste returera fler värden än ett.

Förstår inte riktigt vad du menar här De referenser jag skickar med till de olika funktionerna behövs (såvitt jag har tänkt i alla fall, antar du syftar på funktionen nyttKort() för att slumpa fram ett nytt kort) för att hålla räkning på:
a) vilken spelare det är som anropar ett kort för att kunna tilldela korten rätt. (spelareId)
b) hur många kort respektive spelare har dragit. (antalKort1 & antalKort2)

Mja, nyttKort() gör ju lite mer än vad den utger sig för att göra, nyttKort() borde väl egentligen bara dra ett kort från kortleken och returera det, men nu verkar det som om den är medveten om spelarnas kort, vem som ska få kort osv.. Hur som helst; kika på meny(), där tar du in en char som referensvariabel, vilket faktiskt betyder att mer data måste skickas, än om du hade skickat värdet i sig. En char består ju som bekant av en enda byte, men när du skickar in variabeln som en referens så skickar du in hela(!) 4 byte's till funktionen. Det är inga stora skillnader det rör sig om i det här fallet, men det jag försöker få fram är att det oftast är onödigt att skicka in enkla typer såsom int, char, float osv som referenser.

meny() skulle egentligen inte behöva några argument alls, du skulle kunna deklarera den som 'char meny();' och låta den returera det val som användaren har gjort. Som det ser ut nu så är det lite dåligt med funktioner som returerar något i koden

Globala variabler är generellt ett stort "no-no", jag syftar på variablerna 'kort1' och 'kort2'.

Varför? Har det med minnesanvändning att göra?

Mjo, först och främst så kommer de ju att ligga och ta upp minne under hela programmets livstid, även om de inte används i någon funktion. Utöver det så skitar man ju ner den globala namnrymden också..

cout << "\e[H\e[2J"; Detta har jag aldrig sett förut, men jag antar att den tömmer skärmen? '\e' följer inte standarden och bör undvikas.

Återigen, what? Är det själva principen att inte tömma skärmen på text, eller är det själva funktionen jag använt för att göra det som är fel? I så fall, vad kan jag skriva istället?

Mja, jag är ju faktiskt emot själva principen att tömma skärmen, men det största problemet här är att den där koden inte bara är plattformsberoende, den fungerar kanske inte ens mellan olika shelltyper(har icke testat, endast spekulationer). Andra lösningar som brukar dyka upp är ju att använda system("clear"); men även det är en dålig, plattformsberoende och slö lösning.

Det finns tyvärr inget enkelt, plattformsoberoende sätt att tömma skärmen på(såvida man inte blandar in tredjepartsbibliotek såsom ncurses) och därför bör det undvikas(what's the deal anyway?).

På vissa ställen ser jag att du gör dessa jämförelser; kort1.namn == "Knekt", kort1.namn == "Dam" osv. Borde du inte kunna jämföra mot ett heltalsvärde istället? t.ex. kort1.varde == 11, kort1.varde == 12, såna jämförelser är mycket, mycket snabbare!

Mja, i och med att knektar, kungar och damer (och till och med 10:or) har samma värde, alltså 10 poäng, så funkar det inte skulle jag tro

Jovisst, men du behöver ju ändå inte lagra hela namnet i varje kort, istället kan varje kort ha ett heltal mellan 2-14 som symboliserar kortets valör, namnet behövs ju bara när du skriver ut kortet, och då kan du skriva en funktion som tar fram en "mänsklig" representation av kortet. På så sätt blir det lite enklare att sortera korten också.

När dina projekt växer sig över några hundra rader kan det också vara rätt najs att dela upp projektet i flera källfiler, för att dra ner kompileringstiden efter att du ändrat på något. Det blir också lite enklare att överblicka vilka delar som ditt projekt består av.

Flera källfiler? Menar du att till exempel använda "ofstream" för att skriva data till en separat textfil som jag sedan anropar inifrån programmet? Borde inte det i så fall ta upp mer minnesanvändning av att behöva göra det hela tiden?

Icke, det handlar om att man delar upp sitt projekt's källkod i flera olika filer, för att på så sätt snabba upp kompileringen. Om du bara gör en liten ändring i någon funktion så behöver du inte kompilera om hela projektet, utan bara den lilla delen som du ändrade.

[Neonii]

Aha, okej, jag tror jag hänger med på vad ni menar!

Det finns tyvärr inget enkelt, plattformsoberoende sätt att tömma skärmen på(såvida man inte blandar in tredjepartsbibliotek såsom ncurses) och därför bör det undvikas(what's the deal anyway?).

Men det ser ju SNYGGARE ut om man bara visar upp den aktuella texten på skärmen, i alla fall i ett spel som Black Jack. Jag har redan fått klart för mig från användare här på forumet att det är ett stort nej-nej att göra skärmrensningar i exempelvis bashscripts, just av anledningen att man måste kunna se vad som händer etc. Men i ett spel så är det (enligt min uppfattning i alla fall ) snyggare ifall man bara visar den text som i det här exemplet har att göra med den aktuella omgången att göra..

Du vill separera själva spelet från användargränssnittet så när du har lärt dig att göra ett grafiskt gränssnitt lätt kan byta ut det gamla.
Då kan du lägga allt som är specifikt för användargränssnittet (t ex alla cin och cout) i en fil och allt annat (alltså själva spelet) i en annan. Vips så har du två filer som:
1. Ger dig en bättre överblick
2. Gör det lättare att byta ut vissa delar

Detta är lite oklart för mig hur det skall gå till, att skicka anrop mellan filer. Jag har endast lärt mig att läsa in filer i C++ genom att läsa in dom som separata textfiler med hjälp av Ofstream och sådant. Men jag antar att jag kommer veta lite mer då jag har läst på lite om hur man effektiviserar sin kod!

Tack för alla synpunkter än så länge! Jag kommer nog inte att ändra någonting i koden som jag skrivit (dels för att det inte är något seriöst projekt, och dels för att jag nu har insett hur rörig koden är ) utan istället hoppa på att lära mig mer om C++ och bli lite mer "avancerad". Har planer på att göra ett schack-spel inom en snar framtid, kanske till och med grafiskt ifall jag vet hur man gör

/Kristoffer