XML Einlesen - Welche API ist die richtige?

[Ollek]

Hallo,

ich schreibe gerade eine Anwendung in der man Systemdaten hinterlegen kann.
Diese Systemdaten möchte ich auch wenn das Programm geschlossen wird in einer XML speichern, damit diese beim nächsten Programmaufruf wieder zur Verfügung stehen.

Jetzt bin ich mir nicht sicher, mit welcher API ich diese ganze Sache angehen soll. Ich habe das ganze mal mit JAXP ausprobiert. Das Einlesen klappt auch. Allerdings frage ich mich, ob dieses die richtige API ist. Habe vorher nicht viel mit XML in Java gemacht. Besonders kein Auslesen und später wieder reinschreiben.

Also der Ablauf soll so sein: Programmstart --> Einlesen der Datei --> Programmende --> Schreiben der Datei.

Ich habe die folgende Struktur in der XML
[XML]
<?xml version="1.0" encoding="UTF-8"?>
<environment>
<system>
<name>system1</name>
<hostname>system1</hostname>
<user>user1</user>
<password>user1pwd</password>
</system>
<system>
<name>system2</name>
<hostname>system2</hostname>
<user>user2</user>
<password>userpwd2</password>
</system>
</environment>
[/XML]

Hier einmal mein Programmcode. Kann man das so machen?

System-Klasse

@XmlAccessorType(XmlAccessType.FIELD)
@XmlType(name = "", propOrder = {
    "name",
    "hostname",
    "user",
    "password"
})
@XmlRootElement(name = "system")
public class System {

    @XmlElement(required = true)
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    @XmlSchemaType(name = "NCName")
    protected String name;
    @XmlElement(required = true)
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    @XmlSchemaType(name = "NCName")
    protected String hostname;
    @XmlElement(required = true)
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    @XmlSchemaType(name = "NCName")
    protected String user;
    @XmlElement(required = true)
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    @XmlSchemaType(name = "NCName")
    protected String password;

    public System(String name, String hostname, String user, String password) {
		this.name = name;
		this.hostname = hostname;
		this.user = user;
		this.password = password;
	}

public System() {
		
}

    public String getName() {
        return name;
    }

    public void setName(String value) {
        this.name = value;
    }

    public String getHostname() {
        return hostname;
    }

    public void setHostname(String value) {
        this.hostname = value;
    }

    public String getUser() {
        return user;
    }

    public void setUser(String value) {
        this.user = value;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String value) {
        this.password = value;
    }
}

Environment-Klasse

@XmlAccessorType(XmlAccessType.FIELD)
@XmlType(name = "", propOrder = {
    "system"
})
@XmlRootElement(name = "environment")
public class Environment {

	private static Environment environment = null;
	
	private Environment(){
		
	}
	
	public void setEnvironment(Environment environment){
		this.environment = environment;
	}
	
	public static Environment getInstance(){
		if(environment == null){
			environment = new Environment();
		}
		return environment;
	}
	
    @XmlElement(required = true)
    protected ArrayList<System> system;

    public ArrayList<System> getSystem() {
        if (system == null) {
            system = new ArrayList<System>();
        }
        return this.system;
    }
}

ObjectFactory-Klasse

@XmlRegistry
public class ObjectFactory {

    private final static QName _Name_QNAME = new QName("", "name");
    private final static QName _Hostname_QNAME = new QName("", "hostname");
    private final static QName _Password_QNAME = new QName("", "password");
    private final static QName _User_QNAME = new QName("", "user");

    public ObjectFactory() {
    	
    }

    public System createSystem() {
        return new System();
    }

    public Environment createEnvironment() {
        return Environment.getInstance();
    }

    @XmlElementDecl(namespace = "", name = "name")
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    public JAXBElement<String> createName(String value) {
        return new JAXBElement<String>(_Name_QNAME, String.class, null, value);
    }

