Permalänk
Medlem

[C++] Kodstruktur

Tack för hjälpen!

Permalänk
Medlem

Jag tycker det ser bra ut. Har du delat in det i olika filer eller är alla i en och samma fil ? Alltså koden, är allt i main filen ? Jag är inte den mest pro-programmeraren här men jag läste Programmering B för något år sedan och fick VG och min uppgift var inte såhär avancerat så jag tycker det är bra. Referenser och listor är bra. Sen så finns det en slags regel att man inte ska ha public variabler i struct-klasserna. Så i vissa perspektiv bör de vara privata. Sen kanske man kunde delat in kunder i en klass och konton i en annan. (refererar till en liknande uppgift jag gjort). Sen kanske man ska ha kommentarer är alltid bra för att förtydliga koden för läsaren.

Det är det jag kan komma på såhär på kvällstimmen, hoppas det varit till någon hjälp då jag inte är så erfaren ännu

Visa signatur

ATX: Intel Core i5 4690k | Asus Z97-A | MSI R9 390 8GB | 2x Corsair DDR3 2133Mhz | Crucial BX480Gb | Corsair TX650W | Fractal Design Define R4
mITX: Intel Core i3 6100 | Asus B150I | 2x8Gb Corsair DDR4 2133Mhz | Samsung 850 Evo 500GB | Cooler Master V750 | Fractal Design Define Nano S

Permalänk
Hedersmedlem

Du skulle kanske kunna överväga att använda en map<int,konton> för att slippa alla de där iterationerna över konton-listor. Och en sätt att undvika felaktig inläsning är väl att kontrollera alla värden innan de används; är pos != npos, till exempel.

Permalänk
Medlem

Så som jag förstår det så handlar de om att göra de private för att man inte skall kunna strula till något. Tror jag kan ta något exempel. Du har tex gjort en kalender. Då är det väldigt många funktioner osv som går i bakgrunden för att allt skall fungera. Om man då tex låter någon annan underhålla koden. Så tex du har kodat då vilka månader som har 31 och 30 dagar och februari som kan ha 28 och 29. Om du då låter tex en variable date vara publik då tillåter du någon att kunna göra om den variabeln till tex 40. Med en instans av variabeln så kanske calender.date = 40. Vilket ger ett fel när programmet körs då det bara finns max 31 dagar. Så det handlar om att ha en bra design som reducerar chansen att uppstå problem. Då är det bättre att ha alla klass-variabler privata och använda sig av set/get metoder för att ange namn eller värden på variabler. Det var den bästa förklaringen jag kunde komma på hoppas du förstå lite i alla fall. Men googla på det annars eller se om någon annan på forumet vet ett bättre svar eller ett mer avancerat

Visa signatur

ATX: Intel Core i5 4690k | Asus Z97-A | MSI R9 390 8GB | 2x Corsair DDR3 2133Mhz | Crucial BX480Gb | Corsair TX650W | Fractal Design Define R4
mITX: Intel Core i3 6100 | Asus B150I | 2x8Gb Corsair DDR4 2133Mhz | Samsung 850 Evo 500GB | Cooler Master V750 | Fractal Design Define Nano S

Permalänk
Medlem
Skrivet av linuxe93:

Tror jag fattar, tack för förklaringen!

Något mer jag undrar är att min lärare skrev ju som sagt:
"en för MVG-nivå skall det också vara snyggt designat enligt de kriterier som vi gått igenom under A- & B-kursen.
(Konstantdeklarerade variabler istället för konstanta värden, iaf där värdet används mer än en gång)
- Funktioner som är generella men samtidigt inte gör för mycket.
- Återanvändning av kod, t.ex. med hjälp av funktioner som kan användas på flera olika ställen i koden
- Bra & tydliga variabel- & funktionsnamn
- Ej onödigt komplicerade lösningar".

Är det någonstans i min kod där jag inte uppfyller något av dessa, nu kan det vara svårt att se vad vissa delar i koden gör när jag inte skrivit kommentarer än, men finns det t.ex. ställen där jag kan slå ihop 2 funktioner eller dylikt för att göra koden snyggare.
Dessutom undrar jag om variabel- & funktionsnamnen är tydliga.

Nu är jag ingen lärare så jag kan tyvärr inte uttala mig om betygskriterierna, men jag tycker att koden är fullt läslig. Personligen skulle jag dock överväga att baka in allt i en klass och ha alla dina funktioner som klassmetoder och låta de inlästa värdena sparas i en klassvariabel.

