Wieso ist das Programm ein Alptraum?

Hab beschlossen, mal auf die Schnelle ein kleines Kartenspiel zu programmieren.
Leider kann ich nix anderes als C, und hab mal einfach drauf los programmiert.
Hab meinen bisherigen Quelltext dann meinem Bruder gezeigt, der ein 'echter'
Programmierer ist (der studiert Info), und der hat gemeint das Programm ist
ein Alptraum, und ich soll von vorne anfangen.
Wollte er da nur gemein sein, oder stimmt das, was er sagt? Warum ist das
Programm so ein Alptraum:

#include "WinCardGame.h"

struct AppData* GodObject;

int WINAPI WinMain(HINSTANCE hInst, HINSTANCE phInst, LPSTR commline, int showstyle){

	if(!(GodObject = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct AppData))))
		return EXIT_FAILURE;

#ifdef _DEBUG
	if(!(GodObject->logFile = fopen("DebugLog.txt", "w"))) 
		goto cleanup1;
#endif

	GodObject->CardImg = LoadImage(NULL, "Cards104_128.bmp", IMAGE_BITMAP, 0, 0, LR_LOADFROMFILE);
	if(!GodObject->CardImg){
#ifdef _DEBUG
		fprintf(GodObject->logFile, "LoadImage() ist defekt");
#endif
		goto cleanup1;
	}

	if(!WrapImageAndInitializeCards()) /* ok */
		goto cleanup2;

	CalcGUIElementLocations(); /* ok */
	GodObject->hInst = hInst;
	if(!CreateGUI()) /* ok */
		goto cleanup2;
	InitializePlayers();

	{
	unsigned appState = START_NEW_GAME;
	while(appState = handleState(appState)); /* ok. Verzweigt zu angemessenen Unterprogrammen, bis appState 0 zurueckkommt */
	}

	cleanup3(); /* TODO -> erst mal dummy, OS raeumt normalerweise eh auf */

/* Resourcen in umgekehrter Reihenfolge aufraeumen */
cleanup2:
	DeleteObject(GodObject->CardImg);

cleanup1:
#ifdef _DEBUG
	fclose(GodObject->logFile);
#endif
	HeapFree(GetProcessHeap(), 0, GodObject);
	return 0;
}

Bitte ehrlich sein! Ich kann Kritik gut vertragen! Wieso ist das Programm
ein 'Alptraum'?

Außerdem hätte ich noch ein kleines Problemchen:
Zur Zeit ist es so, dass sich die Größe der GUI sowie der GUI-Elemente
anpasst an die Größe der Bild-Resource! Das ist so, weil ich gelesen habe, dass
man lieber mit BitBlt statt mit StretchBlt arbeiten soll, weil StretchBlt
langsam wäre und durch das Vergrößern oder Verkleinern einfach die Bildqualität
stark abnähme. BitBlt funktioniert soweit super, allerdings: Ist das irgendwie
'merkwürdig' oder 'schlecht gelöst', wenn die Abmessungen der GUI so sehr von
den Abmessungen des Bitmaps abhängen? Sollte man trotzdem lieber StretchBlt
verwenden (dann könnte ma die GUI-Elemente flexibler in der Größe gestalten)?
Ist die Abnahme der Bildqualität wirklich so schlimm mit StretchBlt(), hat wer
Erfahrung mit der Funktion?

goto = nogo.
Aber einen Albtraum wüde ich es nicht nennen. Gibt es schlimmeres.
Wobei C allerdings ohnehin ein Albtraum ist

Marc++us

Ohne hier speziell das Programm zu diskutieren, goto ist in C ein durchaus akzeptabler Ersatz für das fehlende Exception-Handling, um sauberen Exit-Code zu erzeugen und Ressourcen aufzuräumen (das goto springt auf den Code, der bei C++ in einem catch stehen würde).

Marc++us schrieb:

Ohne hier speziell das Programm zu diskutieren, goto ist in C ein durchaus akzeptabler Ersatz für das fehlende Exception-Handling, um sauberen Exit-Code zu erzeugen und Ressourcen aufzuräumen (das goto springt auf den Code, der bei C++ in einem catch stehen würde).

was? es heißt doch immer von jeder seite dass man goto nicht benutzen soll oO

Marc++us

Ja, seltsam, oder?