    @XmlElementDecl(namespace = "", name = "hostname")
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    public JAXBElement<String> createHostname(String value) {
        return new JAXBElement<String>(_Hostname_QNAME, String.class, null, value);
    }

    @XmlElementDecl(namespace = "", name = "password")
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    public JAXBElement<String> createPassword(String value) {
        return new JAXBElement<String>(_Password_QNAME, String.class, null, value);
    }

    @XmlElementDecl(namespace = "", name = "user")
    @XmlJavaTypeAdapter(CollapsedStringAdapter.class)
    public JAXBElement<String> createUser(String value) {
        return new JAXBElement<String>(_User_QNAME, String.class, null, value);
    }

}

XMLToObject - Klasse (Hier wird der ganze Vorgang gestartet)
Hier Frage ich mich, ob es ebenfalls in Ordnung ist, wie ich das mit dem Environment umgesetzt habe. Kann ich mit in meiner Singleton-Klasse Environment eine setEnvironment() Methode schreiben die ein Environment Object entgegennimmt und das einzige Object damit updatet? Sinn hie rist, dass ich die ArrayList mit den Systems in jeder Klasse verfügbar sind. Denn diese zeige ich später in einer JList an.
Ist eventuell ein wichtiger Hintergrund, damit ihr mein Problem versteht, bzw. mir helfen könnt.

public class XMLtoObject {
	
	static Log4JLogger logger = Log4JLogger.getInstance();

	public static void createObjectFromXML(){
		try {
			JAXBContext jc = JAXBContext.newInstance("de.ernstings.xml");
			Unmarshaller unmarshaller = jc.createUnmarshaller();
			
			Environment environment = Environment.getInstance(); 
			environment = (Environment) unmarshaller.unmarshal(new File(System.getProperty("user.dir") + "\\conf\\environment.xml"));
			Environment.getInstance().setEnvironment(environment);
			
//			for(int i = 0; i < environment.getSystem().size(); i++){
//				de.ernstings.model.System system = environment.getSystem().get(i);
//				System.out.println("System " + system.getName());
//			}
		} catch (JAXBException e) {
			logger.log(Level.ERROR, XMLtoObject.class, "Fehler beim Einlesen der XML Datei.", e);
		}
	}
}

Wenn noch eine Verbesserung entweder mit einer anderen API oder im Programmcode würde ich mich über eure Vorschläge freuen..

Viele Grüße von der Insel

 

[nillehammer]

Ich designe es immer so, dass primitive Attribute in Java auch Attribute im XML sind. Ich würde also name, hostname etc. als XML-Attribute des system-Elements modellieren, nicht als eigene Elemente. Das spart etwas overhead und liest sich meiner Meinung nach auch besser. An Overhead sparst Du dir bei Nutzung von Attributen statt Kindelementen viele schließende Tags.

Bei den Attribut-/Elementnamen halte ich mich immer an die Java-Konvention. Elemente sind für mich die Instanzen von Klassen. Ihre Namen modelliere ich deswegen entsprechend der Namens-Konvention für Klassen. Bei Attributen das gleiche.

Das resultierende XML sähe bei mir dann so aus:
[XML]
<?xml version="1.0" encoding="UTF-8"?>
<Environment>
<System name="system1" hostName="system1" user="user1" password="user1pwd">
<System name="system2" hostName="system2" user="user2" password="user2pwd">
</Environment>
[/XML]
Die XML-Struktur ist ja auch etwas Geschmackssache. So find ich es halt hübsch.

Und zur Frage:

Hier Frage ich mich, ob es ebenfalls in Ordnung ist, wie ich das mit dem Environment umgesetzt habe. Kann ich mit in meiner Singleton-Klasse Environment eine setEnvironment() Methode schreiben die ein Environment Object entgegennimmt und das einzige Object damit updatet?