Permalänk
Medlem
Skrivet av linuxe93:

Vi har ej gått igenom klasser än (antar att det ingår i prog. C), så är inte helt haj på det.
Men tack för tipset!

Okej. Då förstår jag att det är lite knepigt.

Att baka ihop funktioner just nu tycker jag inte att du skall göra då jag gillar att du har en tydlig uppdelning där en sak tydligt gör det som står i funktionsnamnet. Förövrigt tycker jag att variabelnamnen är tydliga.

Nu vet jag inte riktigt hur vad ni har för direktiv angående detta, men praxis brukar vara att man har engelska namn på funktioner och variabler. Detta skall dock inte alls ses som ett måste.

Permalänk
Medlem

Nu har jag inte läst alla kommentarer men tycker det i allmänhet ser bra ut. Det som egentligen saknas är kommentarer till vad metoderna gör(även om det är rätt obvious). Ofta gör man ungefär som jag skriver nedan ovanför metoderna. Minns inte om det är @ som är standard i c++ men något i den stilen.

/**
metoden gör bla bla bla
@parameter1 : yadayda
@parameter2 : lalalala
*/

Visa signatur

Chasi: Thermaltake XaserVI LBNS | Mobo: P9X79 PRO
Cpu: SB-E 3930k 4.8ghz @ 1.34v | Kylning: 2x Laing DCC, EK supreme HF Cu, 45*60cm nissan bilkylare.
Ram: 6x4GB G.skill 2133mhz cl11 | Disk: Samsung 830 256GB

Permalänk
Medlem

Tycker koden ser fin ut. Bra jobbat.

En del saker jag tänkte lägga till:

* Om jag hade gjort programmet hade jag lagt in cin i din meny funktion och satt en retur ifrån den. Men det spelar ingen roll egentligen.

* För en lite finare kod hade jag lagt koden i denna struktur och tagit bort variablerna för funktion dekerationerna, ser lite finare ut.
så här:

