Bewertung und Verbesserung meines Programms



  • Hallo Freunde der Sonne,

    Ich habe ein kleines Programmchen geschrieben und wollte euch bitten, es zu zerpflücken, und mir soviele Tipps wie nur möglich geben, damit ich mir gleich einen optimalen Programmierstil angewöhnen kann. In meinen Programm muß man Rechenaufgaben lösen. Das ist einer meiner ersten Versuche, Programmcode in verschiedene Funktionen aufzuteilen. Ich habe sonst nur die Anweisungen aneinandergereiht.

    #include <stdio.h>
    #include <stdlib.h>
    
    #define WAHR 1
    #define FALSCH 0
    
    // übergeordnete Funktion - Aufgabe wird gestellt und muß gelöst werden
    void aufgabe(int zahl1, int zahl2, char operation);
    
    // Aufgabe wird erstellt + ausgegeben und Ergebnis der Aufgabe wird 
    // zurückgegeben
    int aufgabe_stellen(int zahl1, int zahl2, char operation);
    
    // Benutzer muß Lösung eingeben - Eingabe wird geprüft, Ergebnis der 
    // Prüfung wird zurückgegeben, benötigt zuvor Ergebnis für Vegleich
    int loesung_abfragen(int loesung);
    
    // liefert anhand des Prüfungsresultats eine entsprechende Meldung
    void resultat(int pruefung);
    
    int main(void)
    {
    	int anzahl_aufgaben = 1;
    	char aufgabenart = ' ';
    	printf("Programm kann mit Eingabe von \"0\" beendet werden\n");
    	printf("Die Division erfolgt ganzzahlig und ohne Rest\n\n");
    	while(anzahl_aufgaben != 0 && aufgabenart != '0')
    	{
    		printf("Anzahl Aufgaben: ");
    		scanf("%d", &anzahl_aufgaben);
    		printf("[+] Addition, [-] Subtraktion, [*] Multiplikation, [/] Division: ");
    		scanf(" %c", &aufgabenart);
    		int i;
    		for(i = 0; i < anzahl_aufgaben; i++)
    		{
    			int zahl1;
    			int zahl2;
    
    			if (aufgabenart != '+')
    			{
    				zahl1 = rand() % 20 + 1;
    				zahl2 = rand() % 20 + 1;
    			}
    			if (aufgabenart != '-')
    			{
    				zahl1 = rand() % 20 + 1;
    				zahl2 = rand() % 20 + 1;
    			}
    			if (aufgabenart != '*')
    			{
    				zahl1 = rand() % 15 + 1;
    				zahl2 = rand() % 15 + 1;
    			}
    			if (aufgabenart != '/')
    			{
    				zahl1 = rand() % 20 + 1;
    				zahl2 = rand() % 20 + 1;
    			}
    			aufgabe(zahl1, zahl2, aufgabenart);
    		}
    	}
    }
    
    void aufgabe(int zahl1, int zahl2, char operation)
    {
    	int ergebnis = aufgabe_stellen(zahl1, zahl2, operation);
    	int pruefung = loesung_abfragen(ergebnis);
    	resultat(pruefung);
    }
    
    int aufgabe_stellen(int zahl1, int zahl2, char operation)
    {
    	printf("%d %c %d = ", zahl1, operation, zahl2);
    	if (operation == '+') return zahl1 + zahl2;
    	if (operation == '-') return zahl1 - zahl2;
    	if (operation == '*') return zahl1 * zahl2;
    	if (operation == '/') return zahl1 / zahl2;
    }
    
    int loesung_abfragen(int loesung)
    {
    	int eingabe;
    	scanf("%d", &eingabe);
    	if (eingabe == loesung) return WAHR;
    	else return FALSCH;
    }
    
    void resultat(int pruefung)
    {
    	if(pruefung == WAHR) printf("Richtig!\n");
    	else printf("Falsch!\n");
    }
    


  • Unter anderem brauchst du kein WAHR oder FALSCH definieren, s gibt stdbool.h, das liefert true, false und bool mit. Auf Gleicheit oder Ungleichheit zu prüfen und dann wahr oder falsch zurückzugeben ist unnötig, du kannst auch gleich das Ergebnis von == oder != verwenden.
    Außerdem ist das Auswählen der Werte für die Unterschiedlichen Operatoren ziemlich merkwürdig, schließlich wird, würde '+' eingegeben, den beiden Variablen dreimal etwas zugewiesen. Vielleicht meintest du == statt != ? Selbst dann kannst du drei zusammenfassen oder gleich alle vier, würde sich der Spielraum beim Multiplizieren nicht unterscheiden.
    Womöglich solltest du, statt direkt mit dem eingegebenen Operatorenzeichen zu arbeiten, ein enum verwenden, dann bist du vom Eingabezeichensatz losgelöst.
    Statt printf ohne Formatspezifizierer kannst du auch gleich puts verwenden.
    Deutscher Quelltext ist ein no-go.



  • Erstens: Schreibe Englisch im Quellcode. Je früher du dir das angewöhnst, desto besser.

    Zweitens: Solange du nicht Prototyp und Funktionskörper bewusst trennen musst - Headerdateien, die Symbole nur bekanntmachen und nicht definieren sollen - schreibst du nach dem Prototypen am besten direkt den Körper. Sonst ist das nur wieder redundanter Code. Und nach wiederverwendbarem Bibliothekscode sieht das ja nicht aus.

    Drittens: du willst nicht Code auf Teufel komm raus in Funktionen unterteilen, sondern wo du eine oft durchzuführende Teilaufgabe soweit abstrahieren kannst, dass dir die Redundanz selbst auf den Zeiger geht.

    Viertens: Warum keine do-while Schleife für die Programmschleife? Der Code soll ja wenigstens einmal durchgelaufen werden.

    Fünftens: Ich definiere mir meine Variablen lieber direkt am Anfang einer Funktion, und nicht zwischendrin. Ist aber eher eine subjektive Präferenz; ich finde den Code so leichter zu lesen, und auf mich macht es den Eindruck, als ob der Programmierer wusste, was er tat.

    Sechstens: was willst du mit den if (aufgabenart != '<zeichen>') -Blöcken erreichen?



  • 1.) Fehlt da nicht noch ein srand(time(NULL))?
    2.) Schaue dir mal ein Tool ala doxygen an, welches aus Code ein HTML Dokumentation generiert. Gut das du dokumrntierst. 👍
    3.) Schau dir mal Enums an. Operation ist eine Aufzählung und sollte als enum definiert werden. Wenn du dann noch das Enum dokumentierst bin ich zufrieden. 😉
    4.) Ich bin kein Freund von einzeilgen if Anweisungen, da diese nicht ganz so achön zu debuggen ist. Aber das ist Geschmacksache.



  • Das sieht strukturell OK aus, separierbare Teilaufgaben in Funktionen aufzuteilen,

    - return in main() fehlt
    - Eingabeprüfungen bei deinen Nutzereingaben fehlen
    - int i aus Z 33 gehört in Z 34(for) oder Z 23
    - nach jedem scanf (dessen Rückgabewert übrigens eine Aussage über valide Eingaben gibt) gehört ein "while( getchar()!=EOF );"
    - Negativlogik in Z39/44/49/54 ist blöd und wartungsunfreundlich, besser hier switch/case und im Defaultzweig kannst du dann eine Fehlermeldung unterbringen
    - globale Integerkonstanten immer als enum, #define ist Scheiße: "enum {FALSCH, WAHR};"



  • Wutz schrieb:

    - return in main() fehlt

    Warum? Wenn ich "reaching the } that terminates the main function returns a value of 0." richtig interpretiere, ist auch in C kein return aus dem main nötig (in C++ weiß ich es sicher).

    Zusätzlich zu den anderen vielen richtigen Bemerkungen würde ich noch anmerken wollen, dass die Funktionsnamen (*) eher schlecht sind, ggf. auch die Dinge, die in der Funktion drin sind, nicht optimal gewählt sind. Deine eine Funktion "resultat", die nur ein printf ist, würde ich nicht machen. Die verwirrt ja fast nur.

    ad *): Man sollte an dem Funktionsnamen schon halbwegs gut erkennen können, was die Funktionen tun. Generische Begriffe wie "aufgabe" oder "resultat" sind eher schlecht geeignet. Diese passen auf jedes Problem, man weiß aber nicht, was diese "Aufgabe" oder dieses "Resultat" konkret macht/ist.
    "aufgabe_stellen" könnte dann eher "wendeOperatorAn" heißen. Hier zeigt sich dann, dass (wie schon erwähnt) englische Namen oft besser sind, u.a. weil sie oft kürzer sind und somit mehr Infos in den Namen passen und auch weil das natürlich in internationalen Teams besser ist.
    Es ist tatsächlich meiner Erfahrung nach so, dass Namensgebung ein relevantes Problem ist. Ist nicht immer einfach, aber je besser die Namen, desto leichter ist Code zu lesen / zu warten.

    Das hier:

    if (eingabe == loesung) return WAHR;
        else return FALSCH;
    

    würde man verkürzen auf: return eingabe == loesung; (verzichte auf WAHR und FALSCH!)

    Und zuletzt noch: der Codeabschnitt ab if (aufgabenart != '+') ist schlecht. Was soll der tun? (haben die anderen auch schon bemängelt)



  • Die Unterscheidung der Operationen, was zahlreich bemängelt wurde, sollte bewirken, dass je nach Aufgabentyp die Zahlen angepaßt werden. Ich sah gerade, daß ich das wohl ein bißchen vergessen habe. Beispielsweise ist eine Division schwerer, wenn die erste Zahl den Wert 80 und die zweite den Wert 20 annehmen kann, als wenn beide Zahlen maximal den Wert 15 annehmen.

    Zu der Sache mit den Funktionen: Ich dachte, man sollte Funktionen nicht nur aus Gründen der Redundanzvermeidung, sondern auch aus Gründen der Übersichtlichkeit erstellen. Jedoch bereue ich die zu feine Unterteilung der Aufgabenfunktion in Unterfunktionen, das war wirklich nur ein unnötiges Zersplittere.

    Das ich meine Quelltexte nur auf Deutsch schreibe hat den Grund, dass meine Programme wahrscheinlich eh keine internationalen Schlager werden und ich abgesehen davon jetzt auch nicht der Internationalist schlechthin bin. Ich habe auch schon auf Englisch geschrieben, was mir allerdings keine Schwierigkeiten bereitet hat, da mein Englisch doch Recht gut ist.

    Ich danke auch für die zusätztlichen Tipps! Ich werde mir die kommenden Tage auch nochmal ein paar Artikel zum Thema prozeduraler Programmierung zu Gemüte führen, vielleicht bringt's was...


Log in to reply