Finde ich nicht gut. Meiner Meinung nach vermischst Du hier zwei Aufgaben (sog. concerns), die getrennt gehören:
1. Du hast das Environment mit der SystemList als Daten. Das ist für mich ganz klar eine klassische Java-Bean.
2. Darüber hinaus hast Du die Aufgabe zentraler Datenhaltung, die ermöglicht, von überall im Code auf dieselben Daten zuzugreifen. Das ist für mich ein Service, der zunächst als Interface spezifiziert gehört. Die Implementierung darf dann gerne ein Singleton sein.

 

[Ollek]

Deine Verbesserung mit den Attributen am System-Element setze ich um. Danke für den Vorschlag. Wenn man drüber nachdenkt macht es auch mehr sinn.

[XML]
<?xml version="1.0" encoding="UTF-8"?>
<Environment>
<System name="system1" hostName="system1" user="user1" password="user1pwd">
<System name="system2" hostName="system2" user="user2" password="user2pwd">
</Environment>
[/XML]

Und zur Frage:

Finde ich nicht gut. Meiner Meinung nach vermischst Du hier zwei Aufgaben (sog. concerns), die getrennt gehören:
1. Du hast das Environment mit der SystemList als Daten. Das ist für mich ganz klar eine klassische Java-Bean.
2. Darüber hinaus hast Du die Aufgabe zentraler Datenhaltung, die ermöglicht, von überall im Code auf dieselben Daten zuzugreifen. Das ist für mich ein Service, der zunächst als Interface spezifiziert gehört. Die Implementierung darf dann gerne ein Singleton sein.

Also Environment deklariere ich nicht als Singleton, sondern lasse die so wie die System-Klasse, richtig?

Das mit dem Interface und deren zugriff habe ich dann noch nicht verstanden. Könntest du mir hier ein Beispiel liefern? Stehe dort irgendwie auf dem Schlau. ???:L

 

[nillehammer]

Environment = Datenklasse = normale Java-Bean, also ganz rudimentär so:

public final class Environment {

  private List<System> systems;

  public final List<System> getSystems() {

     return this.systems
  }

  public final void setSystems(List<System> systems) {

    this.systems = systems;
  }
}

Dann die Datenhaltung. Welche Funktion würdest Du von ihr erwarten? Daraus werden Methoden, die in einem Interface definiert werden, ungefähr so:

public interface Datenhaltung {

   public Environment getEnvironment();

   public void updateEnvironment(Environment environment);

   ... irgendwann kommt bestimmmt mehr dazu ...
}

Dann die Implementierung. Hier im Moment ein Singleton, wer weiß, was später:

public enum DatenHaltungSingleton implements Datenhaltung {
  INSTANCE;

  private Environment environment;

  @Override
  public Environment getEnvironment() {
     return this.environment;
  }

  @Override
  public void updateEnvironment(Environment environment) {
     this.environment = environment;
  }

Dann die Verwendung im Code:

private final DatenHalgung datenHaltung = DatenHaldungSingleton.INSTANCE;
.....
Environment environment = datenHaltung.getEnvironment();

Die Implementierung DatenHaldungSingleton ist im Moment recht "langweilig". Aber das wird im Verlaufe des Projekts wachsen. Vielleicht würde die Logik für das Parsen des XML und marshalling/unmarshalling hier noch gut reinpassen, in dem Du eine zweite Implementierung "DatenhaltungXMLImpl" oder so schreibst.

 

[Ollek]

Vielen Dank für das ausführliche Beispiel.

Werde das mal auf meine Anwendung umsetzen und es danach hier posten. :toll:

 

[Ollek]

Hallo,

hier mal eine funktionierende Lösung. Bin auf deine Meinung gespannt.
Habe mich nun für DOM entschieden, da es nicht mehr als 10 Systeme geben wird.

XMLInObject
Hier wird die XML eingelesen und in System Objekten geschrieben.

public class XMLInObject {
	
	private static Log4JLogger logger = Log4JLogger.getInstance();
	private DataStorage dataStorage = DataStorageSingleton.getInstance();
	