Das wird immer so gesagt, aber gerade für C++-Programmierer ist goto meines Erachtens eine gute Krücke für Exception-Handling. Wenn man den Code ohne goto schreibt hat man ewige ifs und Flags, total unübersichtlich. Dann lieber aufgeräumt ein Sprung mit goto auf den Exit-Handler...

Gregor

Wenn Leute goto prinzipiell ablehnen, denken sie glaube ich eher an so etwas:

http://svn.r-project.org/R/trunk/src/appl/fft.c

TyRoXx

Gregor schrieb:

Wenn Leute goto prinzipiell ablehnen, denken sie glaube ich eher an so etwas:

http://svn.r-project.org/R/trunk/src/appl/fft.c

Das wurde dekompiliert, oder?

Bashar

*  These routines are based on code by Richard Singleton in the
 *  book "Programs for Digital Signal Processing" put out by IEEE.
 *
 *  I have translated them to C [...]

Marc++us schrieb:

Ja, seltsam, oder?

Das wird immer so gesagt, aber gerade für C++-Programmierer ist goto meines Erachtens eine gute Krücke für Exception-Handling. Wenn man den Code ohne goto schreibt hat man ewige ifs und Flags, total unübersichtlich. Dann lieber aufgeräumt ein Sprung mit goto auf den Exit-Handler...

Wer GOTO in einem C Programm braucht um den Code übersichtlich hinzukriegen, der hat generell beim Design der Software schon etwas falsch gemacht.

GOTO ist und bleibt ein nogo, das ist schlechter Programmierstil und das gilt auch für C Code.

Marc++us

Mit solch absoluten Meinungen tue ich mir eher schwer - das impliziert ja, daß man über den Vorschlag nicht mehr nachdenken muß und sich damit neuer Erkenntnisse versperrt.

Offensichtlich war goto wichtig genug um es nicht deprecated zu machen.

Einige interessante Beispiele stehen hier:

http://stackoverflow.com/questions/245742/examples-of-good-gotos-in-c-or-c

Man kann viele der klassischen Spaghetticode-Beispiele mit dem C-goto nur in pathologischen Beispielen bauen, wegen der erzwungenen Sprungbereiche.

Gerade Exit-Code gewinnt dramatisch an Klarheit durch goto, die andernfalls hilfsweise notwendigen Variablen und Abfragen führen zu weniger Klarheit.

if (!openDataFile())
  goto quit;
if (!getDataFromFile())
  goto closeFileAndQuit;
if (!allocateSomeResources)
  goto freeResourcesAndQuit;
// Do more work here....
freeResourcesAndQuit:
   // free resources
closeFileAndQuit:
   // close file
quit:
   // quit!

Aus dem verlinkten Thread. Klar, kurz, übersichtlich, keine Spaghetti.

Das geht auch ohne goto und es bleibt schöner strukturierter Code

int foo(....){
  if (!openDataFile())
    return 1;
  if (!getDataFromFile())
  {
    closeFile();
    return 1; /* oder 2, wer damit was machen will (z.B. Ausgabe der 
                 Fehlermeldung oder weiterführende delegierte Fehlerbehandlung
                 etc. */
  ) 
  if (!allocateSomeResources)
  {
    freeResourcesAndQuit();
    return 1; // siehe oben
  }

  // Do more work here....
  freeResourcesAndQuit();

  // free resources
  closeFile();

  // close file
  return 0;
  // quit!
}

cooky451

Programmierfachprofi schrieb:

Das geht auch ohne goto

Jo.

Programmierfachprofi schrieb:

und es bleibt schöner strukturierter Code

Ne, nicht mal ansatzweise. Sieht man schon daran, dass dein Code einen Fehler hat. Guck dir mal Zeile 13 an.

cooky451 schrieb:

Ne, nicht mal ansatzweise. Sieht man schon daran, dass dein Code einen Fehler hat. Guck dir mal Zeile 13 an.

Das ist nur eine Namensbenennung, die ich nicht umbenannt habe.
Also Schall und Rauch.

Mach aus dem freeResourcesAndQuit(); in 13 und 18 einfach ein freeResources(); dann paßt es.

cooky451

Und du raffst es nicht mal mit Hinweisen, da fehlt ein closeFile(). q.e.d.

C++ Forumbot

Dieser Thread wurde von Moderator/in rapso aus dem Forum Spiele-/Grafikprogrammierung in das Forum C (C89 und C99) verschoben.

Im Zweifelsfall bitte auch folgende Hinweise beachten:
C/C++ Forum :: FAQ - Sonstiges :: Wohin mit meiner Frage?

