Verbesserungs Vorschläge



  • Hallo, ich habe vor 3 Tagen angefangen C++ zu lernen (Meine erste Programmier Sprache hab also keine andere Erfahrung)

    Da mir gerade Langweilig war habe ich micht entschieden einen Konsolen Rechner in C++ zu schreiben, und ich wollte hier meinen Code zeigen für Verbesserungsvorschläge (Was ich anders hätte machen sollen oder was ich einfach nur anders/besser machen könnte)

    *Der Code ist ganz alleine von mir Geschrieben da ich Copy-Paste nicht als Lernen bezeichne und deshalb sogut wie alles mögliche selbst machen will, das einzige wonach ich Googlen musste war "SetConsoleTitle" da ich einen Error hatte.

    #include <iostream>
    #include <windows.h>
    
    int iZahl1;
    int iZahl2;
    
    int iRechenArt;
    
    auto iStart = true;
    
    int main()
    {
    	while (iStart) {
    		SetConsoleTitle(TEXT("Rechner"));
    		std::cout << "Wollen sie den Rechner Starten? (0=false/1=true)\n";
    		std::cin >> iStart;
    
    		if (!iStart) {
    			return 0;
    		}
    		std::cout << "Addition = 1, Subtraktion = 2, Multiplikation = 3, Division = 4\n";
    		std::cin >> iRechenArt;
    
    		switch (iRechenArt) {
    		case 1:
    			SetConsoleTitle(TEXT("Addition"));
    			system("cls");
    			std::cout << "Geben sie die Erste Zahl ein" << std::endl;
    			std::cin >> iZahl1;
    			std::cout << "Geben sie die Zweite Zahl ein" << std::endl;
    			std::cin >> iZahl2;
    			std::cout << "Ergebnis: " << "" << iZahl1 << " + " << "" << iZahl2 << " = " << iZahl1 + iZahl2 << std::endl;
    			break;
    		case 2:
    			SetConsoleTitle(TEXT("Subtraktion"));
    			system("cls");
    			std::cout << "Geben sie die Erste Zahl ein" << std::endl;
    			std::cin >> iZahl1;
    			std::cout << "Geben sie die Zweite Zahl ein" << std::endl;
    			std::cin >> iZahl2;
    			std::cout << "Ergebnis: " << "" << iZahl1 << " - " << "" << iZahl2 << " = " << iZahl1 - iZahl2 << std::endl;
    			break;
    		case 3:
    			SetConsoleTitle(TEXT("Multiplikation"));
    			system("cls");
    			std::cout << "Geben sie die Erste Zahl ein" << std::endl;
    			std::cin >> iZahl1;
    			std::cout << "Geben sie die Zweite Zahl ein" << std::endl;
    			std::cin >> iZahl2;
    			std::cout << "Ergebnis: " << "" << iZahl1 << " * " << "" << iZahl2 << " = " << iZahl1 * iZahl2 << std::endl;
    			break;
    		case 4:
    			SetConsoleTitle(TEXT("Divison"));
    			system("cls");
    			std::cout << "Geben sie die Erste Zahl ein" << std::endl;
    			std::cin >> iZahl1;
    			std::cout << "Geben sie die Zweite Zahl ein" << std::endl;
    			std::cin >> iZahl2;
    			std::cout << "Ergebnis: " << "" << iZahl1 << " / " << "" << iZahl2 << " = " << iZahl1 / iZahl2 << std::endl;
    			break;
    
    		default:
    			return 0;
    		}
    		system("PAUSE");
    	}
    }
    


  • Edit: Mir ist aufgefallen dass ich sehr viel Code unnötig wiederholt habe, deshalb habe ich den Code ein wenig Optimiert. 😃

    #include <iostream>
    #include <windows.h>
    
    int iZahl1;
    int iZahl2;
    
    int iRechenArt;
    
    auto bStart = true;
    
    int main()
    {
    	while (bStart) {
    		SetConsoleTitle(TEXT("Rechner"));
    		std::cout << "Wollen sie den Rechner Starten? (0=false/1=true)\n";
    		std::cin >> bStart;
    
    		if (!bStart) {
    			return 0;
    		}
    		std::cout << "Addition = 1, Subtraktion = 2, Multiplikation = 3, Division = 4\n";
    		std::cin >> iRechenArt;
    		system("cls");
    		std::cout << "Geben sie die Erste Zahl ein" << std::endl;
    		std::cin >> iZahl1;
    		std::cout << "Geben sie die Zweite Zahl ein" << std::endl;
    		std::cin >> iZahl2;
    
    		switch (iRechenArt) {
    		case 1:
    			SetConsoleTitle(TEXT("Addition"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << iZahl1 << " + " << "" << iZahl2 << " = " << iZahl1 + iZahl2 << std::endl;
    			break;
    		case 2:
    			SetConsoleTitle(TEXT("Subtraktion"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << iZahl1 << " - " << "" << iZahl2 << " = " << iZahl1 - iZahl2 << std::endl;
    			break;
    		case 3:
    			SetConsoleTitle(TEXT("Multiplikation"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << iZahl1 << " * " << "" << iZahl2 << " = " << iZahl1 * iZahl2 << std::endl;
    			break;
    		case 4:
    			SetConsoleTitle(TEXT("Divison"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << iZahl1 << " / " << "" << iZahl2 << " = " << iZahl1 / iZahl2 << std::endl;
    			break;
    
    		default:
    			return 0;
    		}
    		system("PAUSE");
    	}
    }
    

  • Mod

    Keine globalen Variablen benutzen! Deklarier Variablen erst genau da, wo du sie brauchst. Zu weit gefasste Gültigkeitsbereiche für Variablen sind eine der Hauptfehlerursachen überhaupt und führt zu besonders schwer zu findenden Fehlern.

    Keine ungarische Notation benutzen (also dein Typkürzel vor Bezeichnern, z.B. das 'i' bei iZahl). Erstens kann man es in Sprachen mit komplexen Datentypen ohnehin nicht konsequent durchziehen und zweitens sollte der konkrete Typ einer Variable nie eine Rolle spielen. Daher ist ungarische Notation in C++ nur unnötiges Rauschen, das das Programm schwerer zu lesen macht.



  • Findest du nicht, dass ungarische Notation und auto zwei Dinge sind, die nicht zusammenpassen? (IMHO ist ungarische Notation (insbesondere in der Windowfehlinterpretation mit Datentypen) blödsinn)

    Globale Variablen? Definiere Variablen genau da, wo sie benutzt werden, nicht eher.

    Ein Ganzzahltaschenrechner?

    Könnte man die Rechenart nicht gleich als +-*/ eingaben können?



  • SeppJ schrieb:

    Keine globalen Variablen benutzen! Deklarier Variablen erst genau da, wo du sie brauchst. Zu weit gefasste Gültigkeitsbereiche für Variablen sind eine der Hauptfehlerursachen überhaupt und führt zu besonders schwer zu findenden Fehlern.

    Keine ungarische Notation benutzen (also dein Typkürzel vor Bezeichnern, z.B. das 'i' bei iZahl). Erstens kann man es in Sprachen mit komplexen Datentypen ohnehin nicht konsequent durchziehen und zweitens sollte der konkrete Typ einer Variable nie eine Rolle spielen. Daher ist ungarische Notation in C++ nur unnötiges Rauschen, das das Programm schwerer zu lesen macht.

    manni66 schrieb:

    Findest du nicht, dass ungarische Notation und auto zwei Dinge sind, die nicht zusammenpassen? (IMHO ist ungarische Notation (insbesondere in der Windowfehlinterpretation mit Datentypen) blödsinn)

    Globale Variablen? Definiere Variablen genau da, wo sie benutzt werden, nicht eher.

    Ein Ganzzahltaschenrechner?

    Könnte man die Rechenart nicht gleich als +-*/ eingaben können?

    Danke euch beiden, mit der ungarischen Notation habe ich jetzt direkt wieder aufgehört da ich zumglück sowieso erst in diesem Projekt damit angefangen habe.

    #include <iostream>
    #include <windows.h>
    
    int main()
    {
    	auto Start = true;
    	while (Start) {
    		SetConsoleTitle(TEXT("Rechner"));
    		std::cout << "Wollen sie den Rechner Starten? (0=false/1=true)\n";
    		std::cin >> Start;
    
    		if (!Start) {
    			return 0;
    		}
    
    		int RechenArt;
    		std::cout << "Addition = 1, Subtraktion = 2, Multiplikation = 3, Division = 4\n";
    		std::cin >> RechenArt;
    		system("cls");
    
    		int Zahl1;
    		int Zahl2;
    		std::cout << "Geben sie die Erste Zahl ein" << std::endl;
    		std::cin >> Zahl1;
    		std::cout << "Geben sie die Zweite Zahl ein" << std::endl;
    		std::cin >> Zahl2;
    
    		switch (RechenArt) {
    		case 1:
    			SetConsoleTitle(TEXT("Addition"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " + " << "" << Zahl2 << " = " << Zahl1 + Zahl2 << std::endl;
    			break;
    		case 2:
    			SetConsoleTitle(TEXT("Subtraktion"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " - " << "" << Zahl2 << " = " << Zahl1 - Zahl2 << std::endl;
    			break;
    		case 3:
    			SetConsoleTitle(TEXT("Multiplikation"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " * " << "" << Zahl2 << " = " << Zahl1 * Zahl2 << std::endl;
    			break;
    		case 4:
    			SetConsoleTitle(TEXT("Divison"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " / " << "" << Zahl2 << " = " << Zahl1 / Zahl2 << std::endl;
    			break;
    
    		default:
    			return 0;
    		}
    		system("PAUSE");
    	}
    }
    

    Sieht es so schon wieder besser aus?



  • Aufbauend auf dem Vorschlag mit der Eingabe des Rechenzeichens ist das vllt. was für dich (Quick & Dirty):

    IDEONE



  • #include <iostream>
    #include <windows.h>
    
    int main()
    {
    	auto Start = true;
    	while (Start) {
    		SetConsoleTitle(TEXT("Rechner"));
    		std::cout << "Wollen sie den Rechner Starten? (0=false/1=true)\n";
    		std::cin >> Start;
    
    		if (!Start) {
    			exit(0);
    		}
    
    		int Zahl1;
    		int Zahl2;
    		char RechenArt;
    		std::cout << "Geben sie ihre Rechnung an" << std::endl;
    		std::cin >> Zahl1;
    		std::cin >> RechenArt;
    		std::cin >> Zahl2;
    
    		switch (RechenArt) {
    		case '+':
    			SetConsoleTitle(TEXT("Addition"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " + " << "" << Zahl2 << " = " << Zahl1 + Zahl2 << std::endl;
    			break;
    		case '-':
    			SetConsoleTitle(TEXT("Subtraktion"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " - " << "" << Zahl2 << " = " << Zahl1 - Zahl2 << std::endl;
    			break;
    		case '*':
    			SetConsoleTitle(TEXT("Multiplikation"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " * " << "" << Zahl2 << " = " << Zahl1 * Zahl2 << std::endl;
    			break;
    		case '/':
    			SetConsoleTitle(TEXT("Divison"));
    			system("cls");
    			std::cout << "Ergebnis: " << "" << Zahl1 << " / " << "" << Zahl2 << " = " << Zahl1 / Zahl2 << std::endl;
    			break;
    
    		default:
    			return 0;
    		}
    		system("PAUSE");
    	}
    }
    

    Danke für eure Hilfe, Ich habe aufjedenfall viel dazugelernt ^^.


  • Mod

    Deine Start-Schleife ist irgendwie ein bisschen komisch gebaut. Sagen wir mal so: Wird die Bedingung in Zeile 7 jemals false sein? Nein, die Schleife würde stattdessen in Zeile 12 verlassen. Das kann man auch geschickter formulieren.

    exit solltest du dir übrigens sofort wieder abgewöhnen. exit räumt keine automatischen Variablen auf. Dieses Aufräumen ist aber der Kern jeder Ressourcenverwaltung in C++. Von der main aus kannst du einfach return machen, das räumt korrekt auf.


  • Mod

    Der switch-case enthält immer noch recht viel Wiederholungen, das lässt sich noch weiter verdichten, z.B.

    LPCTSTR title;
            int Ergebnis;
            switch (RechenArt) {
            default: return 0;
            case '+': Ergebnis = Zahl1 + Zahl2; title = TEXT("Addition");       break;
            case '-': Ergebnis = Zahl1 - Zahl2; title = TEXT("Subtraktion");    break;
            case '*': Ergebnis = Zahl1 * Zahl2; title = TEXT("Multiplikation"); break;
            case '/': Ergebnis = Zahl1 / Zahl2; title = TEXT("Division");       break;
            }
            SetConsoleTitle(title);
            system("cls");
            std::cout << "Ergebnis: " << "" << Zahl1 << " / " << "" << Zahl2 << " = " << Ergebnis << std::endl;
    


  • SeppJ schrieb:

    Deine Start-Schleife ist irgendwie ein bisschen komisch gebaut. Sagen wir mal so: Wird die Bedingung in Zeile 7 jemals false sein? Nein, die Schleife würde stattdessen in Zeile 12 verlassen. Das kann man auch geschickter formulieren.

    exit solltest du dir übrigens sofort wieder abgewöhnen. exit räumt keine automatischen Variablen auf. Dieses Aufräumen ist aber der Kern jeder Ressourcenverwaltung in C++. Von der main aus kannst du einfach return machen, das räumt korrekt auf.

    Gut exit gewöhne ich mir direkt wieder ab ^^.
    Die Schleife ist nur da weil ich keinen anderen weg wüsste nach der Rechnung wieder von vorne anzufangen.

    camper schrieb:

    Der switch-case enthält immer noch recht viel Wiederholungen, das lässt sich noch weiter verdichten, z.B.

    LPCTSTR title;
            int Ergebnis;
            switch (RechenArt) {
            default: return 0;
            case '+': Ergebnis = Zahl1 + Zahl2; title = TEXT("Addition");       break;
            case '-': Ergebnis = Zahl1 - Zahl2; title = TEXT("Subtraktion");    break;
            case '*': Ergebnis = Zahl1 * Zahl2; title = TEXT("Multiplikation"); break;
            case '/': Ergebnis = Zahl1 / Zahl2; title = TEXT("Division");       break;
            }
            SetConsoleTitle(title);
            system("cls");
            std::cout << "Ergebnis: " << "" << Zahl1 << " / " << "" << Zahl2 << " = " << Ergebnis << std::endl;
    

    Das hatte ich vorhin auch schon vor nur hatte ich keine Ahnung wie ich es machen sollte. Vielen Dank, auf das mit dem title wäre ich anders auch nicht darauf gekommen .


  • Mod

    wasauchimmer schrieb:

    Die Schleife ist nur da weil ich keinen anderen weg wüsste nach der Rechnung wieder von vorne anzufangen.

    Schleifen sind schon in Ordnung. Deine ist bloß etwas ungünstig formuliert, da die erste Bedingung nichts tut. Da könnte genau so gut while(true) stehen und dein Programm würde exakt gleich verlaufen.



  • SeppJ schrieb:

    wasauchimmer schrieb:

    Die Schleife ist nur da weil ich keinen anderen weg wüsste nach der Rechnung wieder von vorne anzufangen.

    Schleifen sind schon in Ordnung. Deine ist bloß etwas ungünstig formuliert, da die erste Bedingung nichts tut. Da könnte genau so gut while(true) stehen und dein Programm würde exakt gleich verlaufen.

    Stimmt, werde ich mir merken vielen dank.

    #include <iostream>
    #include <windows.h>
    
    int main()
    {
    	while (true) {
    		SetConsoleTitle(TEXT("Rechner"));
    
    		int Zahl1;
    		int Zahl2;
    		char RechenArt;
    
    		std::cout << "Geben sie ihre Rechnung an" << std::endl;
    		std::cin >> Zahl1;
    		std::cin >> RechenArt;
    		std::cin >> Zahl2;
    
    		LPCTSTR title;
    		int Ergebnis;
    		switch (RechenArt) {
    		case '+':
    			title = TEXT("Addition");
    			Ergebnis = Zahl1 + Zahl2;
    			break;
    		case '-':
    			title = TEXT("Subtraktion");
    			Ergebnis = Zahl1 - Zahl2;
    			break;
    		case '*':
    			title = TEXT("Multiplikation");
    			Ergebnis = Zahl1 * Zahl2;
    			break;
    		case '/':
    			title = TEXT("Division");
    			Ergebnis = Zahl1 / Zahl2;
    			break;
    		default:
    			return 0;
    		}
    		SetConsoleTitle(title);
    		system("cls");
    		std::cout << "Ergebnis: " << Ergebnis << std::endl;
    	}
    }
    


  • Egal welchen Einrückstil: Bleibe einheitlich. Mir persönlich hat deine ursprüngliche Einrückung besser gefallen als das was du zuletzt geschrieben hast (sofern da jetzt keine Tabstops reingeraten sind die, die Formatierung im Forum anders darstellen).

    z.B.:

    int main() {
        while (true) {
            SetConsoleTitle(TEXT("Rechner"));
            ...
        }
    }
    

    oder:

    int main()
    {
        while (true)
        {
            SetConsoleTitle(TEXT("Rechner"));
            ...
        }
    }
    


  • asc schrieb:

    Egal welchen Einrückstil: Bleibe einheitlich. Mir persönlich hat deine ursprüngliche Einrückung besser gefallen als das was du zuletzt geschrieben hast (sofern da jetzt keine Tabstops reingeraten sind die, die Formatierung im Forum anders darstellen).

    z.B.:

    int main() {
        while (true) {
            SetConsoleTitle(TEXT("Rechner"));
            ...
        }
    }
    

    oder:

    int main()
    {
        while (true)
        {
            SetConsoleTitle(TEXT("Rechner"));
            ...
        }
    }
    

    War wohl einfach nur ein bug habs editiert und jetzt ist es wieder normal


Log in to reply