	public void parseFile() throws DocumentException {
			//Speicherort XML-Datei
			File file = new File(java.lang.System.getProperty("user.dir") + "/conf/environment.xml");
			
			SAXReader reader = new SAXReader();
			Document document = reader.read(file);
			
			dataStorage.updateDocument(document);
			createEnvironmentObject(document.getRootElement());
	}

	/**
	 * Erstellt ein Environment-Object
	 * aus einem XML-Element
	 * 
	 * @param environmentXML
	 */
	public void createEnvironmentObject(Element root){
			Environment environment = new Environment();
			
			List list = root.elements("system");
			for(int i = 0; i < list.size(); i++){
				Element current = (Element) list.get(i);
				System system = createSystemObject(current);
				
				environment.addSystem(system);
			}
			dataStorage.updateEnvironment(environment);
	}


	/**
	 * Erstellt ein System-Object 
	 * aus den XML-Attributen des Elememts
	 * 
	 * @param currentSystem
	 * @return
	 */
	public System createSystemObject(Element currentSystem) {
		String name = currentSystem.attributeValue("name").trim();
		java.lang.System.out.println(name);
		String hostname = currentSystem.attributeValue("hostname");
		String user = currentSystem.attributeValue("user");
		String password = Base64Coding.decode(currentSystem.attributeValue("password"));
		
		System system = new System(name, hostname, user, password); 
		return system;
	}

Environment = Datenklasse

public class Environment {

    private ArrayList<System> systems;
    
    public Environment(){
    	this.systems = new ArrayList<System>();
    }
    
    public void addSystem(System system){
    	this.systems.add(system);
    }

    public ArrayList<System> getSystems() {
        return this.systems;
    }
    
    public void setSystems(ArrayList<System> systems){
    	this.systems = systems;
    }

}

System = Datenklasse

public class System {

    private String name;
    private String hostname;
    private String user;
    private String password;

    public System(String name, String hostname, String user, String password) {
		this.name = name;
		this.hostname = hostname;
		this.user = user;
		this.password = password;
	}

	public System() {
		
	}

	public String getName() {
        return name;
    }

    public void setName(String value) {
        this.name = value;
    }

    public String getHostname() {
        return hostname;
    }

    public void setHostname(String value) {
        this.hostname = value;
    }

    public String getUser() {
        return user;
    }


    public void setUser(String value) {
        this.user = value;
    }

    
    public String getPassword() {
        return password;
    }

    
    public void setPassword(String value) {
        this.password = value;
    }

}

DataStorageSingleton
Environment ist somit nur einmal im Programm.

public class DataStorageSingleton implements DataStorage {
	
	private static DataStorageSingleton instance = null;
	private Environment environment;
	
	public static DataStorageSingleton getInstance(){
		
		if(instance == null){
			instance = new DataStorageSingleton();
		}
		
		return instance;
	}

	@Override
	public Environment getEnvironment() {
		return this.environment;
	}

	@Override
	public void updateEnvironment(Environment environment) {
		this.environment = environment;
	}
}

Interface DataStorage

public interface DataStorage {

	public Environment getEnvironment();
	
	public void updateEnvironment(Environment environment);
	
	public Document getDocument();
	
	public void updateDocument(Document document);
}

Noch verbesserungsvorschläge?
Freu mich drauf!

Hättest du es anders gemacht?

Achja, XML
[XML]
<?xml version="1.0" encoding="UTF-8"?>

<environment>
<system name="System1" hostname="db1" user="" password=""/>
<system name="System2" hostname="db2" user="" password=""/>
</environment>

[/XML]

Viele Grüße

 

[nillehammer]

Bist vom JAXB-Mapping jetzt weg und machst es selbst? Warum?
Die Definition der Systems als ArrayList gefällt mir nicht so gut:

public class Environment {
    // Wenn nicht unbedingt ein Detail aus der Implementierung
    // benötigt wird, verwende besser den möglichst allgemeinen
    // Typen, das wäre hier das List-Interface
    // private ArrayList<System> systems;
    private List<System> systems;

...
 // Gleiches bei den gettern/settern
 public List<System> getSystems() {
        return this.systems;
    }