#include <iostream> #include <string> #include <fstream> #include <iomanip> #include <list> #include <time.h> #include <sstream> using namespace std; struct konton { string namn; int nummer; double saldo; }; void bankMeny (); bool sorteraNummer(const konton&, const konton&); bool sorteraNamn(const konton&, const konton&); void laddaKonton (list <konton> &); void sparaKonton (list <konton>); int hanteraKonto (list <konton>); void visaSaldo (list <konton>); void visaKonton (list <konton>, int); void hanteraPengar (list <konton> &, int); konton laggTillKonto (list <konton>); void taBortKonto (list <konton> &); int main () { int menyVal, bankVal, sortVal; list <konton> bankKonto; laddaKonton(bankKonto); while (1) { system("CLS"); bankMeny(); cin >> menyVal; switch (menyVal) { case 1: visaSaldo(bankKonto); break; case 2: bankVal = 1; hanteraPengar(bankKonto, bankVal); break; case 3: bankVal = 2; hanteraPengar(bankKonto, bankVal); break; case 4: system("CLS"); cout << "1) Visa i kontonummerordning" << endl; cout << "2) Visa i namnordning" << endl << endl; cout << "Ange ditt menyval: "; cin >> sortVal; if (sortVal >= 1 && sortVal <= 2) { visaKonton(bankKonto, sortVal); } else { system("CLS"); cout << "Anv\x84nd 1-2 som menyval!" << endl; system("PAUSE"); } break; case 5: bankKonto.push_back(laggTillKonto(bankKonto)); break; case 6: taBortKonto(bankKonto); break; case 7: sparaKonton(bankKonto); return 0; break; default: system("CLS"); cout << "Anv\x84nd 1-7 som menyval!" << endl; system("PAUSE"); } } return 0; } void bankMeny () { cout << "1) Visa saldo" << endl; cout << "2) Ta ut pengar" << endl; cout << "3) S\x84tt in pengar" << endl; cout << "4) Visa alla konton" << endl; cout << "5) Skapa nytt konto" << endl; cout << "6) Ta bort ett befintligt konto" << endl; cout << "7) Avsluta" << endl << endl; cout << "Ange ditt menyval: "; } bool sorteraNummer(const konton& forst, const konton& sist) { return forst.nummer < sist.nummer; } bool sorteraNamn(const konton& forst, const konton& sist) { return forst.namn < sist.namn; } void laddaKonton (list <konton> &bankKonto) { string sparaText; konton addKonto; ifstream laddaFil; laddaFil.open("konton.txt"); if (laddaFil) { while (getline(laddaFil, sparaText)) { size_t pos = sparaText.find('#'); addKonto.namn = sparaText.substr(0, pos); string temp = sparaText.substr(pos+1); pos = temp.find(','); stringstream bufferNummer(temp.substr(0, pos)); bufferNummer >> addKonto.nummer; stringstream bufferSaldo(temp.substr(pos+1)); bufferSaldo >> addKonto.saldo; cout << addKonto.saldo; bankKonto.push_back(addKonto); } } else ofstream sparaFil("konton.txt"); laddaFil.close(); } void sparaKonton (list <konton> bankKonto) { ofstream sparaFil; list <konton>::iterator it; sparaFil.open("konton.txt"); if (sparaFil) { for (it = bankKonto.begin(); it != bankKonto.end(); it++) sparaFil << it->namn << "#" << it->nummer << "," << it->saldo << endl; } else { system("cls"); cout << "Din fil kunde ej sparas!" << endl; system("PAUSE"); } sparaFil.close(); } int hanteraKonto (list <konton> bankKonto) { int kontoNummer = 0; system("CLS"); if (!bankKonto.empty()) // Kollar så att listan med konto har något innehåll { bool hittadeKonto = false; // Bool-variabel som kollar ifall kontot har hittats list <konton>::iterator it; cout << "V\x84lj vilket kontonummer du vill hantera (avsluta med 0): "; cin >> kontoNummer; while(!hittadeKonto && kontoNummer != 0) // While-snurra som körs så länge som "kontoNummer" ej är 0 och "hittadeKonto" är "false" { for (it = bankKonto.begin(); it!=bankKonto.end(); it++) { if (it->nummer == kontoNummer) // Om iteratorn "it" hittar angett konto så blir "hittadeKonto" "true" hittadeKonto = true; } if (!hittadeKonto && kontoNummer != 0) { system("CLS"); cout << "Hittade inget konto med ditt kontonummer!" << endl; system("PAUSE"); system("CLS"); cout << "V\x84lj vilket kontonummer du vill hantera (avsluta med 0): "; cin >> kontoNummer; } } } else { cout << "Det fanns inga konton i programmet!" << endl; system("PAUSE"); } return kontoNummer; } void visaSaldo (list <konton> bankKonto) { list <konton>::iterator it; int kontoNummer = hanteraKonto(bankKonto); if (kontoNummer != 0) // Kollar ifall det returnerade värdet från hanteraKonton inte är 0, i så fall har programmet lyckats hitta ett konto { for (it = bankKonto.begin(); it!=bankKonto.end(); it++) { if (it->nummer == kontoNummer) { system("CLS"); cout.setf(ios::fixed); cout << "Saldo f\x94r konto " << it->nummer << ": " << setprecision(2) << it->saldo << ":-" << endl; } } system("PAUSE"); } } void visaKonton (list <konton> bankKonto, int sortVal) { system("CLS"); if (!bankKonto.empty()) { list <konton>::iterator it; if (sortVal == 1) bankKonto.sort(sorteraNummer); else bankKonto.sort(sorteraNamn); for (it = bankKonto.begin(); it != bankKonto.end(); it++) { cout << it->nummer << ") " << it->namn << endl; } system("PAUSE"); } else { cout << "Det fanns inga konton i programmet!" << endl; system("PAUSE"); } } void hanteraPengar (list <konton> &bankKonto, int bankVal) { list <konton>::iterator it; int kontoNummer = hanteraKonto(bankKonto); double summa; if (kontoNummer != 0) { for (it = bankKonto.begin(); it!=bankKonto.end(); it++) { if (it->nummer == kontoNummer) { system("CLS"); if (bankVal == 1) { cout << "Ange summa att ta ut: "; cin >> summa; while (summa < 0 || summa > it->saldo) { cout << "Ett fel uppstod!" << endl; system("PAUSE"); system("CLS"); cout << "Ange summa att ta ut: "; cin >> summa; } it->saldo -= summa; } else { cout << "Ange summa att s\x84tta in: "; cin >> summa; while (summa < 0) { cout << "Negativa v\x84rden fungerar ej!" << endl; system("PAUSE"); system("CLS"); cout << "Ange summa att s\x84tta in: "; cin >> summa; } it->saldo += summa; } } } } } konton laggTillKonto (list <konton> bankKonto) { srand(unsigned(time(NULL))); bool geNummer = true; int kontoNummer = 1000 + rand()%9000; string kontoNamn; konton laggaTill; list <konton>::iterator it; cin.ignore(); while (geNummer) { if (!bankKonto.empty()) { for (it = bankKonto.begin(); it != bankKonto.end(); it++) { if (it->nummer != kontoNummer) geNummer = false; else { geNummer = true; kontoNummer = 1000 + rand()%9000; it--; } } } else geNummer = false; } laggaTill.nummer = kontoNummer; laggaTill.saldo = 0; system("CLS"); cout << "Ange kontots namn: "; getline(cin, kontoNamn); laggaTill.namn = kontoNamn; system("CLS"); cout << "Grattis! Du har nu skapat ett nytt konto!" << endl; cout << "Ditt kontonummer: " << laggaTill.nummer << endl; system("PAUSE"); return laggaTill; } void taBortKonto (list <konton> &bankKonto) { system("CLS"); if (!bankKonto.empty()) { bool taBortSuccess = false; int kontoNummer; list <konton>::iterator it; bankKonto.sort(sorteraNummer); for (it = bankKonto.begin(); it != bankKonto.end(); it++) cout << it->nummer << ") " << it->namn << endl; cout << endl << "Ange kontonummer (avsluta med 0): "; cin >> kontoNummer; while(!taBortSuccess && kontoNummer != 0) { for (it = bankKonto.begin(); it!=bankKonto.end(); it++) { if (it->nummer == kontoNummer) { taBortSuccess = true; bankKonto.erase(it); break; } } if (!taBortSuccess && kontoNummer != 0) { system("CLS"); cout << "Hittade inget konto med ditt kontonummer!" << endl; system("PAUSE"); system("CLS"); for (it = bankKonto.begin(); it != bankKonto.end(); it++) cout << it->nummer << ") " << it->namn << endl; cout << endl << "Ange kontonummer (avsluta med 0): "; cin >> kontoNummer; } } } else { cout << "Det fanns inga konton i programmet!" << endl; system("PAUSE"); } }