Dieses Posting wurde automatisch erzeugt.

Ich halte die goto-als-RAII-Ersatz-Denkweise nicht für richtig sinnvoll -- der entstehende Code mag manchmal (wenn auch da streitbar) übersichtlicher sein als der gleiche Code mit Rückgabewertbehandlung, aber das ist weniger ein Argument für die Verwendung von goto als gegen Funktionen, die so viel Ressourcen verwalten, dass man den Überblick verliert bzw. nebenher noch massig Kram zu tun versuchen.

Im vorliegenden Fall sehe ich beispielsweise keinen Grund, *GodObject auf den Heap zu legen (der Name "GodObject" lässt eh nichts gutes vermuten, zumal es ein globaler Zeiger ist), und die Initialisierung respektive Zerstörung gehört in eine eigene Funktion. In der main selbst sollte m.E. nicht viel mehr stehen als

int main() {
  struct AppData app;

  if(InitAppData(&app)) {
    PlayGame(&app);
    DestroyAppData(&app);
  } else {
    return -1;
  }
}

InitAppData hat dann im Fehlerfall dafür zu sorgen, dass keine Aufräumarbeiten übrig bleiben. Wenn struct AppData mehrere Ressourcen verwalten muss, ist die Kaskadenproblematik damit natürlich nicht komplett entfernt, aber sie ist dann in einer Funktion isoliert, die sich um nichts anderes kümmert, und das sollte sich dann, sofern es sich nicht um ein Monsterobjekt handelt, ohne Übersichtlichkeitsprobleme regeln lassen. Wenn es sich um ein Monsterobjekt handelt, ist dieser Umstand das Problem, und das Monsterobjekt sollte in mehrere Unterstructs unter- oder aufgeteilt werden.

hustbaer

"if error goto exit" ist eines der Standardmuster in der C-Programmierung.
Vieles kann man ohne goto schöner lösen, aber manchmal sind die Alternativen einfach nur Overkill.
C ist einfach eine hässliche Sprache, und der gezielte Einsatz von goto macht sie stellenweise um eine kleine Spur ~~schöner~~ weniger hässlich.

cooky451

seldon schrieb:

als der gleiche Code

Konsequent weitergedacht bedeutet das, dass keine Funktion mehr als eine Ressource reservieren darf. Das halte ich nicht immer für die beste Lösung. Somit sind wir wieder bei goto. Oder man rückt den Good-Path ein:

if (openDataFile())
{
  if (getDataFromFile())
  {
    if (allocateSomeResources())
    {
      // Do more work here...
      freeSomeResources();
    }
    freeData();
  }
  closeFile();
}

Skaliert natürlich auch nicht besonders gut, aber vielleicht gut genug, um sagen zu können, dass man den Rest in Funktionen aufteilt.

Marc++us

@Programmierfachprofi:

Man sieht deutlich, daß Deine Variante semantisch gleich ist, durch die notwendige Duplizierung von Code aber fehleranfälliger und damit auch schlechter wartbar. Dein Beispiel war eigentlich wunderbar geeignet, um die Vorteile eines goto zu beleuchten.

@cooky:

Das geht natürlich, aber denk Dir das mal für Init-Abläufe bei einer hardwarenahen Sache wie einer Abfolge von DirectX-Resourcen-Anforderungen o.ä., wenn Du da jedesmal eine Funktion mit Parametern und Rückmeldewert schreibst, am Ende hast Du da ja für jede API-Funktion eine Wrapper-Funktion gebaut... und übersichtlich ist es nicht wirklich. Noch dazu weil dann sämtliche Beispiele aus der API-Doku direkt mit den API-Funktionen arbeiten, dann muß man alle Beispiele auf die eigenen Funktionen umschreiben. Da verzettelt man sich wohl relativ leicht.

Wenn man so arbeitet, wieso nimmt man nicht gleich C++ und kapselt es in Klassen. Das wäre dann konsequenter.

Bitte ein Bit

//...
  cleanup3(); /* TODO -> erst mal dummy, OS raeumt normalerweise eh auf */
  //...

@Lotte89
Ein solcher Kommentar spiegelt auch keinen guten Stil wider. Wer Resourcen allokiert sollte auch diese wieder freigeben.

Ja, das OS gibt Resourcen wieder frei. Aber stell dir vor, du willst die Stelle in eine Funktion packen und weiterentwicklen (Funktion in Schleife). Dann hast du von heute auf morgen ein schönes Speicherloch.