Code vereinfachen möglich?

[Fatal Error]

Hallo!

Ich hab mal wieder eine Frage...
Und zwar geht es darum den code einfach besser zu schreiben, ist zwar nicht exakt ausgedrückt aber ihr werdet schon wissen was ich meine:

hier einmal ein code stückerl:

if ((getY() + 1) < 8) list.add(new Point(getX(), getY() + 1));
if ((getX() + 1) < 8) list.add(new Point(getX() + 1, getY()));
if ((getY() - 1) >= 0) list.add(new Point(getX(), getY() - 1));
if ((getX() - 1) >= 0) list.add(new Point(getX() - 1, getY()));
if ((getY() + 1) < 8 && (getX() + 1) < 8) list.add(new Point(getX() + 1, getY() + 1));
if ((getY() - 1) >= 0 && (getX() + 1) < 8) list.add(new Point(getX() + 1, getY() - 1));
if ((getY() + 1) < 8 && (getX() - 1) >= 0) list.add(new Point(getX() - 1, getY() + 1));
if ((getY() - 1) >= 0 && (getX() - 1) >= 0) list.add(new Point(getX() - 1, getY() - 1));

Und jetzt meine Frage: Wie kann man soetwas einfacher oder leichter gestalten?
Irgendwo hab ich ma gehört das Math.abs ziemlich brauchbar ist, nur weiß ich nicht wie ich es da einsetzen könnte?
Wenn man das nicht vereinfachen kann is auch ok, aber ich bin mir ziemlich sicher das es geht.
Schonmal danke für eure Vorschläge/Lösungen

 

[PollerJava]

State- Entwurfsmuster würd ich mal sagen, hilft immer bei zu vielen if- Abfragen, für jede if- Abfrage ein eigenes Objekt,
googeln,

 

[SlaterB]

alle Ausdrücke
list.add(new Point(getX(), getY() + 1));
kannst du durch eine Hilfsoperation ersetzen:
newP(list, 0,1);
welche sich x und y holt und 0 bzw. 1 addiert (x und y)
wenn list auch irgendwie durch getList() erreichbar ist, dann kann dieser Parameter auch wegfallen

-----------

falls du damit leben kannst, kannst du alle Ausdrücke
(getY() + 1) < 8 durch getY() < 7 usw. ersetzen

oder du definierst dir vorher Variablen:
int y = getY();
int yM1 = y-1;
int yP1 = y+1;
usw.

--------

letztlich hast du aber 8 verschiedene Anwendungsfälle mit 8 unterschiedlichen Punkten,
da sind 8 ifs nicht verkehrt

bisschen Schachtelung wäre noch denkbar:

if ((getY() + 1) < 8) { 
   list.add(new Point(getX(), getY() + 1)); 
   if ((getX() + 1) < 8) { 
     ...
   }
   if (...) { 
        ...
   }
} else {

}

 

[SlaterB]

edit: doch nicht so sinnreich

 

[kama]

Hallo,

if ((getY() - 1) >= 0) {
	list.add(new Point(getX(), getY() - 1));
	if ((getX() + 1) < 8) {
		list.add(new Point(getX() + 1, getY() - 1));
	}
	if ((getX() - 1) >= 0) {
		list.add(new Point(getX() - 1, getY() - 1));
	}
}

if ((getY() + 1) < 8) {
	list.add(new Point(getX(), getY() + 1));
	if ((getX() + 1) < 8) {
		list.add(new Point(getX() + 1, getY() + 1));
	}
	if ((getX() - 1) >= 0) {
		list.add(new Point(getX() - 1, getY() + 1));
	}
}
if ((getX() + 1) < 8) {
	list.add(new Point(getX() + 1, getY()));
}
if ((getX() - 1) >= 0) {
	list.add(new Point(getX() - 1, getY()));
}

Wie wäre es damit...

MfG
Karl Heinz Marbaise

 

[maki]

Als erstes würde ich die ständigen (und langen) getX() und getY() Methodenaufrufe nur einmal durchführen:

int x = getX();
int y = getY();

dann würde ich mich fragen, warum x + 1 < 8 nicht als x < 7 geschrieben werden kann.

als nächstes würde ich mich fragen, warum man gleiche Bedinungen nicht zusammenschreibt

if (y < 7) 
    list.add(new Point(x, y + 1));
...
if (y < 7 && x < 7) 
    list.add(new Point(x + 1, y + 1));

das sollte besser so dargestellt werden:

if (y < 7)  {
    list.add(new Point(x, y + 1));
    
    if(x < 7) {
        list.add(new Point(x + 1, y + 1));
    }
}