  public void setSystems(List<System> systems){
        this.systems = systems;
  }

Bei den Datenklassen mache Dir nochmal Gedanken, ob Du die ganzen Getter und Setter sowie die Konstruktoren wirklich alle brauchst (nachdem Du jetzt kein JAXB mehr machst, wahrscheinlich nicht). Z.B. wäre es mit setSystems(...) möglich, die Liste null zu setzen, was bei einem nachfolgenden add zu einer NullointerException führen würde. Du rufst den Setter auch nirgends auf. Brauchst Du ihn also unbedingt? Falls nicht, weg damit! Je weniger Code, desto weniger Fehlerrisiko. Das ist ein Pattern, an das man sich bei jeder Klasse halten sollte.

 

[Ollek]

Gefiel mir einfach besser und war für mich verständlicher und nachvollziehbarer..

Wenn ich aber in meinem ganzen Programm die ArrayList<System> verwenden, dann würde es doch Sinn machen oder?

Was hältst du eigentlich davon, das ich das Document ebenfalls mit in den DataStorageSIngleton gepackt habe?

VG

 

[nillehammer]

Wenn ich aber in meinem ganzen Programm die ArrayList<System> verwenden, dann würde es doch Sinn machen oder?

Nein, es macht nie Sinn, einen speziellen Typen zu verwenden, wenn der allgemeinere alle Funktionen bietet, die man braucht. Ich würde also dazu raten, auch den Rest Deines Programms anzupassen.

Ich kenne eigentlich nur einen Anwendungsfall, wo man explizit mit ArrayList arbeiten sollte. Nämlich, wenn die Liste sehr groß ist und es sehr viele Direktzugriffe per Index gibt.

Was hältst du eigentlich davon, das ich das Document ebenfalls mit in den DataStorageSIngleton gepackt habe?

Hab ich garnicht bemerkt, weil die von Dir gepostete Implementierung den entsprechenden Code nicht enthält. Die DataStorage ist ja die Brücke zwischen der Java-Welt und der physischen Speicherung (in dem Fall hier XML). Insofern muss die natürlich auch im Zweifel die physische Speicherung kennen. Aber so, wie Du es umgesetzt hast, ist es nicht ganz richtig. Du hast es als Interface-Methoden spezifiziert. Damit hast du die public API auf XML festgelegt. Stell Dir mal vor, Du würdest das ganze irgendwann auf eine Datenbank umstellen. Da würde die Methode nicht mehr viel Sinn machen. Übergib es besser als Konstruktorparameter. Oder schreibe Dir widerrum einen weiteren Service, der das Laden des Document übernimmt und der von Deiner Storage-Implementierung aufgerufen wird.

 

[Ollek]

Ich werde beides morgen umsetzen und hier meine Lösung Posten.

 

[Ollek]

Nein, es macht nie Sinn, einen speziellen Typen zu verwenden, wenn der allgemeinere alle Funktionen bietet, die man braucht. Ich würde also dazu raten, auch den Rest Deines Programms anzupassen.

Ich kenne eigentlich nur einen Anwendungsfall, wo man explizit mit ArrayList arbeiten sollte. Nämlich, wenn die Liste sehr groß ist und es sehr viele Direktzugriffe per Index gibt.

this.systems = new List<DB2System>();

Das funktioniert nicht, dass er mir hier eine Fehlermeldung ausgibt.
Liegt daran, dass List nen Interface ist. Nehme ich an der Stelle dann ArrayList?? ???:L

Das andere setze ich gleich mal um.

 

[nillehammer]

Liegt daran, dass List nen Interface ist. Nehme ich an der Stelle dann ArrayList??

Ja, bei der Definition von Variablen, Parametern und Rückgabewerten nimmst Du das (abstrakte) Interface. Bei der Zuweisung musst Du dann natürlich eine (konkrete) Implementeriung (z.B. ArrayList) nehmen:

private List<System> = new ArrayList<>();

 

[Ollek]

Hab ich garnicht bemerkt, weil die von Dir gepostete Implementierung den entsprechenden Code nicht enthält. Die DataStorage ist ja die Brücke zwischen der Java-Welt und der physischen Speicherung (in dem Fall hier XML). Insofern muss die natürlich auch im Zweifel die physische Speicherung kennen. Aber so, wie Du es umgesetzt hast, ist es nicht ganz richtig. Du hast es als Interface-Methoden spezifiziert. Damit hast du die public API auf XML festgelegt. Stell Dir mal vor, Du würdest das ganze irgendwann auf eine Datenbank umstellen. Da würde die Methode nicht mehr viel Sinn machen. Übergib es besser als Konstruktorparameter. Oder schreibe Dir widerrum einen weiteren Service, der das Laden des Document übernimmt und der von Deiner Storage-Implementierung aufgerufen wird.

Habe nun das Document und deren Methoden aus dem Interface rausgenommen.
Habe in der DataStorageSingleton einfach eine Variable und Mathode deklariert, die er vorher überes Interface implementieren musst. Das ist doch in Ordnung, oder?

Hier die Überarbeitete Klasse:

public class DataStorageSingleton implements DataStorage {
	