* för att skippa att hålla på med \x84 för att få åäö så skulle jag ha skrivit setlocale(0,"swedish"); i början av main.

Mer har jag inte att säga just nu. kanske kommer på mer senare när jag har kollat mer på koden.

//Edit
En fråga, du kanske inte har lärt dig klasser än? Ser att du använder struct istället. Lägg till en construtor i den, så kanske läraren blir glad :-), vem vet?

Visa signatur

OS: Win7 x64, GPU: Geforce Gigabyte GTX580 SOC
CPU: Intel i5 2500k (4.5gHz), MB: Asus P8P65 PRO Rev(3.1),
PSU: XFX 750w (modular), RAM: 2x Crosair Vengence 1600mz 4024mb
Cooling: CoolIT ECO A.L.C CPU COOLER
Chassi: Raven rv03

Permalänk
Medlem
Skrivet av linuxe93:

Tack så mycket!
Du har absolut helt rätt med det variablerna och menyfunktionen, ska fixa det omedelbart

Skrivet av ediz:

Tycker koden ser fin ut. Bra jobbat.

En del saker jag tänkte lägga till:

* Om jag hade gjort programmet hade jag lagt in cin i din meny funktion och satt en retur ifrån den. Men det spelar ingen roll egentligen.

* För en lite finare kod hade jag lagt koden i denna struktur och tagit bort variablerna för funktion dekerationerna, ser lite finare ut.
så här:

*Massa kod*

* för att skippa att hålla på med \x84 för att få åäö så skulle jag ha skrivit setlocale(0,"swedish"); i början av main.

Mer har jag inte att säga just nu. kanske kommer på mer senare när jag har kollat mer på koden.

//Edit
En fråga, du kanske inte har lärt dig klasser än? Ser att du använder struct istället. Lägg till en construtor i den, så kanske läraren blir glad :-), vem vet?

Angånde variabelnamnen i funktionsdeklarationerna kan jag bara säga att jag har den motsatta åsikten. Tycker att det blir mycket lättare att se vad koden gör om man bara kan skumma igenom deklarationslistan och få de informativa variabelnamnen.

Nu skall jag isäng, men bra jobbat och lycka till!

Permalänk
Inaktiv