Für den Rest des Codes dann analog anwenden, natürlich angenommen dass die Logik stimmt.

 

[Ark]

@Fatal Error: Beschreib doch einmal, was genau dieser Code bezwecken soll. Für mich sieht es so aus, als versuchtest du irgendwie ein 8x8 großes Feld zu füllen, indem du dir die betroffenen Punkte in einer Liste speicherst. Mich würde auch mal interessieren, was der GC zu deinem Vorhaben sagt.

Ark

 

[Guest]

Variante 1 (nicht optimal, aber übersichtlich)

if (getX() < 7) {
   list.add(new Point(getX() + 1, getY()));
   if (getY() >= 1) {
      list.add(new Point(getX() + 1, getY() - 1));
   }
}

if (getX() >= 1) {
   list.add(new Point(getX() - 1, getY()));
   if (getY() < 7) {
      list.add(new Point(getX() - 1, getY() + 1));
   }
}

if (getY() < 7) {
   list.add(new Point(getX(), getY() + 1));
   if (getX() < 7 ) {
      list.add(new Point(getX() + 1, getY() + 1));
   }
}

if (getY() >= 1) {
   list.add(new Point(getX(), getY() - 1));
   if (getX() >= 1) {
      list.add(new Point(getX() - 1, getY() - 1));
   }
}

Variante 2 (negative Logik)

Point p[] = {
   new Point(getX() + 1, getY())
  ,new Point(getX() + 1, getY() - 1)
  ,new Point(getX() - 1, getY())
  ,new Point(getX() - 1, getY() + 1)
  ,new Point(getX(), getY() + 1)
  ,new Point(getX() + 1, getY() + 1)
  ,new Point(getX(), getY() - 1)
  ,new Point(getX() - 1, getY() - 1)
}

if(getX() >= 7) {
   p[0] = p[1] = p[5] = null;
}

if(getX() < 1) {
   p[2] = p[3] = p[7] = null;
}

if(getY() >= 7) {
   p[3] = p[4] = p[5] = null;
}

if(getY() < 1) {
   p[1] = p[6] = p[7] = null;
}

for(int i=0; i<p.length; i++) {
   if(p[i] != null) {
      list.add(p[i]);
   }
}

Variante 3 (basiert auf Variante 1; weniger übersichtlich)

interface Op {
   boolean eval(int n);
}

public void addPoints(List<Point> list, Op o1, Op o2, int n1, int n2, int x1, int y1, int x2, int y2) {
   if ( o1.eval(n1) ) {
      list.add(new Point(x1, y1));
      if ( o2.eval(n2) ) {
         list.add(new Point(x2, y2));
      }
   }
}

Op o1 = new Op() {
   public boolean eval(int n) {
      return n < 7;
   }
);
Op o2 = new Op() {
   public boolean eval(int n) {
      return n >= 1;
   }
);

addPoints(list, o1, o2, getX(), getY(), getX()+1, getY(), getX()+1, getY()-1);
addPoints(list, o2, o1, getX(), getY(), getX()-1, getY(), getX()-1, getY()+1);
addPoints(list, o1, o1, getY(), getX(), getX(), getY()+1, getX()+1, getY()+1);
addPoints(list, o2, o2, getY(), getX(), getX(), getY()-1, getX()-1, getY()-1);

 

[Fatal Error]

Danke gleich mal für die Antworten!

ich bin schon ein wenig blöd weil ich (x + 1) < 8 statt x < 7 hingeschrieben habe Oo...
hab das ganze jetzt mal so verändert:

int x = getX();
int y = getY();
		
if (x < 7) {
	addToList(list, x + 1, y);
	if (y >= 1) addToList(list, x + 1, y - 1);
	if (y < 7) addToList(list, x + 1, y + 1);
}
if (y < 7) {
	addToList(list, x, y + 1);
	if (x >= 1) addToList(list, x - 1, y + 1);
}
if (x >= 1) {
	addToList(list, x - 1, y);
	if (y >= 1) addToList(list, x - 1, y - 1);
}
if (y >= 1) addToList(list, x, y - 1);

wobei addToList(...) nur einen neuen Punkt in die Liste einfügt.

@Ark:

also...ja hier geht es darum die Felder zu bekommen, auf die sich der König beim Schach bewegen darf.
8*8 Feld ist also jetzt klar.
Nach dem obigen Code werden noch alle Felder, die von einer Figur gedeckt werden entfernt.
Das funktioniert auch super, jede Figur hat eine Methode die die Felder zurückgibt auf die sie ziehen darf. Von der Performence her merkt man eigentlich nichts, also das es lang dauern würde oder ähnliches.
Was der Garbage Collector da aber alles zu leisten hat hab ich mich nicht angesehen

