Hässlicher Code

Vertexwahn

Eigentlich eine einfache Aufgabe - folgender Datentyp liegt für Brüche vor:

typedef struct Fraction {
	bool		sign;		// + = false, - = true
	unsigned int	numerator;
	unsigned int	denominator;
} Fraction;

Zu programmieren ist ein Funktion, die zwei Brüche addiert - sieht bei mir wie folgt aus:

Fraction *AddFraction(Fraction *a, Fraction *b, Fraction *sum)
{
	assert(a);	   // a != NULL
	assert(b);     // b != NULL
	assert(sum);   // sum != NULL

	if(!a || !b || !sum)
		return NULL;

	if(sgnFraction(a) == -1 && sgnFraction(b) == 1  ||		// 3. case
	   sgnFraction(a) == -1 && sgnFraction(b) == 0)			// 6. case
	{
		if(a->numerator * b->denominator >= b->numerator * a->denominator)   // |a| >= b?
		{
			sum->sign        = MINUS;
			sum->numerator	= a->numerator   * b->denominator - b->numerator * a->denominator;
			sum->denominator	= a->denominator * b->denominator;
		}
		else
		{
			sum->sign		= PLUS;
			sum->numerator	= b->numerator * a->denominator - a->numerator * b->denominator;
			sum->denominator	= a->denominator * b->denominator;
		}
	}
	else
	{
		if(sgnFraction(a) == 1 && sgnFraction(b) == -1  ||		// 7. case
		   sgnFraction(a) == 0 && sgnFraction(b) == -1)			// 8. case
		{
			if(a->numerator * b->denominator >= b->numerator * a->denominator)		// a >= |b|
			{
				sum->sign        = PLUS;
				sum->numerator	= a->numerator * b->denominator - b->numerator * a->denominator;
				sum->denominator	= a->denominator * b->denominator;
			}
			else
			{
				sum->sign		= MINUS;
				sum->numerator	= b->numerator * a->denominator - a->numerator * b->denominator;
				sum->denominator	= a->denominator * b->denominator;
			}
		}
		else
		{	// left cases
			sum->sign		= a->sign;
			sum->numerator	= a->numerator * b->denominator + b->numerator * a->denominator;
			sum->denominator	= a->denominator * b->denominator;
		}
	}

	//// ReduceFraction Fraction	
	ReduceFraction(sum);

	return sum;
}

ich nehm mal an, dass da keiner mehr durchblick was die Funktion überhaupt tun soll und außerdem sieht irgendwie alles so aus, als ob es sehr umständlich gelöst wäre (bzw. ist :()

keine Ahnung wie ich das "schöner" machen kann - habt ihr ein paar Tipps für mich, wie man so etwas transparenter macht

PS: der Komplette Code ist hier zu finden:
http://turing.fh-landshut.de/~jamann/Fraction.c
http://turing.fh-landshut.de/~jamann/Fraction.h
http://turing.fh-landshut.de/~jamann/Testbed_Fraction.c

Daniel E.

Vertexwahn schrieb:

keine Ahnung wie ich das "schöner" machen kann - habt ihr ein paar Tipps für mich, wie man so etwas transparenter macht

Erst mal finde ich das etwas seltsam, das Vorzeichen von dem Zähler zu trennen, aber gut. Mein Vorschlag: die Nenner auf einen gemeinsamen kgV bringen, die Zähler (gem. der Vorzeichen) addieren, den Bruch kürzen, fertig.

groovemaster

Vertexwahn schrieb:

und außerdem sieht irgendwie alles so aus, als ob es sehr umständlich gelöst wäre (bzw. ist :()

Das siehst du richtig.

Vertexwahn schrieb:

keine Ahnung wie ich das "schöner" machen kann - habt ihr ein paar Tipps für mich, wie man so etwas transparenter macht

Ich würde auf jedenfall das Vorzeichen nicht separat handeln. Das macht vieles nur unnötig kompliziert. Ich würde deshalb Zähler und Nenner signed machen.
Ansonsten ist die Vorgehensweise schon richtig. Letztendlich sollte es ungefähr wie folgt aussehen

Fraction *AddFraction(Fraction *a, Fraction *b, Fraction *sum)
{
    assert(a);       // a != NULL
    assert(b);     // b != NULL
    assert(sum);   // sum != NULL

    if(!a || !b || !sum)
        return NULL;

    sum->numerator = a->numerator * b->denominator + b->numerator * a->denominator;
    sum->denominator = a->denominator * b->denominator;
    ReduceFraction(sum);

    return sum;
}

DrGreenthumb

groovemaster schrieb:

assert(a);       // a != NULL
    assert(b);     // b != NULL
    assert(sum);   // sum != NULL
   
    if(!a || !b || !sum)
        return NULL;

das war wohl nichts...

groovemaster

DrGreenthumb schrieb:

das war wohl nichts...

Was meinst du damit?
Ich hab den Code ja lediglich übernommen. Wenn jemand sowas für sinnvoll hält, dann soll er's halt machen.

DrGreenthumb

groovemaster schrieb:

DrGreenthumb schrieb:

das war wohl nichts...

Was meinst du damit?
Ich hab den Code ja lediglich übernommen. Wenn jemand sowas für sinnvoll hält, dann soll er's halt machen.

wenn das jemand für sinnvoll hält, hat er schlichtweg den sinn nicht verstanden.

Vertexwahn

DrGreenthumb schrieb:

wenn das jemand für sinnvoll hält, hat er schlichtweg den sinn nicht verstanden.

was ist daran verkehrt?

assert wird ja bei der Release Version ignoriert - falls doch mal der Fall der Fälle eintreffen, sollte dann soll halt einfach NULL zurückgegeben werden

groovemaster schrieb:

Ich würde auf jedenfall das Vorzeichen nicht separat handeln.

Am Datentyp für den Bruch darf ich nichts ändern

Shade Of Mine

Vertexwahn schrieb:

assert wird ja bei der Release Version ignoriert - falls doch mal der Fall der Fälle eintreffen, sollte dann soll halt einfach NULL zurückgegeben werden

Und genau das ist falsch.

Entweder es ist ein Fehler oder ein erlaubter wert. beides gleichzeitig geht nicht.

debug version muss sich so wie release version verhalten, sonst ist testen nicht gut moeglich, weil du doppelt soviel testen muesstest (was du natuerlich nicht machst, weil ja sowieso schon zu wenig getestet wird).

wenn nun NULL zurueck geliefert wird, dann wird es den caller vermutlich sowieso aufhauen, weil er nicht auf NULL testen wird (wozu auch? bzw. wenn er es wird, ist der code ja ungetestet...).

fazit: entweder ist etwas erlaubt oder nicht. ein bisschen erlaubt ist wie ein bisschen schwanger

DrGreenthumb

Vertexwahn schrieb:

DrGreenthumb schrieb:

wenn das jemand für sinnvoll hält, hat er schlichtweg den sinn nicht verstanden.

was ist daran verkehrt?

assert wird ja bei der Release Version ignoriert - falls doch mal der Fall der Fälle eintreffen, sollte dann soll halt einfach NULL zurückgegeben werden

das passt logisch nicht zusammen. Entweder die funktion aktzeptiert null-werte oder nicht. Daran muss man sich halten, wenn man die Funktion aufruft. Wenn die Argumente nicht null sein dürfen, es aber trotzdem sind, ist es ein Bug. Durch die if-Abfrage wird dieser in der Release-Version nur verschleiert und damit ist keinem geholfen.

Vertexwahn

yup - du hast recht - ist inkonsequent - werde ich ändern