	private static DataStorageSingleton instance = null;
	private Environment environment;
	private Document document;
	
	public static DataStorageSingleton getInstance(){
		
		if(instance == null){
			instance = new DataStorageSingleton();
		}
		
		return instance;
	}

	@Override
	public Environment getEnvironment() {
		return this.environment;
	}

	@Override
	public void updateEnvironment(Environment environment) {
		this.environment = environment;
	}

	public Document getDocument() {
		return document;
	}

	public void updateDocument(Document document) {
		this.document = document;
	}
}

Oder noch eine Verbesserung? :rtfm:
Vielen Dank schonmal

 

[Ollek]

Ja, bei der Definition von Variablen, Parametern und Rückgabewerten nimmst Du das (abstrakte) Interface. Bei der Zuweisung musst Du dann natürlich eine (konkrete) Implementeriung (z.B. ArrayList) nehmen:

private List<System> = new ArrayList<>();

Habe ich so umgesetzt. :toll:

 

[nillehammer]

Habe in der DataStorageSingleton einfach eine Variable und Mathode deklariert, die er vorher überes Interface implementieren musst. Das ist doch in Ordnung, oder?

Die Schwäche des Implementierungsdetails "XML" hast du jetzt aus dem Interface raus. Damit wäre ein DataStorage schon mal gegen andere Implementierungen austauschbar. Hauptziel also erreicht.

Aber, die Schwäche ist aber noch nicht ganz verschwunden, weil es in der Implementierung noch sichtbare (public) Methoden gibt, die das Implementierungsdetail "XML" nicht nur "verraten" sondern auch explizit genutzt werden müssen, um die Java-Repräsentation (Environment) mit der XML-Repräsentation (Document) synchron zu halten.

Bei einem perfekten Design dürfte es nur public Methoden geben, die mit den Java-Objekten arbeiten (getEnvrionment, saveEnvironment, updateEnvironment o.ä). Und das DataStorage kümmert sich dann intern um alles.

Aber es ist schon gut genung. Wenn es funktioniert, steck Deine Zeit lieber in andere Teile des Projekts.

Oder noch eine Verbesserung?

Nur eine kleine: Initialisiere die instance-Variable sofort bei der Deklaration und gib sie in getInstance einfach zurück, anstatt sie lazy mit

if(instance=null)

zu initialisieren. Da hier keine komplizierten Objektstrukturen hinter der Implementierung stecken, gewinnst Du durch die lazy-Variante fast nichts. Du musst aber mehr Code schreiben und hast eine kleines Risiko, dass ungewollt mehrere Instanzen erzeugt werden, wenn mehrere Threads gleichzeitig die Methode getInstance aufrufen.