@Gast hab deinen Beitrag erst nach dem schreiben von meinem gesehen, mal anschaun

 

[Guest]

Noch eine Variante

enum Op {
  X1,
  X2,
  Y1,
  Y2;

  public boolean eval(int x, int y) {
     if(X1 == this) {
       return x < 7;
     }
     else if (X2 == this) {
       return x >= 1;
     }
     else if(Y1 == this) {
       return y < 7;
     }
     return y >= 1;
  }
}

public void addPoints(List<Point> list, int x, int y, Op o1, Op o2, int dx1, int dy1, int dx2, int dy2) {
   if ( o1.eval(x, y) ) {
      list.add(new Point(x+dx1, y+dy1));
      if ( o2.eval(x, y) ) {
         list.add(new Point(x+dx2, y+dy2));
      }
   }
}

int x=getX(), y=getY();
addPoints(list, x, y, Op.X1, Op.Y2,  1,  0,  1, -1);
addPoints(list, x, y, Op.X2, Op.Y1, -1,  0, -1,  1);
addPoints(list, x, y, Op.Y1, Op.X1,  0,  1,  1,  1);
addPoints(list, x, y, Op.Y2, Op.X2,  0, -1, -1, -1);

OK, ich glaube das reicht. Deine ursprüngliche Version ist eigentlich OK. :wink:

 

[Ark]

also...ja hier geht es darum die Felder zu bekommen, auf die sich der König beim Schach bewegen darf.
8*8 Feld ist also jetzt klar.
Nach dem obigen Code werden noch alle Felder, die von einer Figur gedeckt werden entfernt.
Das funktioniert auch super, jede Figur hat eine Methode die die Felder zurückgibt auf die sie ziehen darf.

Vielleicht ist es günstiger, jede Figur selbst zu fragen, ob sie ein bestimmtes Feld(x,y) erreichen könnte:

public boolean controlsField(int x, int y);

Die Zahl der möglichen Felder ist nämlich für jede Figur verschieden begrenzt. Springer und König können z.B. maximal 8 Felder erreichen. Die Dame hat natürlich wesentlich mehr Spielraum. Jede Figur hat ihre eigenen Gesetzmäßigkeiten. Ein Turm(xT,yT) kann z.B. das Feld(x0,y0) niemals erreichen, wenn x0 != xT && y0 != yT gilt. Wenn aber das Feld(x0,y0) erreicht werden könnte, soll der Turm intern eine Liste auffrischen, die das betroffene Feld enthalten könnte (und dementsprechend mitteilen, ob nun das Feld(x0,y0) erreicht wird oder nicht). Bei der nächsten Anfrage braucht dann nur noch die Liste durchgegangen werden. Der Vorteil dabei ist, dass die maximale Größe der Listen statisch von der Figur abhängt. (Ein Turm kann bspw. maximal 14 Felder erreichen.)

Wenn es um Geschwindigkeit geht, kann man noch etwas rausholen, indem der Turm 4 verschiedene Listen mit maximal 7 Feldern speichert; für jede Richtung eine. So müssen nicht alle möglichen Felder, sondern nur die in Richtung des Feldes(x0,y0) untersucht werden. Den Garbage Collector sollte das auch freuen, denn durch die statische Größe und die interne Verwaltung der Listen als int-Arrays taucht während dieser Verarbeitung nicht ein einziges new auf.

Dies sei eine kleine Anregung. Vielleicht lässt sich das eine oder andere gewinnbringend einsetzen.

Ark

EDIT: Es geht sogar noch schneller: Die meisten Figuren können ja in eine Richtung maximal nur bis zu einer fremden Figur oder bis gerade vor eine eigene Figur ziehen. Der Aufwand sinkt so von O(7) auf O(1) und die internen Arrays sind gar nicht mehr nötig! :shock: Zumindest wenn controlsField(int,int) das zweite Mal für die gleiche Richtung aufgerufen wird, sonst bleibt es bei O(7). Die Arrays mit den Koordinaten sind aber trotzdem nicht nötig!

Ark

EDIT: Eine ganz andere Idee ist es, boolean[][] whiteControls und boolean[][] blackControls anzulegen und darin zu speichern, ob überhaupt irgendeine Figur einer Partei ein bestimmtes Feld kontrolliert. Allerdings ist dann nicht mehr nachvollziehbar, welche Figuren ein Feld kontrollieren. Vielleicht lassen sich aber all diese Techniken sinnvoll miteinander kombinieren, je nach Aufgabenstellung.

Ark

 

[Fatal Error]

hmm...

