Erste Programm - Kritik

[flammus]

Hallo,
bin jetzt mit meinem ersten Programm (na gut, nach 'Hello World') fertig. Ich habe schon etwas Erfahrung in VB.NET und kann mich noch erinnern, dass meine ersten Tools schrecklich waren. Eine Menge Bugs und noch mehr unschöner Code.

Ich würde gerne mal ein kleines Feedback hören, am besten die harte Wahrheit. Ich möchte diesmal von Anfang an unschönen Code vermeiden. Erzählt mir bitte einfach kurz, wie ich mich mache und was man verbessern könnte / muss

Es handelt sich hierbei um das Spiel "HangMan" in einer Consolen-Anwendung und besteht nur aus einer Funktion neben der Main:

#include <iostream>

#include <stdlib.h>
#include <stdio.h>
#include <ctype.h>
using namespace std;
void Play(string word);
string xStr;
string oldStr = "";
int wordLength;
int main() {
	//RateWort wird festgelegt
	string word = "HangMan";

	//Wortlänge wird bestimmt für die Schleifen
	wordLength = word.length();

	//"Password-Chars"
	for (int i = 0; i < wordLength; i++) {
		xStr += "-";
	}

	oldStr = xStr;
	cout << "Los geht's" << endl;
	cout << xStr << endl;

	Play(word);
	return 0;
}
void Play(string word) {
	//Falls eine Charkette eingegeben wird, zählt der erste Buchstabe

	string read;
	char firstLetter;
	cin >> read;
	firstLetter = read[0];

	xStr = "";
	//Für jeden Buchstaben
	for (int ii = 0; ii < wordLength; ii++) {
		//ersten Buchstaben als Vergleichschar wählen
		char vglChar = firstLetter;

		//Wenn der Buchstabe dem Vergleichschar des Users entspricht
		if (tolower(word[ii]) == tolower(vglChar)) {
		//LösungsWortBuchstabe = UserEingabe

			//Die oldStr-Überprüfung, damit bereits gelöste Buchstaben
			//nicht wieder überschrieben werden

			if (oldStr[ii] == '-' || oldStr[ii] == 0) {
				if (xStr[ii] == '-' || xStr[ii] == 0) {
					//Wenn noch nicht gelöst
					xStr += word[ii];
				}
			} else {
				//Wenn schon gelöst, gelösten Buchstaben adden
				xStr += oldStr[ii];
			}
		}	else {
			//LösungsWortBuchstabe != UserEingabe

			//Die oldStr-Überprüfung, damit bereits gelöste Buchstaben
			//nicht wieder überschrieben werden
			if (oldStr[ii] == '-' || oldStr[ii] == 0) {
				if (xStr[ii] == '-' || xStr[ii] == 0) {
					//nicht gelöste Buchstaben als Passwort-Char anzeigen
					xStr += '-';
				}
			} else {
				//Wenn schon gelöst, gelösten Buchstaben adden
				xStr += oldStr[ii];
			}
		}
	}

	//oldStr aktualisieren
	oldStr = xStr;

	//Ausgabe, die anzeigt, ob Erfolg
	cout << xStr << endl;

	//Wenn Wort noch nicht vollständig gelöst -> Rekursion
	if (xStr != word) {
		Play(word);
	} else {
	//Ansonsten Erfolgsmeldung
	cout << "Gelöst ;)" << endl;
	cout << "Beliebige Taste zum Beenden drücken...";
	//Beenden
	cin.get();
	}
	return;
}

Danke

[hustbaer]

string xStr; 
string oldStr = "";

Nichtssagende Namen sind nie gut.

for (int i = 0; i < wordLength; i++) { 
    xStr += "-"; 
}

Ist OK, geht aber einfacher (kürzer) mit

xStr = string(wordLength, '-');

string read; 
char firstLetter; 
cin >> read; 
firstLetter = read[0];

Und was passiert wenn der Input-Stream "aus" ist? Dann ist "read" leer und es klescht.

for (int ii = 0; ii < wordLength; ii++)

Wieso ii und nicht einfach nur i? Wäre üblicher...

