Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm facing the issue of how to reconstruct data across a network in Java. As part of my Component based game engine (read about it here, I have to encode components as XML on the server, send it to the client, and then reconstruct the appropriate component with all the data included in the XML. For example:

package.TextureComponent
id 1 /id
width 100 /width
height 100 /height
/package.TextureComponent

Right now, I use reflection on the client to construct a new TextureComponent (but this will work for any component with primitive data types) using the complete name of the class. I then sequentially read all the String parameters from the XML into the component using reflection in this BasicDecoder:

public class BasicDecoder implements DataDecoder {

    @Override
    public void decode(DataComponent component, String[] data) {
        Class<? extends DataComponent> compClass = component.getClass();

        // fills in all public fields with data in order
        Field[] fields = compClass.getFields();
        for (int i = 0; i < fields.length; i++) {
            Field field = fields[i];
            Class fieldType = field.getType();
            String typeName = fieldType.getName().toUpperCase().replace(".", "");
            FieldType type;
            try {
                type = FieldType.valueOf(typeName);
            } catch (Exception ex) {
                throw new RuntimeException("BasicDecoder cannot handle field: " + typeName);
            }
            try {
                switch (type) {
                case INT:
                    field.setInt(component, Integer.parseInt(data[i]));
                    break;
                case DOUBLE:
                    field.setDouble(component, Double.parseDouble(data[i]));
                    break;
                case BYTE:
                    field.setByte(component, Byte.parseByte(data[i]));
                    break;
                case SHORT:
                    field.setShort(component, Short.parseShort(data[i]));
                    break;
                case LONG:
                    field.setLong(component, Long.parseLong(data[i]));
                    break;
                case FLOAT:
                    field.setFloat(component, Float.parseFloat(data[i]));
                    break;
                case BOOLEAN:
                    field.setBoolean(component, Boolean.parseBoolean(data[i]));
                    break;
                case CHAR:
                    field.setChar(component, data[i].charAt(0));
                    break;
                case JAVALANGSTRING:
                    field.set(component, data[i]);
                    break;
                }
            } catch (Exception ex) {
                ex.printStackTrace();
            }
        }
    }

    private enum FieldType {
        INT, DOUBLE, BYTE, SHORT, LONG, FLOAT, BOOLEAN, CHAR, JAVALANGSTRING
    }
}

Is there any way I can improve this code? Is reflection the wrong way to go? I shied away from having each Component define its own decoder because I felt that it would result in a lot of classes which look more or less similar.

share|improve this question

2 Answers 2

up vote 5 down vote accepted

Reflection is not the correct way to do this. You will need Serialization for this. Another option is to use XML marshalling and unmarshalling, but I am not so familiar with that.

For serialization I suggest you to pick up a copy of Effective Java, and read the last couple of items of the book, which are all about Serialization.

You really need to take care of the following, since you are dealing with a game and network:

  • Check your class invariants, constructors may not violate them.
  • Make sure that your seriliazation is all safe as attackers can exploit a lot of things to do some quite surprising things.
  • Take care of the evolvability of your classes, serialization does pose some issues on that.

I would recommend all your classes to implement a static inner classes called ProxyHelper and implement the Proxy Serialization Pattern on the classes.

Some other ad-hoc review points of your current code:

  • Do not catch Exception, catch the subclass(es) that you actually see happening, currently it will also catch RuntimeExceptions.
  • Do preferably not throw RuntimeException, but add somewhat more detail, IllegalStateException may be a candidate here.
  • Follow the coding conventions, you do it quite well in general, but in the switch (type) { } every case (and inherited code lines) need to be indented one more factor.

In general I would never let Reflection do such a task, I would add with the following code, either a public constructor taking all arguments of the object (which you can get from the String[] data), or if there are too many arguments (more than five usually), you can also emply the Builder Pattern.

share|improve this answer
1  
I would not use XML to transfer data if it's a game, a bit too heavy IMO. The concept of marshall/unmarshall would be the same though. –  Marc-Andre May 12 at 13:36
2  
While I agree with your preference on the switch indentations, there are actually reasons why that's not the case (ha ha): stackoverflow.com/questions/4509039/… –  Simon André Forsberg May 12 at 13:46

Effective Java Item 47: Know and use the libraries

There's nothing wrong with reflection to do this in my opinion, but there is no need to reinvent the wheel. There is already a library named Jackson that can do this for you.

Assuming that your XML looks like this:

<package.TextureComponent>
    <id>1</id>
    <width>100</width>
    <height>100</height>
</package.TextureComponent>

You can use Jackson to load it into a TextureComponent class:

@JacksonXmlRootElement(localName="package.TextureComponent")
public class TextureComponent {
    public int id;
    public int width;
    public int height;

    @Override
    public String toString() {
        return "TextureComponent [id=" + id + ", width=" + width + ", height=" + height + "]";
    }
}

(You can use getters and setters instead of public fields)

Code to load it:

public static void main(String[] args) {
    XmlMapper xmlMapper = new XmlMapper();
    // Configurations you might want to use:
    // xmlMapper.enable(SerializationFeature.INDENT_OUTPUT);
    // xmlMapper.disable(SerializationFeature.FAIL_ON_EMPTY_BEANS);
    // xmlMapper.enable(ToXmlGenerator.Feature.WRITE_XML_DECLARATION);
    try {
        TextureComponent obj = xmlMapper.reader(TextureComponent.class).readValue("<package.TextureComponent><id>1</id><width>100</width><height>100</height></package.TextureComponent>");
        System.out.println(obj);
    }
    catch (IOException e) {
        e.printStackTrace();
    }
}

Output:

TextureComponent [id=1, width=100, height=100]

If you're using Maven, here's the dependencies you need to use Jackson:

<dependency>
    <groupId>com.fasterxml.jackson.dataformat</groupId>
    <artifactId>jackson-dataformat-xml</artifactId>
    <version>2.3.3</version>
    <type>bundle</type>
</dependency>
<dependency>
    <groupId>com.fasterxml.jackson.core</groupId>
    <artifactId>jackson-core</artifactId>
    <version>2.3.3</version>
    <type>bundle</type>
</dependency>
<dependency>
    <groupId>com.fasterxml.jackson.core</groupId>
    <artifactId>jackson-annotations</artifactId>
    <version>2.3.3</version>
    <type>bundle</type>
</dependency>
<dependency>
    <groupId>com.fasterxml.jackson.jaxrs</groupId>
    <artifactId>jackson-jaxrs-xml-provider</artifactId>
    <version>2.3.3</version>
    <type>bundle</type>
</dependency>

I should add that Jackson also have support for other POJOs, Lists, Maps, Sets, and more or less every other data type you need.

The beauty of Jackson is that it also supports JSON (this is built-in inside Jackson itself), and also YAML and a whole lot of other formats if you use additional Jackson-plugin libraries.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.