WinLife Simulation bitte testen

[Gregor]

Gregor schrieb:

Ist Kritik am Quellcode erwünscht?

Ach, ich geh einfach mal davon aus, dass das der Fall ist. Sonst hättest du den Quellcode wohl nicht zum Download freigegeben. Ich poste einfach mal 2 Stellen aus deinem Quellcode, ohne etwas dazu zu sagen. Vielleicht kommst du ja selbst drauf, was ich an den entsprechenden Stellen schlecht finden könnte.

Stelle 1:

if(PosX<1)		//left side					
	{
		if(Array[xright][PosY].exist)		
			count+=1;

		if(PosY<1)	
		{
			if(Array[PosX][ydown].exist)						
				count+=1;
			if(Array[xright][ydown].exist)
				count+=1;
		}
		else if(PosY==(Height-1))
		{
			if(Array[PosX][yup].exist)
				count+=1;
			if(Array[xright][yup].exist)
				count+=1;
		}
		else
		{
			if(Array[PosX][ydown].exist)
				count+=1;
			if(Array[xright][ydown].exist)
				count+=1;
			if(Array[PosX][yup].exist)
				count+=1;
			if(Array[xright][yup].exist)
				count+=1;
		}
	}
	else if(PosX==(Width-1))			//right side
	{
		if(Array[xleft][PosY].exist)
			count+=1;
		if(PosY<1)
		{
			if(Array[xleft][ydown].exist)
				count+=1;
			if(Array[PosX][ydown].exist)
				count+=1;
		}
		else if(PosY==(Height-1))
		{
			if(Array[xleft][yup].exist)
				count+=1;
			if(Array[PosX][yup].exist)
				count+=1;
		}
		else 
		{
			if(Array[xleft][ydown].exist)
				count+=1;
			if(Array[PosX][ydown].exist)
				count+=1;
			if(Array[xleft][yup].exist)
				count+=1;
			if(Array[PosX][yup].exist)
				count+=1;
		}
	}
	else if(PosY<1)		//top
	{
		if(Array[xleft][ydown].exist)
				count+=1;
		if(Array[PosX][ydown].exist)
				count+=1;
		if(Array[xright][ydown].exist)
				count+=1;
		if(Array[xleft][PosY].exist)
				count+=1;
		if(Array[xright][PosY].exist)
				count+=1;
	}
	else if(PosY==(Height-1))	//bottom
	{
		if(Array[xleft][yup].exist)
				count+=1;
		if(Array[PosX][yup].exist)
				count+=1;
		if(Array[xright][yup].exist)
				count+=1;
		if(Array[xleft][PosY].exist)
				count+=1;
		if(Array[xright][PosY].exist)
				count+=1;
	}
	else	//somewhere in the middle of the array (no border-cell)
	{
		if(Array[xleft][PosY].exist)
				count+=1;
		if(Array[xleft][ydown].exist)
				count+=1;
		if(Array[xleft][yup].exist)
				count+=1;
		if(Array[PosX][ydown].exist)
				count+=1;
		if(Array[PosX][yup].exist)
				count+=1;
		if(Array[xright][PosY].exist)
				count+=1;
		if(Array[xright][ydown].exist)
				count+=1;
		if(Array[xright][yup].exist)
				count+=1;
	}

Stelle 2:

if(Array)
	{
		for(int i=0;i<Width;i++)
		{
			delete[] Array[i];
		}
		delete[] Array;
		Array=0;
	}
	if(CmpArray)
	{
		for(int i=0;i<Width;i++)
		{
			delete[] CmpArray[i];
		}
		delete[] CmpArray;
		CmpArray=0;
	}

Ich denke, dass es noch ne Menge weiterer Stellen in der Art in deinem Code gibt, werde aber nicht danach suchen.

[ethereal]

