Refactoring von mehreren identischen Klassen

[guguli]

Hallo zusammen,

Ich brauche eure Hilfe um meinen Codd zu refactorn
Also ich habe 3 Packages 2009, 2010 und 2011
die haben jeweils eine service class und eine factory class.
Also Service2009, Service2010 und Service2011 und je eine Factory class, die einfach Factory heißt.

Die factory klassen haben eine getInstance method und eine SetInstance.

Beispielsweise für 2011 sieht die Factory class so aus:

public final class Factory {

   private static EService<K2011DTO> instance;

   private Factory() {}
   
   public static void setInstance(final EService<K2011DTO> instance) {
       Factory.instance = instance;
   }

   public static EService<K2011DTO> getInstance() {
       if (instance != null) {
           return instance;
       } else {
           return new Service2011();
       }
   }
}

für weitere Jahren ändert sich einfach der generische Type K2011DTO und die Service2011 class entsprechend des Packages.

Desweiteren gibt es 3 andere Packages jahr2009, jahr2010 und jahr2011
Die haben je eine Gebe und eine Hole klasses.
Diese klassen benutzen die getInstance methode der factory classes.
Beispielweise für Gebe classs 2011 sieht das so aus:

public final class Gebe2011 implements Processor {

   @Override
   public void process(final MainDTO request, final MainDTO response, final BearbeiterContext bearbeiterContext,
           final FachContext fachContext) {

       EService<K2011DTO> service = Factory.getInstance();
       K2011DTO kDTO = DatenProcessor.REQUEST_HELPER.gebeRequestToDto(request,
               K2011DTO.class);
       service.gebe(fachContext, kDTO, request.getNachrichten());

   }
}

Die EService ist eine Interface, welche so aussieht:

public interface EService<DTO> extends KService<DTO> {
}

nun möchte ich die Factory class so refactorn, dass nur einmal für alle Jahren existiert und nicht für jedes Jahr einmal.

Ich hoffe ich habe mich verständlich ausgedrückt.

 

[mihe7]

Du könntest z. B. den Service unter einem Namen bei der Factory registrieren und dort die Services in einer Map speichern.

 

[guguli]

Du könntest z. B. den Service unter einem Namen bei der Factory registrieren und dort die Services in einer Map speichern.

Kannst du das bitte mit einem Beispielcode genauer erläutern?
Danke

 

[mihe7]

Im Moment schlecht, bin unterwegs. An irgendeiner Stelle registrierst Du aktuell ja die Instanz Deines Services. Dort änderst Du den Aufruf z.B. zu

Factory.setInstance("2011", service); // wobei service == this sein könnte

Und in der Factory hast Du halt eine Map<String, EService> o.ä.

 

[guguli]

Ich habe nachgeschaut, die setInstance Methode wird gar nicht benutzt, sie ist nur für testfälle da. Also getInstance() schaut ob eine Instance existiert wenn nicht dann erzeut ein Objekt der entsprechenden Klasse.

Also ich dachte irgendwie mit generic und Class und untypisierter Parameter T würde das gehen.
Also der getInstance() methode einen Übergabeparameter geben.

Wisst ihr wie ich das meine?

 

[mrBrown]

Spricht denn irgendwas gegen die Lösung von @mihe7?

Den String kannst du gegen jedes andere Objekt ersetzen, wenn dir der nicht passt. Das initialisieren der Objekte kannst du im Konstruktor oder auch im statischen Initializer machen, wenns static sein muss.

 

[guguli]

Spricht denn irgendwas gegen die Lösung von @mihe7?

das Problem ist dass setInstance niergendwo aufgerufen wird. Sie wurde nur fürs Testen geschrieben. Also ich uss nur mit getInstace() arbeiten.

also hier wird getInstance aufgerufen:

EService<K2011DTO> service = Factory.getInstance();

also das muss so abgändert werden sodass zu dieser Implementierung passen wird:

