Erste Programm - Kritik

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

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.

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