Variablen fast identische Setter

[thomasch]

Hallo,
ich habe eine Klasse(Bean), in der ich verschiedene Variablen setzen muss. Diese Variablen müssen alle auf den selben Wertebereich getestet werden.

Momentan sieht das vereinfacht so aus:

public class MyClass{
    private boolean success=true;
    private final int FOO = 123;
    private int fdo;
    private int fbu;
    // es folgen diverse weitere private int...

   /**
    * testet den Wertebereich eines Farbwertes
    * @param col
    * @return true, wenn Wertebereich passt, sonst false
    */
   private boolean checkColorVal(int col){
      if(
           (col<0)||
           (col>FOO)
        ){
         return false;
      }else{
         return true;
      }
   }

    /**
    * Setter für fbu
    * @param fbu 
    */
  public void setFbu(int fbu) {
      if(this.checkColorVal(fbu)){
         this.fbu = fbu;
      }else{
         this.success=false;
      }
   }

   /**
    * Setter für fdo
    * @param fdo 
    */
   public void setFdo(int fdo) {
      if(this.checkColorVal(fdo)){
         this.fdo = fdo;
      }else{
         this.success=false;
      }
   }
}

Ich hab mir jetzt gedacht, dass man die Setter eigentlich zusammenfassen könnte. Mir schwebt etwas in der Art wie

...

   private void setCol(int colVar,int colVal){
      if(this.checkColorVal(colVal)){
         colVar = colVal;
      }else{
         this.success=false;
      }
   }

und

   /**
    * Setter für fbu
    * @param fbu 
    */
   public void setFbu(int fbu) {
      this.setCol(this.fbu, fbu);
   }

vor. Dazu müsste ich die Variablen by reference übergeben - wobei die leider primitive sind und ich annehme, dass mit hier die Verwendung von Integer statt int nicht weiter helfen dürfte...

Habt Ihr eine Idee?

Vielen Dank

Thomasch

 

[JohannisderKaeufer]

Was hat es mit dem boolean success auf sich?

Eine Möglichkeit ist das du keine int Werte in deiner Bean verwendest, sondern ein Objekt und statt dem success Exceptions wirfst.

public class ColorValue{
private int value;
public void setValue(int value) throws ValueOutOfRangeException {
  if(value>0 && value < F00){
     this.value = value;
  }else{
    throw new ValueOutOfRangeException();
  }
}
public getValue(){return this.value;}
}

 

[TheDarkRose]

Was ist so schlimm daran, dass du zwei Setter hast?

 

[bygones]

solange nach aussen hin du die noetigen setters getrennt haelst find ichs ok. Intern die private anzubieten, die dir duplicated code erspart ist an sich eine gute idee.

Nur

private void setCol(int colVar,int colVal){
      if(this.checkColorVal(colVal)){
         colVar = colVal;
      }else{
         this.success=false;
      }
   }

ist nonsense... warum brauchst du colVar 2x ? [c]colVar=colVar[/c] bringt gar nix - aber das hast du schon selbst gesehen....

nein Integer wuerde dir auch nix bringen. Auch dass deine Methode unterschiedliche Wege geht (mal wird das int gesetzt, mal ein boolean) ist nicht hilfreich.

somit seh ich erstmal keine moegliche verbesserung hier (jdf nicht eine die sinnvoll ist)

 

[thomasch]

Was ist so schlimm daran, dass du zwei Setter hast?

Dass ich nicht 2 sondern eigentlich 6 oder 7 habe und jedesmal fast den selbn Code ausführe...

 

[tfa]

Dass ich nicht 2 sondern eigentlich 6 oder 7 habe und jedesmal fast den selbn Code ausführe...

Das ist nichts schlimmes. Du kannst deine IDE die Getter und Setter automatisch erzeugen lassen, ganz wie du willst (Code-Templates). Unschön finde ich hier, dass einfach nur ein Flag gesetzt wird, wenn die Parameterprüfung fehl schlägt. Wie wär's mit IllegalArgumentException?

 

[bygones]

Dass ich nicht 2 sondern eigentlich 6 oder 7 habe und jedesmal fast den selbn Code ausführe...

solche "optimierungen" sind der tod fuer deine API. wie gesagt - was du intern fuer dich machst ist deine sache.