if (tolower(word[ii]) == tolower(vglChar)) {

tolower(vglChar) bleibt in der Schleife ja immer gleich, du kannst den tolower-Aufruf also genau so gut aus der Schleife rausziehen.

[hustbaer]

Was jetzt kommt ist einigermassen schmerzhaft...

if (tolower(word[ii]) == tolower(vglChar)) { 
	if (oldStr[ii] == '-' || oldStr[ii] == 0) {   // Der Check oldStr[ii] == 0 ist unnötig - wozu soll der gut sein?

		// Du hast xStr vor der Schleife auf "" (=Länge 0) gesetzt.
		// D.h. der string enthält beim 1. Durchlauf genau NULL Zeichen, keine, nix.
		// D.h. du darfst auch auf keine Zeichen über xStr[index] zugreifen, auch nicht mit index == 0.
		// Was du da machst wird zwar mit quasi allen Implementierungen funktionieren,
		// aber im Debug-Mode solltest du zumindest ein assert() bekommen, und ... trotz dem es wohl funktionieren wird ist es falsch.
		// Und WENN es funktioniert, dann ist der Wert den du bekommst immer 0, weil du den "präventiv angehängten Null-Terminator" bekommst.
		// D.h. das "if" hier gibt immer "true", kann also weg.
		if (xStr[ii] == '-' || xStr[ii] == 0) {
			xStr += word[ii]; 
		} 
	} else { 
		xStr += oldStr[ii]; 
	} 
} else {
	if (oldStr[ii] == '-' || oldStr[ii] == 0) { 
		// Hier gilt das selbe wie oben: der Zugriff xStr[ii] ist falsch, und das if kann weg.
		if (xStr[ii] == '-' || xStr[ii] == 0) { 
			xStr += '-'; 
		} 
	} else { 
		xStr += oldStr[ii]; 
	} 
}

Bauen wir das also mal um:

if (tolower(word[ii]) == tolower(vglChar)) { 
	if (oldStr[ii] == '-') {
		xStr += word[ii];
	} else {
		xStr += oldStr[ii];
	}
} else {
	if (oldStr[ii] == '-') { 
		xStr += '-'; 
	} else { 
		xStr += oldStr[ii]; 
	} 
}

Fällt dir was auf?
Wenn oldStr[ii] ein '-' ist dann ein '-' anhängen und sonst oldStr[ii] anhängen.
Also sinngemäss... wir schreiben jeden Tag auf was für ein Tag ist, ausser Sonntag, da schreiben wir auf dass Sonntag ist.
Anders gesagt: das letzte "if" ist auch unnötig:

if (tolower(word[ii]) == tolower(vglChar)) { 
	if (oldStr[ii] == '-') {
		xStr += word[ii];
	} else {
		xStr += oldStr[ii];
	}
} else {
	xStr += oldStr[ii]; 
}

Und jetzt können wir noch die beiden else-Zweige zusammenfassen, weil die eh das gleiche machen:

if (tolower(word[ii]) == tolower(vglChar)
    && oldStr[ii] == '-')
{ 
	xStr += word[ii];
} else {
	xStr += oldStr[ii];
}

Und siehe da, wieder redundanter Code.
Wenn gefunden, und in oldStr noch nicht gefunden, dann word[ii] anhängen, sonst oldStr[ii] anhängen.
Öhm. Also das && oldStr[ii] == '-' hier ist überflüssig.
Weil ... wenn gefunden, dann soll eh immer word[ii] angehängt werden. Wenn nämlich oldStr[ii] nicht gleich '-' ist, dann kann das nur deswegen sein, weil vorher schonmal word[ii] an der Stelle angehängt wurde. Also wenn oldStr[ii] nicht '-' ist dann ist es gleich word[ii].

D.h. das && kann weg und dann haben wir

if (tolower(word[ii]) == tolower(vglChar))
{ 
	xStr += word[ii];
} else {
	xStr += oldStr[ii];
}

So. Jetzt ist auch endlich mal verständlich was der ganze Code sollte.

Und jetzt fällt auch auf, dass du das noch billiger haben kannst. Wozu xStr immer löschen und dann neu aufbauen? Statt dessen kannst du gleich einfach die Zeichen in xStr überschreiben die gelöst wurden.
Dadurch wird dann auch oldStr überflüssig, d.h. die Variable entfernst du ganz. Und das xStr="" vor der Schleife kommt weg, und das if in der Schleife wird zu

if (tolower(word[ii]) == tolower(vglChar)) { 
	xStr[ii] = word[ii];
}

Tadaaaa.

Liest sich irgendwie einfacher wie ich finde.

[hustbaer]

//Wenn Wort noch nicht vollständig gelöst -> Rekursion 
if (xStr != word) { 
    Play(word); 
} else { 
    //Ansonsten Erfolgsmeldung 
    cout << "Gelöst ;)" << endl; 
    cout << "Beliebige Taste zum Beenden drücken..."; 
    //Beenden 
    cin.get(); 
} 
return;

Also Rekursion deren max. Tiefe du nicht kontrollieren kannst finde ich nicht gut. Und Rekursion wo es - so wie hier - überhaupt nicht nötig ist, finde ich auch nicht gut.
(Die Tiefe könntest du kontrollieren indem du dir falsch geratene Buchstaben merkst -- gibt ja nur endlich viele Buchstaben. Das 2. Argument bleibt aber.)
Ich würde eher einfach true/false zurückgeben, und Play dann in einer Schleife aufrufen.

Die "Gelöst" Ausgabe an der Stelle finde ich auch komisch. Ist eigenartig "asymmetrisch" - der "Start-Text" ("Los geht's") wird in main() ausgegeben, der "End-Text" ("Gelöst") aber in Play().

[hustbaer]

Zusammengefasst, mit 1-2 weiteren kleinen "Schönheitskorrekturen":

#include <iostream> 
#include <string> 
#include <cctype>

using namespace std; 

bool Play(string word); 

string xStr; 

int main() { 
	string word = "HangMan"; 

	xStr = string(word.length(), '-');

	cout << "Los geht's" << endl; 

	bool done = false;
	while (!done)
	{
		cout << xStr << endl; 
		done = Play(word);
	}

	cout << "Ferdich, hurrah!" << endl; 

	return 0; 
} 

bool Play(string word) { 

	string inputString; 
	cin >> inputString; 

	if (!cin || inputString.empty()) {
		cout << "WTF!?!" << endl;
		exit(1);
	}

	char const inputChar = inputString[0]; 
	char const inputCharLower = tolower(inputChar);

	for (size_t i = 0; i < word.length(); i++) { 
		if (tolower(word[i]) == inputCharLower) { 
			xStr[i] = word[i]; 
		}
	}

	return xStr == word;
}

Da finde ich jetzt immer noch ein paar Dinge komisch, aber das sollte für's erste mal reichen.

[Werner Salomon]

sicher kein Anfängercode, aber viel lustiger:

#include <algorithm> // transform
#include <ctime> // time
#include <cstdlib> // (s)rand
#include <iostream>
#include <locale> // ctype
#include <string>

int main()
{
    using namespace std;
    string words[] = {
        "Yachtclubbesitzer",
        "Bettelmoench",
        "HangMan",
        "Motoraufhaengung",
        "Hundefaenger",
        "Koelnisch-Wasser"
        "Neuschwanstein",
        "Regenbogenforelle",
        "USB-Stick",
        "Grobmotoriker"
    };
    srand( time(0) );
    const string& word = words[rand() % sizeof(words)/sizeof(*words)];

    cout << "Bitte Buchstaben eingeben, un das Wort zu raten" << endl;
    const ctype< char >& ct = use_facet< ctype< char > >( locale() );
    string result( word.size(), '-' );
    for( char c; result != word && cin >> c; cout << result << endl )
        transform( begin(word), end(word), begin(result), begin(result),
            [&ct, &c]( char w, char r ) { return ct.tolower( c ) == ct.tolower( w )? w: r; } );
    if( result == word )
        cout << "<< Ferdich, hurrah! >>" << endl;
    return 0;
}

Gruß
Werner

[Gast]

Das letzte Beispiel ist heftig. Ich bin auch dabei C++ in Verbindung mit Qt zu lernen. Ich lese lieber einen Code der mehr Zeilen hat wir nötig, aber dafür sehr übersichtlich ist, als wenn viel in eine Zeile gequetscht wird.

Wie sieht es eigentlich mit den Blockklammern aus, setzt macht man die nicht in C++ anders als in Java? Von der Funktion her ist es ja egal, aber gibt es da nicht irgendwelche Codingrulez unter den C++ Programmmierern?

[Mr X]

Butterbrot schrieb:

Wie sieht es eigentlich mit den Blockklammern aus, setzt macht man die nicht in C++ anders als in Java? Von der Funktion her ist es ja egal, aber gibt es da nicht irgendwelche Codingrulez unter den C++ Programmmierern?

Es gibt sehr viele verschiedene Stile - und im Grunde alle Varianten lassen sich in der Praxis auch finden:

if(foo) {
} else {
}

if(foo)
{
}
else
{
}

if(foo) {
}
else {
}

if(foo)
    {
    }
else
    {
    }

[CatDog11]

@Werner oder sonst jemand

Was macht dieser Teil in transform (VS2008 meckert bei mir ausserdem: Syntaxfehler: '[')

[&ct, &c]( char w, char r ) { return ct.tolower( c ) == ct.tolower( w )? w: r; }

MfG

[Michael E.]

Das ist ein Lambda-Ausdruck. Den gibt es erst seit C++11, d.h. du musst einen neueren Compiler nehmen oder den Lambda-Ausdruck als normale Funktion schreiben.

[Der-neue-Standard-C++11]

CatDog11 schrieb:

Was macht dieser Teil in transform (VS2008 meckert bei mir ausserdem: Syntaxfehler: '[')

[&ct, &c]( char w, char r ) { return ct.tolower( c ) == ct.tolower( w )? w: r; }

siehe C++11-Standard Kapitel 5.1.2 Lambda expressions. VS2008 kann das nicht, VS2010 kann es.

[pumuckl]

CatDog11 schrieb:

Was macht dieser Teil in transform

Das nennt sich Lambda(closure) und erzeugt, simpel ausgedrückt, ein Funktionsobjekt, dessen operator() zwei chars als Argumente erwartet und damit das macht, was zwischen den geschweiften Klammern steht.

(VS2008 meckert bei mir ausserdem: Syntaxfehler: '[')

Kein Wunder, Lambdas sind ein C++11 Feature, das vom VS2008 noch nicht unterstützt wird.

C++03-Version:

//irgendwo*

struct LowerComp {
  char& c;
  const ctype< char >& ct;
  LowerLess(char& c, const ctype< char >& ct) : c(c), ct(ct) {}
  char operator()( char w, char r ) 
  { return ct.tolower( c ) == ct.tolower( w )? w: r; }
};

//dann:
transform( begin(word), end(word), begin(result), begin(result),
            LowerComp(c, ct));

______________________
* Nicht irgendwo, sondern außerhalb jeglicher Funktionen, C++03 erlaubt keine lokalen Typen in Templates

[CatDog11]

Vielen Dank für die ausführliche Erklärung, somit erklärt sich das Ganze

MfG

[CatDog11]

Falls es irgendjemand interessiert noch zusammengefasst eine Version, welche bei mir zumindest in VS2008 lauffähig ist (weil ich das kleine Progrämmchen von Werner eigtl. ziemlich cool finde). Vielleicht interessierts ja jemand

#include <algorithm> // transform
#include <ctime> // time
#include <cstdlib> // (s)rand
#include <iostream>
#include <locale> // ctype
#include <string> 

struct LowerComp {
  char& c;
  const std::ctype<char>& ct;
  LowerComp(char& lc, const std::ctype<char>& lct) : c(lc), ct(lct) {}
  char operator()( char w, char r ) { return ct.tolower( c ) == ct.tolower( w ) ? w : r; }
}; 

int main()
{
	using namespace std;
    string words[] = {
        "Yachtclubbesitzer",
        "Bettelmoench",
        "HangMan",
        "Motoraufhaengung",
        "Hundefaenger",
        "Koelnisch-Wasser",
        "Neuschwanstein",
        "Regenbogenforelle",
        "USB-Stick",
        "Grobmotoriker"
    };
    srand( (unsigned int) time(0) );
    const string& word = words[rand() % sizeof(words)/sizeof(*words)];

    cout << "Bitte Buchstaben eingeben, un das Wort zu raten" << endl;
    const ctype< char >& ct = use_facet< ctype< char > >( locale() );
    string result( word.size(), '-' );

    for( char c; result != word && cin >> c; cout << result << endl )
	{
	     transform( word.begin(), word.end(), result.begin(), result.begin(), LowerComp(c, ct));
	}

    if( result == word )
        cout << "<< Ferdich, hurrah! >>" << endl;

    return 0; 
}

MfG