Jepp, Kritik am Quellcode ist natürlich erwünscht.
Hmm, also der obere Quellcode enthält bestimmt jede Menge redundante Abfragen, vielleicht stört dich auch, ob hier nicht geprüft wird, ob PosX/PosY überhaupt im Array liegen. Ich muß zugeben, ich finde dieses Stück Code leider auch nicht schön, aber mir ist da nix anderes eingefallen und ich wollte mich nicht zu lange mit diesem Abschnitt aufhalten, da es mir eher um die SDI-Entwicklung ging. Ich werde mir aber nochmal eine bessere Variante überlegen, habe da auch schon Ansätze.
Beim zweiten stören dich vielleicht die zwei getrennten Schleifen? k.A.

Vielleicht könntest du doch nochmal posten, was du meintest?
Thx

[Erhard Henkes]

Ich denke, dass hier auch bezüglich OOP Verbesserungen möglich sind.

[ethereal]

@Gregor: Den oberen Quellcode habe ich etwas verkürzt:

int LifeMap::GetNumberOfNeighbours(int PosX,int PosY)
{		
int count=0;			
	int xright=PosX+1;
	int	xleft=PosX-1; 
	int ydown=PosY+1;
	int	yup=PosY-1;

	if(!(xleft<0)&&(Array[xleft][PosY].exist))
		count++;
	if(!(xleft<0)&&!(yup<0)&&(Array[xleft][yup].exist))
		count++;
	if(!(xleft<0)&&!(ydown>Height)&&(Array[xleft][ydown].exist))
		count++;
	if(!(ydown>Height)&&(Array[PosX][ydown].exist))
		count++;
	if(!(yup<0)&&(Array[PosX][yup].exist))
		count++;
	if(!(xright>=Width)&&(Array[xright][PosY].exist))
		count++;
	if(!(xright>=Width)&&!(yup<0)&&(Array[xright][yup].exist))
		count++;
	if(!(xright>=Width)&&!(ydown>Height)&&(Array[xright][ydown].exist))
		count++;
return count;
}

Aber war es das, was du meintest???

@Erhard: Ja, das denke ich auch. Ich bin allerdings nicht so bewandert was OOP angeht.
Eine Sache ist mir allerdings aufgefallen, was mich die ganze Zeit schon gestört hat:
Die SDI-Anwendungen basieren ja auf dieser Dokument-Ansicht-Architektur. Alle wesentlichen Daten sollen ja imho in der Document-Class verarbeitet werden. Die View-Class greift auf die Daten über GetDocument() zu. Um dann auf Variablen der Doc-Cl. zuzugreifen, müssen die jedesmal public sein, oder ich bastele mir Memberfunktionen, die die Variablen zurückgeben, aber das ist mit der Kirche ums Dorf und auch nicht schöner. Sollte ich stattdessen eine dritte Klasse machen, die die Variablen verwaltet, auf die die Doc & View-Class zugreifen müssen? So eine Art Singleton?

[Gast]

Wäre schön, wenn die Welt torusförmig wäre - also die Zellen auf der anderen Seite wieder rauskommen.

Hab auch mal so'n 'Game of Life' nachprogrammiert, aber eher um mich mit der WinAPI auseinanderzusetzen
Vielleicht findest du einige nützliche Anregungen: http://youthenergy.s01.user-portal.com/Life.zip

[Gregor]

ethereal schrieb:

@Gregor: Den oberen Quellcode habe ich etwas verkürzt:

int LifeMap::GetNumberOfNeighbours(int PosX,int PosY)
{		
int count=0;			
	int xright=PosX+1;
	int	xleft=PosX-1; 
	int ydown=PosY+1;
	int	yup=PosY-1;
	
	if(!(xleft<0)&&(Array[xleft][PosY].exist))
		count++;
	if(!(xleft<0)&&!(yup<0)&&(Array[xleft][yup].exist))
		count++;
	if(!(xleft<0)&&!(ydown>Height)&&(Array[xleft][ydown].exist))
		count++;
	if(!(ydown>Height)&&(Array[PosX][ydown].exist))
		count++;
	if(!(yup<0)&&(Array[PosX][yup].exist))
		count++;
	if(!(xright>=Width)&&(Array[xright][PosY].exist))
		count++;
	if(!(xright>=Width)&&!(yup<0)&&(Array[xright][yup].exist))
		count++;
	if(!(xright>=Width)&&!(ydown>Height)&&(Array[xright][ydown].exist))
		count++;
return count;
}