public class Factory<T> {
   private EService<T> instance;
   private Factory() {}
   public <T> EService<T> getInstance(Class<T> jahr) {
       if (instance != null) {
           return (EService<T>) instance;
       } else {
           return new Service2013();
       }
   }

Also static wäre mir lieber. Also wenn es auch static gehen würde dann wäre supper.

DIe implementierung von der klasse Factory ist natürlich nicht so ganz korrekt. Muss noch auch angepasst werden

 

[mrBrown]

Das setInstance kannst im statische Initializer der Factory aufrufen, wenn es nicht direkt in getInstance möglich ist.

Wenn du die Klasse übergibst, kannst du die Instanz auch in getInstance erzeugen. Nutzt du irgendein DI-Framework?

 

[guguli]

Nutzt du irgendein DI-Framework?

Ich benutze JBoss aber hier nutzt uns das nichts. Wieso?

Wenn du die Klasse übergibst, kannst du die Instanz auch in getInstance erzeugen

und wie? Ih würde gerne das einmal also Code beispiel sehen, wenn es geht!

 

[mihe7]

Irgendwie verstehe ich den Sinn dieser Factory nicht ganz. Du übergibst eine Klasse und willst von der Factory eine Instanz haben. Injected die Factory irgendwas? Ansonsten kannst Du ja gleich new verwenden.

EDIT: ach, ich sehe gerade: Du übergibst den Typ-Parameter.

 

[mihe7]

Eine hartkodierte, statische Variante mit allen Klassen zu Demonstrationszwecken:

import java.util.*;
import java.util.function.Supplier;

public class Factory {
    public static interface Service<T> { void run(); }

    static class Service2012 implements Service<String> {
        public void run() { System.out.println("2012"); }
    }

    static class Service2013 implements Service<Integer> {
        public void run() { System.out.println(123); }
    }

    private static Map<Class, Supplier<Service>> suppliers = new HashMap<>();

    static {
        suppliers.put(String.class, () -> new Service2012());
        suppliers.put(Integer.class, () -> new Service2013());
    }

    @SuppressWarnings("unchecked")
    public static <T> Service<T> newInstance(Class<T> clazz) {
        Supplier<Service> supplier = suppliers.get(clazz);
        if (supplier != null) {
            return supplier.get();
        }
        return null;
    }

    public static void main(String[] args) {
        Service<String> service1 = Factory.newInstance(String.class);
        Service<Integer> service2 = Factory.newInstance(Integer.class);
        Service<String> service3 = Factory.newInstance(String.class);
        service1.run();
        service2.run();
        System.out.println(service1 == service3);
    }
}

In diesem Beispiel werden neue Instanzen der jeweiligen Serviceklasse erzeugt.

 

[guguli]

Danke erst mal, das hilft mir schon denke ich. Kannst du mir aber diesen Teil mal erklären?

static {
suppliers.put(String.class, () -> new Service2012());
suppliers.put(Integer.class, () -> new Service2013());
}

 

[Flown]

Was mir noch dazu einfällt, man kann das auch über die SPI (ServiceLoader API) machen.

 

[mrBrown]

Oder alternativ CDI, wenn du sowieso JavaEE nutzt.
Wenn du eh die gewünschte Klasse übergibst, kannst du mit CDI.current().select(clazz).get() eine Instanz bekommen. Scope etc dann direkt an der Klasse mit Annotationen steuern

 

[guguli]

Ich hab das wie folgt angepasst:

so rufe ich die methode getInstance auf aus der entsprechenden klassen

EService<2010DTO> kService = Factory.getInstance(2010DTO.class);

EService<2013DTO> kService = Factory.getInstance(2013DTO.class);

...
[/CODE]
und das ist meien Factory Klasse

public class Factory {

   @SuppressWarnings("rawtypes")
   private static Map<Class, Supplier<EService>> suppliers = new HashMap<>();

   private Factory() {
   }

   static {
       suppliers.put(2009DTO.class, () -> new Service2009());
       suppliers.put(2010DTO.class, () -> new Service2010());
       suppliers.put(2012DTO.class, () -> new Service2012());
       suppliers.put(2013DTO.class, () -> new Service2013());
       suppliers.put(2015DTO.class, () -> new Service2015());
       suppliers.put(2017DTO.class, () -> new Service2017());
       suppliers.put(2019DTO.class, () -> new Service2019());

   }