Wenn du wirklich eine Klasse hast die 6 oder 7 setter braucht, so braucht sie das. Ueberleg eher ob du alle setter brauchst oder ob deine Struktur richtig ist. Ein solches Monster in einen Schafspelz von nur 2 Methoden zu verstecken ist nur eine verschleierung und keine loesung

 

[thomasch]

Was hat es mit dem boolean success auf sich?

Eine Möglichkeit ist das du keine int Werte in deiner Bean verwendest, sondern ein Objekt und statt dem success Exceptions wirfst.

public class ColorValue{
private int value;
public void setValue(int value) throws ValueOutOfRangeException {
  if(value>0 && value < F00){
     this.value = value;
  }else{
    throw new ValueOutOfRangeException();
  }
}
public getValue(){return this.value;}
}

Die Klasse soll letztendlich alle übergebenen Variablen auf ihre Wertebereiche testen. Wenn auch nur ein Test fehlschlägt kann ich eh nicht weiter arbeiten und muss darauf reagieren.

Unter anderem werde da noch Koordinatenbereiche und Stringwerte getestet.

Ich hab mir jetzt noch eine Möglichkeit überlegt, die ich aber auch blos nicht wirklich umsetzen möchte:

   private void setColDoof(int colVal,String name){
      //int colVar;
      
      if(this.checkColorVal(colVal)){
         if(name.equals("fdo")){
            this.fdo=colVal;
         }else if (name.equals("fli")){
            this.fli=colVal;
         }
         // usw.
      }else{
         this.success=false;
      }
   }

 

[bygones]

Ich hab mir jetzt noch eine Möglichkeit überlegt, die ich aber auch blos nicht wirklich umsetzen möchte:

richtig... bloss nicht

 

[thomasch]

Wenn du wirklich eine Klasse hast die 6 oder 7 setter braucht, so braucht sie das.

Die Setter an sich brauch ich. Nur machen die alle fast das selbe...

 

[thomasch]

Wie wär's mit IllegalArgumentException?

Die muss ich spätestens in der davor liegenden .jsp abfangen.
Ich dachte, da wäre ein

(if !myBean.success){gehWeg()}

die bessere Wahl...

 

[JohannisderKaeufer]

Die Klasse soll letztendlich alle übergebenen Variablen auf ihre Wertebereiche testen. Wenn auch nur ein Test fehlschlägt kann ich eh nicht weiter arbeiten und muss darauf reagieren.

Dann ist das mit dem success auch Kokolores. Wenn ein Setter nicht aufgerufen wird hast du dort einen Null-Wert und trotzdem noch true.

Was mir zu dem Thema einfällt ist das Builder Pattern, das Joshua Bloch beschreibt, wobei dann beim aufruf von build alles nochmal überprüft wird.

 

[bERt0r]

Eine Möglichkeit wäre, deine Variablen in einer HashMap zu speichern, sind schließlich alles Farben oder? Dann machst du ein mapping HashMap<String,Color> wobei der String der Variablenname und Color die Variable ist. Der Zugriff auf eine Hashmap ist ziemlich performant, ein Konstrukt wie du oben beschrieben hast bremst bei mehreren Variablen (und damit if abfragen) ziemlich aus und ist extrem ineffizient.

if(this.checkColorVal(colVal)){
if(name.equals("fdo")){
this.fdo=colVal;
}else if (name.equals("fli")){
this.fli=colVal;
}

edit: Ich hab grade gesehen dass du ja ein Bean schreibst, bei Beans musst du natürlich für jede Variable eine get/set Methode schreiben, sonst ists kein Bean

 

[thomasch]

Hallo,
ich hab das jetzt gelöst, in dem die Testroutine einfach einen int zurück gibt.

private int setCol(int colVar,int colVal){
      int ret=-1;
      if(this.checkColorVal(colVal)){
         ret = colVal;
      }else{
         this.success=false;
      }
      return ret;
   }

 

[nillehammer]

Wenn Du eine Validierung ausbauen willst, nutze das Bean Validation Framework.

// Uups hab grad gesehen, dass der von Dir gepostete Code ja garnicht der public setter ist. Deswegen hab ich meinen Beitrag mal etwas verkürzt