2010-01-11 2 views
7

Ich habe es mit einer Reihe von Nachrichtenobjekten zu tun, von denen jedes eine eindeutige Kennung hat, die ihnen entspricht. Jede Nachricht kann entweder aus einer Map oder aus einem ByteBuffer erstellt werden (die Nachrichten sind binär, aber wir wissen, wie man zu und von einer binären Repräsentation überträgt).Java - statische Factory-Methode und Switch-Anweisungen

Die aktuelle Implementierung für diese Meldungen Konstruktion ist in etwa wie folgt: Jetzt

public static Message fromMap(int uuid, Map<String, Object> fields) { 
    switch (uuid) { 
     case FIRST_MESSAGE_ID: 
     return new FirstMessage(fields); 
     . 
     . 
     . 
     default: 
      // Error 
      return null; 
    } 
} 

public static Message fromByteBuffer(int uuid, ByteBuffer buffer) { 
    switch (uuid) { 
     case FIRST_MESSAGE_ID: 
     return new FirstMessage(buffer); 
     . 
     . 
     . 
     default: 
      // Error 
      return null; 
    } 
} 

, Josh Bloch's Effective Java spricht über Punkt 1: statt Konstrukteuren statische Factory-Methoden Betrachten, und dies scheint ein Ort zu sein, wo dieses Muster nützlich (Clients greifen nicht direkt auf die Konstruktoren der Nachrichtenuntertypen zu; stattdessen durchlaufen sie diese Methode). Aber ich mag es nicht, dass wir daran denken müssen, zwei Switch-Statements zu aktualisieren (verletzt das DRY Prinzip).

Ich würde jede Einsicht in die beste Möglichkeit, dies zu erreichen, schätzen; Objekte werden nicht zwischengespeichert (jeder Aufruf von fromMap oder fromByteBuffer gibt ein neues Objekt zurück), wodurch einige Vorteile einer solchen statischen Factory-Methode zunichte gemacht werden. Etwas an diesem Code erscheint mir falsch, daher würde ich gerne die Gedanken der Community darüber hören, ob dies ein gültiger Weg ist, um neue Objekte zu konstruieren, oder wenn nicht, was eine bessere Lösung wäre.

Antwort

12

Vielleicht könnten Sie eine Schnittstelle MessageFactory und Implementierungen davon erstellen:

public interface MessageFactory { 
    Message createMessage(Map<String, Object> fields); 
    Message createMessage(ByteBuffer buffer); 
} 

public class FirstMessageFactory implements MessageFactory { 
    public Message createMessage(Map<String, Object> fields){ 
    return new FirstMessage(fields); 
    } 

    public Message createMessage(ByteBuffer buffer){ 
    return new FirstMessage(buffer); 
    } 

} 

nächstes wird ein Verfahren getFactoryFromId in der gleichen Klasse wie die oben genannten Methoden:

public static MessageFactory getMessageFactoryFromId(int uuid){ 
switch (uuid) { 
    case FIRST_MESSAGE_ID: 
    return new FirstMessageFactory(); 
    ... 
    default: 
     // Error 
     return null; 
    } 
} 

jedoch statt dessen, Es ist besser, eine Hashmap zu erstellen, die die IDs und die Factories enthält, sodass Sie nicht jedes Mal ein neues Factory-Objekt erstellen müssen, wenn Sie eine Nachricht erstellen. Siehe auch die comment below.

und Ihre Methoden:

public static Message fromMap(int uuid, Map<String, Object> fields) { 
    getMessageFactoryFromId(uuid).createMessage(fields); 
} 

public static Message fromByteBuffer(int uuid, ByteBuffer buffer) { 
    getMessageFactoryFromId(uuid).createMessage(buffer); 
} 

Auf diese Weise Sie werden mit der Fabrik-Muster, und es gibt keine Notwendigkeit, zweimal den gleichen Switch-Anweisung zu haben.

(nicht getestet haben, so möglicherweise einige der Kompilierung Fehler/Fehler)

+0

Wenn die Factory-Objekte in einer Map gespeichert und mit UUID abgerufen werden, ist kein Switch erforderlich. – rsp

+0

yup, das habe ich gesagt :-) Ich habe sogar einen Link zu deiner Antwort hinzugefügt :) – Fortega

+0

Es ist eine gute Lösung (also +1 dafür), aber wenn das Ziel darin bestand, den Doppel-Switch loszuwerden, hättest du nur faktorisieren können aus der Switch-Logik zu einer separaten Methode. (Die Karte ist im Grunde ein getarnter Schalter) – wds