   @SuppressWarnings({ "unchecked", "rawtypes" })
   public static <T> EService<T> getInstance(Class<T> dto) {

       Supplier<EService> supplier = suppliers.get(dto);

       if (supplier != null) {
           return supplier.get();
       }
       return null;

   }
}

Aber eine Sache stört mich noch. Dass am ender noch

retrun null

zurückgegenwerden kann.
Wie kann ich das ambesten abfangen?

 

[mrBrown]

Aber eine Sache stört mich noch. Dass am ender noch zurückgegenwerden kann.
Wie kann ich das ambesten abfangen?

z.B. Exception werfen oder Optional nutzen.

 

[mihe7]

Kannst du mir aber diesen Teil mal erklären?

Statische Initialisierung oder meinst Du die Lambdas (also den Part mit "() ->")?

Ein Lamda ist eine anonyme Funktion. Im konkreten Fall kannst Du Dir () -> new Service2009() als Kurzschreibweise für

new Supplier<Service>() {
    @Override
    public Service get() {
        return new Service2009();
    }
};

vorstellen.

Wie kann ich das ambesten abfangen?

Neben den von @mrBrown genannten Möglichkeiten kannst Du natürlich auch einen Standardwert zurückgeben.

Ich muss nochmal darauf hinweisen, dass mit newInstance() jedesmal ein neuer Service erstellt wird. Wenn Du das nicht willst, müsstest Du die Instanzen cachen.

 

[guguli]

Ich muss nochmal darauf hinweisen, dass mit newInstance() jedesmal ein neuer Service erstellt wird. Wenn Du das nicht willst, müsstest Du die Instanzen cachen

Ich glaub das ist der Grund, aus dem static benutzt worden ist. Also wie kann ich denn die Instance catchen?

Danke

 

[mihe7]

Naja, dass Objekte erzeugt werden, ist der Grund, warum es Factory heißt

Wie Du das cached? Du baust noch etwa folgendes ein:

private static Map<Class, Service> instances = new HashMap<>();

public static <T> EService<T> cached(Class<T> dto) {
    return instances.computeIfAbsent(dto, getInstance(dto));
}

und verwendest dann die Methode (sprich: Du benennst die vorhandene getInstance-Methode in newInstance um und diese Methode in getInstance)

EDIT: musst noch casten, hab ich vergessen.

 

[guguli]

ok, hab ich gemacht. Aber ws passiert wenn die Instace schon existiert, dann müsste ich theoretisch ja die instance zurückgeben, aber hier return nur wenn nicht existiert.

@SuppressWarnings({ "unchecked" })
   public static <T> EService<T> getInstance(Class<T> dto) {
       return (EService<T>) instances.computeIfAbsent(dto, newInstance(dto));
       

   }
   
   @SuppressWarnings({ "rawtypes", "unchecked" })
   private static <T> Function<? super Class, ? extends Supplier<EService>> newInstance(Class<T> dto) {
       Supplier<EService> instance = instances.get(dto);

       if (instance != null) {
           return (Function<? super Class, ? extends Supplier<EService>>) instance.get();
       }
       return null;
   }

 

[mihe7]

Aber ws passiert wenn die Instace schon existiert, dann müsste ich theoretisch ja die instance zurückgeben, aber hier return nur wenn nicht existiert.

https://docs.oracle.com/javase/8/do...omputeIfAbsent-K-java.util.function.Function-

 

[guguli]

https://docs.oracle.com/javase/8/do...omputeIfAbsent-K-java.util.function.Function-

habe ich direkt gelesen aber da steht niergendwo was passiert wenn die Instance schon existiert. Daher meie Frage ....
Für mein Verständnis , wenn die Instance exitiert dann musste diese passend zu dem Key zurückgegeben werden.

 

[mihe7]

Ja, das steht unten bei returns: "the current (existing or computed) value associated with the specified key, or null if the computed value is null"

EDIT: und weiter oben auch nochmal. "The default implementation is equivalent to the following steps for this map, then returning the current value or null if now absent:"

 

[guguli]

Ja habe ich übersehen

Ich versuche noch gleichzeitig einen Junit test zu schreiben.
Meine Überlegung ist eine Objekt der Klasse z.B. 2013DTO zu erzeugen dann dieses an getInstance(dto) übergeben.der Rückgabewert ist ja vom typ EStGService<T>,
aber wie kann ich denn das Testen. mit assertTrue und dann instanceOf irgendwie?
ALso irgendwie so:

   Factory factory;
   Service2013 service2013;

   @Before
   public void setUp() {
     factory = new Factory();
       service2013 = new Service2013();
   }

   @After
   public void tearDown() {
       Factory = null;
       service2013= null;
   }

   @Test
   public void getInstance2013Test() {
       
       assertTrue(service2013 instanceOf ...)
   }

das geht nicht ich kann da factory nicht mehr benutzen. Ich wprde gerne sowas schreiben:

  assertTrue(service2013 instanceOf factory.getInstacne(2013DTO.class));

Oder ist mein Ansatz komplett falsch?

 

[guguli]

das kann ich jetzt testen:

@Test
   public void getInstance2013Test() {
       EService<2013DTO>  dto = factory.getInstance(2013DTO.class);
       
       assertTrue(dto instanceof Service2013);
   }

bekomme aber eine ClassCastException:
java.lang.ClassCastException: de.test.Service2013 cannot be cast to java.util.function.Function

aus dieser zeile:

if (instance != null) {
           return (Function<? super Class, ? extends Supplier<EService>>) instance.get();
       }

 

[mihe7]

aus dieser zeile:

Wo ist die Zeile her? Der Cast ist natürlich falsch.

 

[guguli]

Wo ist die Zeile her? Der Cast ist natürlich falsch.

in der newInstance methode

@SuppressWarnings({ "unchecked" })
   public static <T> EService<T> getInstance(Class<T> dto) {
       return (EService<T>) instances.computeIfAbsent(dto, newInstance(dto));
       

   }
   