derzeit isses bei mir das ich eine abstracte Klasse Figure hab, welche ihre Position und ihre Farbe speichert. Weiters hat sie 2 abtracte methoden: getMoveableFields(ChessBoard b) und getAttackableFields(ChessBoard b). (nicht über die namen aufregen ).

die getMoveableFields methode gibt in einer ArrayList<Point> alle Felder zurück, auf die die Figur fahren darf.
die getAttackableFields methode gibt auch in so einer Liste alle Felder zurück, die von der Figur attackiert werden können bzw. alle Felder die die Figur, wenn keine anderen Figuren auf dem Brett wären, erreichen würde. Das brauch ich wenn ich überprüfe ob zb. der König im Schach steht wenn ich mit einer Figur, die sich "vor ihn" hingestellt hat, wegfahre...
diese methoden sind bei der dame zb elendslang...

ob ein bestimmtes Feld dann drinnen ist in den jeweiligen listen kann ich ja leicht mit contains(Point p) herausfinden.
wäre das selbe wie deine angesrochene controlsField methode, nur das ich die liste dann immer neu erzeuge und somit wirklich viel Rechenleistung verschwendet wird. Das versuch ich jetzt auch zu verbessern indem ich mir die Listen intern speicher und nur wenn sich was bewegt hat aktualisiere wie du gesagt hast.
Ja...es wäre noch besser beim Turm 4 listen zu speichern...aber da hab ich jetzt keine ahnung wie ich das dann umschreiben müsste...ist nämlich schon ziemlich kompliziert^^...

edit: dein edit verstehe ich jetzt nicht ganz Oo sry. wie meinst du das genau das die internen arrays nicht mehr nötig sind?


noch ein edit:

hier ist zum anschaun mal die getMoveableFields() Methode vom Läufer:
und hier kann ich mich durch eine ausrede gekonnt aus der afaire ziehen hat ein freund programmiert
dürfts also schimpfen

jede for-schleife ist für eine der 4 Richtungen. in dieser liste stehen dann also alle felder auf die sich die figur bewegen kann, auch wenn eine figur auf einem dieser felder oben steht. die felder hinter dieser figur allerdings nicht mehr.

public ArrayList<Point> getMoveableFields(ChessBoard board) {
		ArrayList<Point> list = new ArrayList<Point>();
		
		int x = getX();
		int y = getY();
		
		for (int cnt = 1, cnt2 = 1; cnt < 8 && cnt2 < 8; cnt++, cnt2++) {
			if(((x + cnt) < 8) && (y + cnt2) < 8) {
				if(board.getField(x + cnt, y + cnt2).getFigure() == null){
					addToList(list, x + cnt, y + cnt2);
				} else {
					addToList(list, x + cnt, y + cnt2);
					break;
				}
			}
		}
		for (int cnt = 1, cnt2 = 1; cnt < 8 && cnt2 < 8; cnt++, cnt2++) {
			if(((x - cnt) >= 0) && (y + cnt2) < 8) {
				if(board.getField(x - cnt, y + cnt2).getFigure() == null){
					addToList(list, x - cnt, y + cnt2);
				} else {
					addToList(list, x - cnt, y + cnt2);
					break;
				}
			}
		}
		for (int cnt = 1, cnt2 = 1; cnt < 8 && cnt2 < 8; cnt++, cnt2++) {
			if(((x + cnt) < 8) && (y - cnt2) >= 0) {
				if(board.getField(x + cnt, y - cnt2).getFigure() == null){
					addToList(list, x + cnt, y - cnt2);
				} else {
					addToList(list, x + cnt, y - cnt2);
					break;
				}
			}
		}
		for (int cnt = 1, cnt2 = 1; cnt < 8 && cnt2 < 8; cnt++, cnt2++) {
			if(((x - cnt) >= 0) && (y - cnt2) >= 0) {
				if(board.getField(x - cnt, y - cnt2).getFigure() == null){
					addToList(list, x - cnt, y - cnt2);
				} else {
					addToList(list, x - cnt, y - cnt2);
					break;
				}
			}
		}
		return list;
	}

 

[Ark]

hmm...
edit: dein edit verstehe ich jetzt nicht ganz Oo sry. wie meinst du das genau das die internen arrays nicht mehr nötig sind?

Nehmen wir als Beispiel die Dame: Sie hat 8 Aktionsradien, die alle verschieden groß sein können. Ich benenne sie mal nach den Himmelsrichtungen:

public final class Queen extends Piece {// Name beachten ;)
    private int radN, radNE, radE, radSE, radS, radSW, radW, radNW;// Aktionsradien
    private int x,y;// eigene Position

    public boolean controlsField(int x,int y){
        // entsprechende Implementierung
    }
}