Aber war es das, was du meintest???

Schon besser. Ich hätte das allerdings eher so gemacht:

int LifeMap::GetNumberOfNeighbours(int PosX,int PosY)
{
   int count = 0;
   for (int x = PosX-1 ; x <= PosX+1 ; ++x)
   {
      for (int y = PoxY-1 ; y <= PosY+1 ; ++y)
      {
         if(exists(x,y)) ++count;
      }
   }
   if (exists(PosX,PosY)) --count;
   return count;
}

bool LifeMap::exists(int PosX,int PosY)
{
   if (PosX < 0) return false;
   if (PosY < 0) return false;
   if (PosX >= Width) return false;
   if (PosY >= Height) return false;
   return Array[PosX][PosY].exist;
}

(keine Ahnung, ob die Syntax hier halbwegs in Ordnung ist. Ich programmiere so selten in C++.)

...naja, eigentlich hätte ich es vermutlich ganz anders gemacht: Weil ich gesehen hätte, dass das, was man hier benötigt, mathematisch gesehen eine Faltung ist, wäre ich mit einer deutlich abstrakteren Sichtweise an die Sache herangegangen. Aber das ist letztendlich Overkill.

Bei der 2. Stelle hast du zwei strukturell vollkommen gleiche Stücke. Da lohnt es sich, so ein Stück in eine Methode mit entsprechenden Parametern auszulagern und die beiden Codestücke durch entsprechende Methodenaufrufe zu ersetzen.

[ethereal]

Das stimmt schon, doch da beide Arrays jedesmal zusammen gelöscht werden und auch keine anderen Arrays existieren, für die gleicher Code gelten müsste, muß das doch nicht sein, oder? Warum sollte ich stattdessen jedesmal schreiben:

SafeDelete(Array);
     SafeDelete(CmpArray);

statt nur einmal

SafeDelete();

Diese mathematische "Faltung" sagt mir leider (noch) nichts, da ich gerade mal Abi gemacht habe und da die Mathematik gegen 0 konvergiert(abgesehen von den ganzen Rechnereien) Wahrscheinlich werd' ich in einem Jahr allerdings ein Informatik-Studium beginnen, dann kannst du mir den ja Tip nochmal geben

[Gregor]

BTW: Du hast in deinem letzten Codestück ein ">= Width", aber auch ein "> Height" stehen. Sieht aus meiner Sicht nach nem Fehler aus. Vermutlich muss es ">= Height" heißen. Das kommt gleich mehrmals vor (Copy&Paste, heh?!).

[Gregor]

ethereal schrieb:

Das stimmt schon, doch da beide Arrays jedesmal zusammen gelöscht werden und auch keine anderen Arrays existieren, für die gleicher Code gelten müsste, muß das doch nicht sein, oder? Warum sollte ich stattdessen jedesmal schreiben:

SafeDelete(Array);
     SafeDelete(CmpArray);

statt nur einmal

SafeDelete();

Ich meinte, dass du innerhalb der SafeDelete()-Methode SafeDelete(Array) und SafeDelete(CmpArray); aufrufst. Dann bleibst du bei einem Methodenaufruf für die ganze Löschung.

[ethereal]

SafeDelete also überladen?
Ok, das geht natürlich so...hmm, ja kann ích machen.
Das andere war kein Copy&Paste (Wirklich nicht )Ich hatte nur zuerst überall > und da kamen Fehlermeldungen (logisch). Also habe ich bei Width >= hingepackt, merkwürdigerweise ging's dann auch.
Dankeschön für den Hinweis.
Gruß
E-the-Real