   @SuppressWarnings({ "rawtypes", "unchecked" })
   private static <T> Function<? super Class, ? extends Supplier<EService>> newInstance(Class<T> dto) {
       Supplier<EService> instance = instances.get(dto);

       if (instance != null) {
           return (Function<? super Class, ? extends Supplier<EService>>) instance.get();
       }
       return null;
   }

 

[mihe7]

Schau Dir nochmal meinen Code aus Kommentar #11 an. Für newInstance brauchst eine Map von Class auf Supplier<EService>. Der Supplier erzeugt die betreffende EService-Instanz, sobald get() (des Suppliers) aufgerufen wird. Für getInstance brauchst Du zusätzlich eine Map von Class auf EService.

Der Rückgabetyp ist bei beiden Methoden einfach ein EService.

 

[guguli]

Eigentlich finde ich das was ich geschrieben haben klingt logisch aber die Methode computeIfAbsent() braucht als 2. Argument eine Function<>.
Das ist die kommplette Klasse:

public class Factory {

   @SuppressWarnings("rawtypes")
   private static Map<Class, Supplier<EService>> instances = new HashMap<>();

   public Factory() {
   }

   static {
       instances.put(2009DTO.class, () -> new Service2009());
       instances.put(2010DTO.class, () -> new Service2010());
       instances.put(2012DTO.class, () -> new Service2012());
       instances.put(2013DTO.class, () -> new Service2013());
       instances.put(2015DTO.class, () -> new Service2015());
       instances.put(2017DTO.class, () -> new Service2017());
       instances.put(2019DTO.class, () -> new Service2019());

   }

   
   @SuppressWarnings({ "unchecked" })
   public static <T> EService<T> getInstance(Class<T> dto) {
       return (EService<T>) instances.computeIfAbsent(dto, newInstance(dto));
       

   }
   
   @SuppressWarnings({ "rawtypes", "unchecked" })
   private static <T> EService<T> newInstance(Class<T> dto) {
       Supplier<EService> instance = instances.get(dto);

       if (instance != null) {
           return instance.get();
       }
       return null;
   }
}

in static{} wird ja gemappt. und in newInstance wird ja mit get() die instanze geholt. aber wie gesagt die Methode computeIfAbsent brucht als 2. Argument eine Function<>.

Für getInstance brauchst Du zusätzlich eine Map von Class auf EService.

Versthe nicht was du damit meinst.

 

[mihe7]

Eigentlich finde ich das was ich geschrieben haben klingt logisch

Naja, Dein Rückgabetyp wäre eine Function gewesen, die eine Class auf einen Supplier abbildet. Die Funktion würde also das Problem lösen, das mit der Methode gelöst werden soll. Da beißt sich der Hund in den Schwanz.

Versthe nicht was du damit meinst.

Lassen wir die Generics mal weg, dann wird es vielleicht deutlicher.

Map<Class,Supplier> suppliers;
Map<Class,EService> instances;

Deine Supplier erzeugen Objekte. D. h. ich kann mir aus der Map einen Supplier ziehen und mir dann vom Supplier ein neues Objekt erstellen lassen, das in Deinem Fall das EService-Interface implementiert. Das ist es, was die newInstance-Methode macht (darum auch das new im Namen der Methode).

Du willst aber je Class höchstens eine EService-Instanz erzeugen. Deswegen musst Du die von newInstance erzeugte EService-Instanz irgendwo cachen. Das machst Du hier einfach über die zweite Map instances.

Die Methode getInstance schaut nun einfach im Cache (das ist die Map instances) nach, ob schon eine EService-Instanz erzeugt wurde. Falls ja, wird diese zurückgegeben, falls nein wird mittels newInstance eine neue Instanz erzeugt, im Cache abgelegt und dann zurückgegeben.

 

[mihe7]

computeIfAbsent() braucht als 2. Argument eine Function

return (EService<T>) instances.computeIfAbsent(dto, k -> newInstance(dto));

oder

return (EService<T>) instances.computeIfAbsent(dto, Factory::newInstance);

 

[guguli]

Ich glaub ich habe das verstanden

public class Factory {