Dela upp i cc och h filer så blir det lite snyggare, lägg till private i structen och skapa get/set funktioner, skapa privata och toma kopieringsconstructorer så att inte kompilatorn genererar egna.

Du skulle kunna skriva typ if(temp.find(',')==-1){ cout << "fel argument\n"; return 0; } för att se så att ',' finns med.
Då du skriver "tecken" så antar jag att du menar chars istället för integers, det kan du lösa med en for-loop och en if(isdigit(........))
Good luck

Permalänk
Medlem
Skrivet av andkem:

Angånde variabelnamnen i funktionsdeklarationerna kan jag bara säga att jag har den motsatta åsikten. Tycker att det blir mycket lättare att se vad koden gör om man bara kan skumma igenom deklarationslistan och få de informativa variabelnamnen.

Nu skall jag isäng, men bra jobbat och lycka till!

Jag förstår vad du menar, jag kan hålla med om det ibland. Men mina strikta programmeringslärare tvinga mig att skriva så. Kommer inte riktigt ihåg varför, men om vi hade minnesläckor, skrev med variablar i deklerationerna, hade globala variablar, igen inkapsling i klasserna och så vidare fick vi underkänt direkt :-).

linuxe93:

Skulle bara tillägga en sak som du inte får glömma framtiden när du gör switch/case. Om du declarerar en variabel i en case, så måste du ha bracers.

ex:

switch (var) { case 1: commands(); break; case 2: { int i =0; double p = 0.0; } break; default: break; }

Visa signatur

OS: Win7 x64, GPU: Geforce Gigabyte GTX580 SOC
CPU: Intel i5 2500k (4.5gHz), MB: Asus P8P65 PRO Rev(3.1),
PSU: XFX 750w (modular), RAM: 2x Crosair Vengence 1600mz 4024mb
Cooling: CoolIT ECO A.L.C CPU COOLER
Chassi: Raven rv03

Permalänk
Medlem

Jag använder alltid bracers ialla fall.

en annan sak som jag såg i structen.

* I regel, Skriv alltid klasser och structs med stor bosktav i början, ex: Konton

* Som felera skriver, det är bättre kod om du anväder inkapsling i structen med private: oh public:
Lägg gärna till konstruktor, set, get funktioner.

god natt

Visa signatur

OS: Win7 x64, GPU: Geforce Gigabyte GTX580 SOC
CPU: Intel i5 2500k (4.5gHz), MB: Asus P8P65 PRO Rev(3.1),
PSU: XFX 750w (modular), RAM: 2x Crosair Vengence 1600mz 4024mb
Cooling: CoolIT ECO A.L.C CPU COOLER
Chassi: Raven rv03

Permalänk
Hedersmedlem

Ibland kan man undvika onödiga upprepningar genom att använda do istället för while:

for (it = bankKonto.begin(); it != bankKonto.end(); it++) cout << it->nummer << ") " << it->namn << endl; cout << endl << "Ange kontonummer (avsluta med 0): "; cin >> kontoNummer; while(!taBortSuccess && kontoNummer != 0) { for (it = bankKonto.begin(); it!=bankKonto.end(); it++) { if (it->nummer == kontoNummer) { taBortSuccess = true; bankKonto.erase(it); break; } } if (!taBortSuccess && kontoNummer != 0) { system("CLS"); cout << "Hittade inget konto med ditt kontonummer!" << endl; system("PAUSE"); system("CLS"); for (it = bankKonto.begin(); it != bankKonto.end(); it++) cout << it->nummer << ") " << it->namn << endl; cout << endl << "Ange kontonummer (avsluta med 0): "; cin >> kontoNummer; } }

borde till exempel kunna skrivas som:

do { for (it = bankKonto.begin(); it != bankKonto.end(); it++) cout << it->nummer << ") " << it->namn << endl; cout << endl << "Ange kontonummer (avsluta med 0): "; cin >> kontoNummer; if(kontoNummer != 0) { taBortSuccess = false; for (it = bankKonto.begin(); it!=bankKonto.end(); it++) if (it->nummer == kontoNummer) { taBortSuccess = true; bankKonto.erase(it); break; } if(!taBortSuccess) { system("CLS"); cout << "Hittade inget konto med ditt kontonummer!" << endl; system("PAUSE"); system("CLS"); } } } while(!taBortSuccess && kontoNummer != 0);

Permalänk
Medlem

Kan meddela att jag lyckades få MVG på uppgiften.
Tack som f*n för er kritik!