0

Sie die AbstractFactory Muster verwenden können, in dem Sie eine Factory-Klasse für jeden Nachrichtentyp haben würde, dass Sie die Nachricht zurückgibt entweder durch Puffer oder Karte. Sie erstellen dann eine Methode, die Ihnen das entsprechende Factory-Objekt zurückgibt. Aus der zurückgegebenen Factory erstellen Sie dann die Nachricht.

1

Gibt es eine Möglichkeit, den ByteBuffer in eine Karte oder etwas anderes zu konvertieren? Es wäre nett, wenn Sie die Eingabe in eine normalisierte Form konvertieren und einen eindeutigen Schalter anwenden.

Wenn Sie eine Nachricht erhalten und sie mit bestimmten Werten formatieren (wie "Die Tabelle: tableName hat keine Spalte namens: colName"), können Sie den ByteBuffer in eine Map konvertieren und die erste aufrufen Methode. Wenn Sie eine neue msgId benötigen, erweitern Sie nur die Methode fromMap.

Es ist etwas wie Factoring der gemeinsame Teil.

3

Wenn Sie Ihre Objekte wie eine Schnittstelle deklarieren Factory-Methoden implementieren:

public Message newInstance(Map<String, Object> fields); 
public Message newInstance(ByteBuffer buffer); 

in einer statischen geschachtelten Klasse, kann Ihr Werk ein Map enthält Factory-Objekte durch die UUID indiziert erstellen:

Map map = new HashMap(); 

map.put(Integer.valueOf(FirstMessage.UUID), new FirstMessage.Factory()); 

und ersetzen Sie Ihre Schalter durch Kartensuche:

public static Message fromMap(int uuid, Map<String, Object> fields) { 

    Factory fact = map.get(Integer.valueOf(uuid)); 

    return (null == fact) ? null : fact.newInstance(fields); 
} 

public static Message fromByteBuffer(int uuid, ByteBuffer buffer) { 

    Factory fact = map.get(Integer.valueOf(uuid)); 

    return (null == fact) ? null : fact.newInstance(buffer); 
} 

Dies kann leicht erweitert werden, um andere Konstruktionsmethoden zu unterstützen.

+0

Nur ein kleiner Kommentar: Sie sollten Integer.valueOf() anstelle von new Integer() verwenden. – I82Much

+0

Guter Punkt, änderte das. – rsp

1

Ich empfehle eine ENUM-Typ mit abstrakten Methoden, wie das folgende Beispiel:

enum MessageType { 

    FIRST_TYPE(FIRST_MESSAGE_ID) { 

     @Override 
     Message fromByteBuffer(ByteBuffer buffer) { 
      return new FirstMessage(buffer); 
     } 

     @Override 
     Message fromMap(Map<String, Object> fields) { 
      return new FirstMessage(fields); 
     } 

     @Override 
     boolean appliesTo(int uuid) { 
      return this.uuid == uuid; 
     } 

    }, 

    SECOND_TYPE(SECOND_MESSAGE_ID) { 

     @Override 
     Message fromByteBuffer(ByteBuffer buffer) { 
      return new SecondMessage(buffer); 
     } 

     @Override 
     Message fromMap(Map<String, Object> fields) { 
      return new SecondMessage(fields); 
     } 

     @Override 
     boolean appliesTo(int uuid) { 
      return this.uuid == uuid; 
     } 

    }; 

    protected final int uuid; 

    MessageType(int uuid) { 
     this.uuid = uuid; 
    } 

    abstract boolean appliesTo(int uuid); 

    abstract Message fromMap(Map<String, Object> map); 

    abstract Message fromByteBuffer(ByteBuffer buffer); 

} 

diese Weise in Ihre bestehende statische Methoden, die Sie dies einfach tun können ...

public static Message fromByteBuffer(int uuid, ByteBuffer buffer) { 
    Message rslt = null; 
    for (MessageType y : MessageType.values()) { 
     if (y.appliesTo(uuid)) { 
      rslt = y.fromByteBuffer(buffer); 
      break; 
     } 
    } 
    return rslt; 
} 

Dieser Ansatz erspart Ihrer statischen Methode die Kenntnis über die von Ihnen unterstützten MessageTypes und deren Erstellung. Sie können Nachrichten hinzufügen, ändern oder entfernen, ohne die statischen Methoden zu refaktorieren.

0

Sie sollten abstrakt Ihr FirstMessage Objekt:

public abstract Message { 
    // ... 
} 

Dann sie in Ihrem Werk zwischenzuspeichern (im Gegensatz zu einem Schalter entgegengesetzt):

private static final Map<Integer, Class<Message>> MESSAGES = new HashMap<Integer, Class<Message>>(); 
static { 
    MESSAGES.put(1, FirstMessage.class); 
} 

In Ihrem Werk Methode:

public static Message fromMap(UUID uuid, Map<String, Object> fields) { 
    return MESSAGES.get(uuid).newInstance(); 
} 

Das ist sowieso nur eine Idee, man müsste einige Überlegungen (um den Konstruktor zu bekommen) erledigen die Felder.

0

Sie könnten Nachricht so ändern, dass sie zwei Initialisierungsmethoden hat, eine für Map und eine für ByteBuffer (anstelle der beiden Contructor-Versionen). Dann gibt Ihre Factory-Methode die konstruierte (aber nicht initialisierte) Nachricht zurück, dann rufen Sie initialize mit dem Map oder ByteBuffer für das zurückgegebene Objekt auf.

So haben Sie jetzt eine Factory-Methode wie folgt aus: -

private static Message createMessage(int uuid) { 
    switch (uuid) { 
     case FIRST_MESSAGE_ID: 
     return new FirstMessage(); 
     . 
     . 
     . 
     default: 
      // Error 
      return null; 
    } 

} 

und dann die öffentlichen Factory-Methoden werden: -

public static Message fromMap(int uuid, Map<String, Object> fields) { 
    Message message = createMessage(uuid); 
    // TODO: null checking etc.... 
    return message.initialize(fields); 
} 

und

public static Message fromByteBuffer(int uuid, ByteBuffer buffer) { 
    Message message = createMessage(uuid); 
    // TODO: null checking etc.... 
    return message.initialize(buffer); 
} 
3

tem 1: Betrachte statische Fabrikmethoden anstelle von Bauer

Sie tun, dass bereits durch den Konstruktor hinter der Factory-Methode versteckt, so gibt es keine Notwendigkeit, hier eine weitere Fabrik Methode hinzuzufügen.

So können Sie es mit einer Factory-Schnittstelle und einer Karte tun.(Im Grunde, was jeder sagt schon, aber mit dem Unterschied, dass Sie die Fabriken mit inneren Klassen Inline können)

interface MessageFactory { 
    public Message createWithMap(Map<String,Object> fields); 
    public Message createWithBuffer(ByteBuffer buffer); 
} 

Map<MessageFactory> factoriesMap = new HashMap<MessageFactory>() {{ 
    put(FIRST_UUID, new MessageFactory() { 
     public Message createWithMap(Map<String, Object> fields) { 
      return new FirstMessage(fields); 
     } 
     public Message createWithBuffer(ByteBuffer buffer){ 
      return new FirstMessage(buffer); 
     } 
    }); 
    put(SECOND_UUID, new MessageFactory(){ 
     public Message createWithMap(Map<String, Object> fields) { 
      return new SecondMessage(fields); 
     } 
     public Message createWithBuffer(ByteBuffer buffer){ 
      return new SecondMessage(buffer); 
     } 
    }); 
    put(THIRD_UUID, new MessageFactory(){ 
     public Message createWithMap(Map<String, Object> fields) { 
      return new ThirdMessage(fields); 
     } 
     public Message createWithBuffer(ByteBuffer buffer){ 
      return new ThirdMessage(buffer); 
     } 
    }); 
    ... 
}}; 

Und Ihre Beschwörungen werden sich in:

public static Message fromMap(int uuid, Map<String, Object> fields) { 
    return YourClassName.factoriesMap.get(uuid).createWithMap(fields); 
} 

public static Message fromByteBuffer(int uuid, ByteBuffer buffer) { 
    return YourClassName.factoriesMap.get(uuid).createWithBuffer(buffer); 
} 

Da die UUID für die verwendete Schalter wird als Schlüssel für die Fabriken verwendet.

0

Über die Lösung, die die enum als Strategie verwendet (Hinzufügen von Strategie-Methoden zu einer Enum), sagt die clean-code Cheet-Sheet-App, dass dies ein Wartbarkeitskiller ist.
Obwohl ich nicht weiß, warum ich das mit Ihnen teilen möchte.

+0

Interessant. Aber bitte fügen Sie Kommentare wie diese als Kommentare und nicht als Antworten hinzu. – I82Much