   @SuppressWarnings("rawtypes")
   private static Map<Class, Supplier<EService>> suppliers = new HashMap<>();
   @SuppressWarnings("rawtypes")
   private static Map<Class, EService> instances = new HashMap<>();

   public Factory() {
   }

   static {
       suppliers.put(2009DTO.class, () -> new Service2009());
       suppliers.put(2010DTO.class, () -> new Service2010());
       suppliers.put(2012DTO.class, () -> new Service2012());
       suppliers.put(2013DTO.class, () -> new Service2013());
       suppliers.put(2015DTO.class, () -> new Service2015());
       suppliers.put(2017DTO.class, () -> new Service2017());
       suppliers.put(2019DTO.class, () -> new Service2019());

   }

   
   @SuppressWarnings({ "unchecked" })
   public static <T> EService<T> getInstance(Class<T> dto) {
       return (EService<T>) instances.computeIfAbsent(dto, k -> newInstance(dto));
       

   }
   
   @SuppressWarnings({ "rawtypes", "unchecked" })
   private static <T> EService<T> newInstance(Class<T> dto) {
       Supplier<EService> instance = suppliers.get(dto);

       if (instance != null) {
           return instance.get();
       }
       return null;
   }
}

das ist mein JUnit test, es sieht eigentlich korrekt aus oder?

public class FactoryTest {

   Factory factory;
   Service2013 service2013;

   @Before
   public void setUp() {
       factory = new Factory();
       service2013 = new Service2013();
   }

   @After
   public void tearDown() {
       factory = null;
   }

   @SuppressWarnings("static-access")
   @Test
   public void getInstance2013Test() {
       EService<2013DTO>  dto = Factory.getInstance(2013DTO.class);
       assertTrue(dto instanceof Service2013);
   }
}

 

[mrBrown]

Du solltest die SuppressWarnings noch beseitigen, die meisten dürfte man verhindern können.

das ist mein JUnit test, es sieht eigentlich korrekt aus oder?

In der Klasse ist alles überflüssig bis auf die eigentliche Test-Methode.

 

[mihe7]

Noch was: entweder ich habe etwas verpasst oder die Bezeichner Deiner DTO-Klassen dürften gar nicht erlaubt sein, weil sie mit einer Ziffer beginnen.

 

[mihe7]

import java.util.HashMap;
import java.util.Map;
import java.util.function.Supplier;

public abstract class Factory {

   private static Map<Class<?>, Supplier<EService<?>>> suppliers = new HashMap<>();
   private static Map<Class<?>, EService<?>> instances = new HashMap<>();

   static {
       suppliers.put(DTO2009.class, () -> new Service2009());
       suppliers.put(DTO2010.class, () -> new Service2010());
       suppliers.put(DTO2011.class, () -> new Service2011());
   }

   
   @SuppressWarnings("unchecked")
   public static <T> EService<T> getInstance(Class<T> dto) {
       return (EService<T>) instances.computeIfAbsent(dto, k -> newInstance(dto));      
   }
   
   @SuppressWarnings("unchecked")
   private static <T> EService<T> newInstance(Class<T> dto) {
       Supplier<EService<?>> instance = suppliers.get(dto);

       if (instance != null) {
           return (EService<T>)instance.get();
       }
       return null;
   }
}

 

[guguli]

Noch was: entweder ich habe etwas verpasst oder die Bezeichner Deiner DTO-Klassen dürften gar nicht erlaubt sein, weil sie mit einer Ziffer beginnen.

ja ich weiss, die bezeichnungen sind zu lang, daher habe ich sie abgeschnitten

Ich danke dir, hast mir sehr geholfen.(Y)

 

[mihe7]

Noch ein paar Worte zu Deinem Test:

public void getInstance2013Test() {
       EService<2013DTO>  dto = Factory.getInstance(2013DTO.class);
       assertTrue(dto instanceof Service2013);
}

Die Bezeichnung der Variable dto ist "falsch" - Du bekommst einen Service, kein DTO.

Dein Test ist nicht vollständig. Es wird erwartet, dass getInstance
a) null liefert, falls für die angegebene DTO-Klasse kein Service registriert ist,
b) nicht null liefert, falls für die angegebene DTO-Klasse ein Service registriert ist,
c) auch bei mehrmaligem Aufruf mit der selben DTO-Klasse immer die selbe Referenz liefert

Analog gilt dies für newInstance, wobei sich die Factory bei c) genau andersrum verhält: dort wird bei mehrmaligem Aufruf jeweils eine andere Referenz erwartet.