Alle Radien sollten zu Beginn auf -1 stehen, wenn sie on the fly ermittelt werden sollen (siehe unten).

Wenn nun controlsField(int,int) aufgerufen wird, muss erst einmal kontrolliert werden, ob die Dame aufgrund der Regeln überhaupt theoretisch dazu in der Lage ist, dieses Feld zu kontrollieren (also prüfen, ob das Feld auf einer der Diagonalen oder auf der Waagerechten oder auf der Senkrechten zur Dame liegt). Eventuell lässt sich das mit dem nächsten Schritt kombinieren: In welcher Richtung liegt das Feld von der Dame aus gesehen (N, E, S, W, NE usw.)? Nehmen wir an, es liegt genau nördlich von der Dame, könnte also potentiell erreicht werden. Jetzt muss überprüft werden, ob radN negativ ist. Wenn ja, wurde nämlich noch gar nicht untersucht, wie weit die Dame momentan nach Norden ziehen könnte. Das sollte jetzt nachgeholt werden. (Dieser Schritt könnte auch erledigt werden, wenn eine Figur gezogen wurde. Dann muss die entsprechende Methode aber bei allen Figuren aufgerufen werden.) Wenn wir den Aktionsradius nach Norden ermittelt haben, hat radN einen entsprechenden Wert, z.B. 4. Nun muss nur noch überprüft werden, ob this.y-radN<=y gilt (muss entsprechend angepasst werden, je nachdem, wo der Ursprung A1 liegt).

Ark

 

[Gast]

@Fatal Error

Das Schachbrett geht doch von 0-7(X- bzw. Y- Richtung).
Damit kannst du doch eine Methode schreiben die überprüft ob ein Punkt existiert oder nicht. (pointIsOk). Die Methode kann ja für verschiedene Figuren verwendet werden.
Der Rest sehe dann so aus.

for(i=-1; i<2;i++){
  for(j=-1;j<2;j++){
    if(!(i==0&&j==0)){
      Point point = new Point(getX()+i;getY()+j);
      if(pointIsOk(point){
        list.add(point);
      }
    }
  }
}

boolean pointIsOk(Point point){
  boolean ok = true;
  if(point.getX()<0 || point.getX>7 || point.getY()<0 || point.getY>7){
  ok=false;
  }
  return ok;
}

 

[Gast]

Mit der Methode pointIsOk() kann man einen läufer so implementieren.

laeuferPositionen(-1,-1);
laeuferPositionen(1,-1);
laeuferPositionen(-1,1);
laeuferPositionen(1,1);

public void laeuferPositionen(int signX, int signY){
int i = 1;
do
{
Point point = new Point(getX()+(i*signX), getY()+(i*signY));
if(pointIsOk(point)){
  list.add(point);
}
}while(pointIsOk(point))
}

 

[Guest]

Mit der Methode pointIsOk() kann man einen läufer so implementieren.

laeuferPositionen(-1,-1);
laeuferPositionen(1,-1);
laeuferPositionen(-1,1);
laeuferPositionen(1,1);

public void laeuferPositionen(int signX, int signY){
int i = 1;
do
{
Point point = new Point(getX()+(i*signX), getY()+(i*signY));
if(pointIsOk(point)){
  list.add(point);
}
}while(pointIsOk(point))
}

ups Zeile 14 müßte noch ein i++; enthalten

 

[Guest]

bzw so

laeuferPositionen(-1,-1);
laeuferPositionen(1,-1);
laeuferPositionen(-1,1);
laeuferPositionen(1,1);

public void laeuferPositionen(int signX, int signY){
int i = 1;
do
{
Point point = new Point(getX()+(i*signX), getY()+(i*signY));
if(pointIsOk(point)){
  list.add(point);
  i++;
}else{
  break;
}
}while(true)
}

 

[Marco13]

Hm. Wenn das ein Schachprogramm werden soll, in dem später auch der Computer spielt, wirst du mit den ganzen if's und dem "new Point" keine Freude haben. Wenn es darum geht, das Spiel für zwei menschliche Spieler zu implementieren, dann mach es so, wie du es am übersichtlichsten findest. Wenn es für einen Computerspieler sein soll, dann kannst du das Spielfeld z.B. als 12x12 int-array speichern, wo der Rand einfach mit "-1" belegt ist (d.h. dort kann man nicht hin) - dann erübrigen sich die if-Abfragen nämlich von selbst....

 

[Ullenboom]

Also ich finde den ursprünglichen Programmcode gut lesbar -- die OO-Lösungen sind für mich weniger gut lesbar. Ich würde es so lassen.

Christian