2
\$\begingroup\$

I'm building an IntelliJ Plugin which will allow a person to select an XML file (and even a snippet of XML in the file, if they like) and, using a really brief GUI, generate classes / code in one of a selection of languages that will read the XML via native functions and generate code to do so on a regular basis.

The idea here is that a user should be able to generate a deserialization function that would not rely on any third-party libraries, but only on the native built-in features of the language and framework. Additionally, this would mean no reflection / introspection would be necessary: just read the XML into a fully strong-typed thing.

I'm going to post this as several "parts", because it's a large project. I'll do so incrementally: starting from the first component and moving towards the last.

The first step in the adventure is to create and use a Parser which can read our XML file into some nodes / objects. The idea here is that it only cares about the structure of the file: node names, keys, and value types, not the actual values. (So <node attr="test"> would say "this is a node named 'node', with an attribute named 'attr', that is a string.")

To start with, we should begin at the, well, beginning: reading the file.

For simplicity, I've built a Reading class that can take a document as a string and attempt to parse the XML out of it:

ArrayList<Node> nodes = Reading.readDocument(document);

This is a very simple interface, but underneath it starts doing a lot of work:

package Xml;

import javax.xml.stream.XMLEventReader;
import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.events.Attribute;
import javax.xml.stream.events.Characters;
import javax.xml.stream.events.StartElement;
import javax.xml.stream.events.XMLEvent;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.Iterator;

public class Reading {
    public static ArrayList<Node> readDocument(String document) throws XMLStreamException {
        XMLInputFactory factory = XMLInputFactory.newInstance();
        XMLEventReader reader = factory.createXMLEventReader(new StringReader(document));
        ArrayList<Node> nodes = new ArrayList<>();
        int indentation = 0;

        while (reader.hasNext()) {
            XMLEvent event = reader.nextEvent();
            if (event.isStartElement()) {
                StartElement startElement = event.asStartElement();
                Node node = new Node(startElement.getName().getLocalPart());

                Iterator attributes = startElement.getAttributes();
                while (attributes.hasNext()) {
                    Attribute attribute = (Attribute)attributes.next();
                    if (attribute.isSpecified()) {
                        node.add_attribute(attribute.getName().getLocalPart(), attribute.getValue());
                    }
                }

                ArrayList<Node> nodeAddition = getActiveNode(nodes, indentation);
                nodeAddition.add(node);
                indentation++;
            } else if (event.isCharacters()) {
                Characters characters = event.asCharacters();
                String data = characters.getData();

                ArrayList<Node> nodeAddition = getActiveNode(nodes, indentation - 1);
                nodeAddition.get(nodeAddition.size() - 1).set_value(data);
            } else if (event.isEndElement()) {
                indentation--;
            }
        }

        return nodes;
    }

    private static ArrayList<Node> getActiveNode(ArrayList<Node> nodes, int indentation) {
        ArrayList<Node> nodeAddition = nodes;

        while (indentation > 0) {
            indentation--;
            nodeAddition = nodeAddition.get(nodeAddition.size() - 1).get_nodes();
        }

        return nodeAddition;
    }
}

Here, we read the XML in and start processing it into our list of array nodes. We keep the nested node structure, and we have the following definition of a Node within our program:

package Xml;

import java.util.ArrayList;
import java.util.HashMap;

public class Node {
    private String _name;
    private String _value;
    private ArrayList<Node> _nodes;
    private HashMap<String, String> _attributes;

    public Node(String name) {
        _name = name;
        _nodes = new ArrayList<>();
        _attributes = new HashMap<>();
    }

    public String get_name() {
        return _name;
    }

    public String get_value() {
        return _value;
    }

    public void set_value(String _value) {
        this._value = _value;
    }

    public HashMap<String, String> get_attributes() {
        return _attributes;
    }

    public void add_attribute(String key, String value) {
        _attributes.put(key, value);
    }

    public ArrayList<Node> get_nodes() {
        return _nodes;
    }
}

Obviously we're reading the value in here, but we'll eliminate that later. That was the easy part.

Next, we need to parse the nodes into some type of "object", here we'll use our Parser to begin doing some real work:

Parser parser = new Parser();                
parser.set_integerRule(getIntegerRule(wrapper.getSelectedNumberRule()));
parser.set_nullDefault(wrapper.getUseStringForNull() ? Type.STRING : Type.NULL);
parser.set_dateFormat(wrapper.getDateFormat());         
parser.set_allNodesAreArrays(wrapper.getAllNodesAreArrays());
for (Tuple2<String, String> rule : wrapper.getBooleanRules()) {
    parser.add_booleanRule(new ParserBooleanRule(rule.getFirst(), rule.getSecond()));
}
ArrayList<CodeObject> codeObjects = parser.parse(nodes);

The Parser is much more complex, and has a lot more going on. To start: it has a selection of "rules" that determine how and why it parses things, particularly how to handle different "features" of our parser. We have a few major features:

  1. Number Rules: when the parser encounters a number type, how should it treat the type? We can use the minimal data-size, or we can assume the minimum is Int, Long, or Double.
  2. Use String for Null: when the parser encounters a null value, how should it proceed? If selected, the parser should intrinsically treat the lack of a value as a String.
  3. Date Format: how should the parser detect dates? If specified, the parser should use the specified format to detect date types.
  4. Boolean Rules: how should the parser detect true/false values? Three built-in rules exist for true/false respectively: Y/N, Yes/No, True/False.
  5. Result Language: this is less for the parser, and more for the generator. What is the destination output language?

So, with that said, we'll look at the Parser:

package Generation;

import Xml.Node;
import org.jetbrains.annotations.NotNull;

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.HashMap;

public class Parser {
    private String _dateFormat;
    private ParserIntegerRule _integerRule;
    private Type _nullDefault;
    private ArrayList<ParserBooleanRule> _booleanRules;
    private boolean _allNodesAreArrays;

    public Parser() {
        _integerRule = ParserIntegerRule.Minimal;
        _nullDefault = Type.NULL;
        _booleanRules = new ArrayList<>();
    }

    public String get_dateFormat() { return _dateFormat; }
    public void set_dateFormat(String _dateFormat) { this._dateFormat = _dateFormat; }
    public ParserIntegerRule get_integerRule() { return _integerRule; }
    public void set_integerRule(ParserIntegerRule _integerRule) { this._integerRule = _integerRule; }
    public Type get_nullDefault() { return _nullDefault; }
    public void set_nullDefault(Type _nullDefault) { this._nullDefault = _nullDefault; }
    public ArrayList<ParserBooleanRule> get_booleanRules() { return _booleanRules; }    
    public void add_booleanRule(ParserBooleanRule rule) { _booleanRules.add(rule); }
    public boolean is_allNodesAreArrays() { return _allNodesAreArrays; }
    public void set_allNodesAreArrays(boolean _allNodesAreArrays) { this._allNodesAreArrays = _allNodesAreArrays; }

    private Type getType(String value) {
        SimpleDateFormat format = new SimpleDateFormat(_dateFormat);
        format.setLenient(false);

        if (value == null || value.equals("")) {
            return _nullDefault;
        }

        if (_dateFormat != null) {
            try {
                format.parse(value);
                return Type.DATE;
            } catch (ParseException ignored) { }
        } 

        for (ParserBooleanRule rule : _booleanRules) {
            String tempValue = value.toLowerCase();
            if (tempValue.equals(rule.get_true()) || tempValue.equals(rule.get_false())) {
                return Type.BOOLEAN;
            }
        }

        if (_integerRule == ParserIntegerRule.Minimal) {
            try {
                //noinspection ResultOfMethodCallIgnored
                Byte.parseByte(value);
                return Type.INT8;
            } catch (NumberFormatException ignored) { }
        }

        if (_integerRule == ParserIntegerRule.Minimal) {
            try {
                //noinspection ResultOfMethodCallIgnored
                Short.parseShort(value);
                return Type.INT16;
            } catch (NumberFormatException ignored) { }
        }

        if (_integerRule == ParserIntegerRule.Minimal || _integerRule == ParserIntegerRule.AssumeInt) {
            try {
                Integer.parseInt(value);
                return Type.INT32;
            } catch (NumberFormatException ignored) { }
        }

        if (_integerRule == ParserIntegerRule.Minimal || _integerRule == ParserIntegerRule.AssumeInt || _integerRule == ParserIntegerRule.AssumeLong) {
            try {
                Long.parseLong(value);
                return Type.INT64;
            } catch (NumberFormatException ignored) { }
        }

        try {
            Double.parseDouble(value);
            return Type.DOUBLE;
        } catch (NumberFormatException ignored) { }

        return Type.STRING;
    }

    private boolean isNumeric(Type type) {
        switch (type) {
            case INT8:
                return true;
            case INT16:
                return true;
            case INT32:
                return true;
            case INT64:
                return true;
            case DOUBLE:
                return true;
            default:
                return false;
        }
    }

    private Type lowestCommonNumericType(Type type1, Type type2) {
        if (type1 == Type.INT8) {
            return type2;
        }
        if (type1 == Type.INT16) {
            if (type2 == Type.INT8) {
                return Type.INT16;
            }

            return type2;
        }
        if (type1 == Type.INT32) {
            if (type2 == Type.INT8 || type2 == Type.INT16) {
                return Type.INT32;
            }

            return type2;
        }
        if (type1 == Type.INT64) {
            if (type2 == Type.INT8 || type2 == Type.INT16 || type2 == Type.INT32) {
                return Type.INT64;
            }

            return type2;
        }

        return Type.DOUBLE;
    }

    private Type compareTypes(Type type1, Type type2) {
        if (type1 == Type.NULL) {
            return type2;
        }

        if (type2 == Type.NULL) {
            return type1;
        }

        if (type1 == Type.OBJECT || type2 == Type.OBJECT) {
            return Type.OBJECT;
        }

        if (_dateFormat != null
                && ((type1 == Type.DATE && isNumeric(type2))
                    || (isNumeric(type1) && type2 == Type.DATE))) {
            SimpleDateFormat format = new SimpleDateFormat(_dateFormat);
            format.setLenient(false);
            if (isNumeric(getType(format.format(Date.from(Instant.now()))))) {
                return type1 == Type.DATE ? type2 : type1;
            }
        }

        if (type1 == Type.BOOLEAN && type2 == Type.BOOLEAN) {
            return Type.BOOLEAN;
        }

        if (type1 == Type.DATE && type2 == Type.DATE) {
            return Type.DATE;
        }

        if (isNumeric(type1) && isNumeric(type2)) {
            return lowestCommonNumericType(type1, type2);
        }

        return Type.STRING;
    }

    private CodeObject objectWithName(Collection<CodeObject> codeObjects, String name) {
        for (CodeObject codeObject : codeObjects) {
            if (codeObject.get_name().equals(name)) {
                return codeObject;
            }
        }

        return null;
    }

    @NotNull
    private ArrayList<CodeObject> getMainObjects(@NotNull ArrayList<Node> nodes) {
        ArrayList<CodeObject> codeObjects = new ArrayList<>();

        for (Node node : nodes) {
            CodeObject object = new CodeObject(node.get_name());
            codeObjects.add(object);

            HashMap<String, String> attributes = node.get_attributes();
            for (String key : attributes.keySet()) {
                CodeObject attrCodeObject = new CodeObject(key, getType(attributes.get(key)));
                object.add_object(attrCodeObject);
                attrCodeObject.set_from(From.ATTRIBUTE);
            }

            ArrayList<Node> subNodes = node.get_nodes();
            if (subNodes.size() == 0) {
                object.set_type(getType(node.get_value()));
                object.set_from(attributes.size() > 0 ? From.NODE : From.VALUE);

                if (node.get_value() != null) {
                    Type myAttrType = getType(attributes.get("_Text"));
                    if (myAttrType == Type.NULL) {
                        myAttrType = Type.STRING;
                    }

                    CodeObject attrCodeObject = new CodeObject("_Text", myAttrType);
                    object.add_object(attrCodeObject);
                    attrCodeObject.set_from(From.RAW_VALUE);
                }
            } else {
                object.add_objects(getMainObjects(subNodes));
                object.set_from(From.NODE);
            }

            if (attributes.size() > 0 || subNodes.size() > 0) {
                object.set_type(Type.OBJECT);
            }
        }

        return codeObjects;
    }

    private void reduceObjects(@NotNull CodeObject existing, @NotNull CodeObject object) {
        existing.set_type(compareTypes(existing.get_type(), object.get_type()));
        existing.set_isArray(existing.is_isArray() || object.is_isArray());
        ArrayList<CodeObject> reducedObjects = reduceObjects(object.get_codeObjects());

        for (CodeObject reducedObject : reducedObjects) {
            CodeObject existingSubObject = objectWithName(existing.get_codeObjects(), reducedObject.get_name());
            if (existingSubObject == null) {
                existing.add_object(reducedObject);
                continue;
            }

            reduceObjects(existingSubObject, reducedObject);
        }
    }

    @NotNull
    private ArrayList<CodeObject> reduceObjects(@NotNull ArrayList<CodeObject> objects) {
        ArrayList<CodeObject> result = new ArrayList<>();

        for (CodeObject object : objects) {
            CodeObject existingObject = objectWithName(result, object.get_name());
            if (existingObject == null) {
                existingObject = new CodeObject(object.get_name(), object.get_type());
                existingObject.set_from(object.get_from());
                existingObject.set_isArray((_allNodesAreArrays && existingObject.get_from() == From.NODE) || object.is_isArray());
                existingObject.add_objects(reduceObjects(object.get_codeObjects()));
                result.add(existingObject);
            } else {
                existingObject.set_isArray(true);
                object.set_isArray(true);
                reduceObjects(existingObject, object);
            }
        }

        return result;
    }

    @NotNull
    public ArrayList<CodeObject> parse(@NotNull ArrayList<Node> nodes) {
        ArrayList<CodeObject> objects = getMainObjects(nodes);
        return reduceObjects(objects);
    }
}

This class is somewhat long, and has a few other definitions we need to include:

The CodeObject:

package Generation;

import java.util.ArrayList;
import java.util.Collection;

public class CodeObject {
    private String _name;
    private Type _type;
    private From _from;
    private boolean _isArray;
    private ArrayList<CodeObject> _codeObjects;

    public CodeObject(String name) {
        _name = name;
        _codeObjects = new ArrayList<>();
    }

    public CodeObject(String name, Type type) {
        this(name);
        _type = type;
    }

    public String get_name() {
        return _name;
    }

    public Type get_type() {
        return _type;
    }

    public void set_type(Type _type) {
        this._type = _type;
    }

    public ArrayList<CodeObject> get_codeObjects() {
        return _codeObjects;
    }

    public void add_object(CodeObject codeObject) {
        _codeObjects.add(codeObject);
    }

    public void add_objects(Collection<CodeObject> codeObject) {
        _codeObjects.addAll(codeObject);
    }

    public From get_from() {
        return _from;
    }

    public void set_from(From _from) {
        this._from = _from;
    }

    public boolean is_isArray() {
        return _isArray;
    }

    public void set_isArray(boolean _isArray) {
        this._isArray = _isArray;
    }
}

The Type:

package Generation;

public enum Type {
    NULL,
    STRING,
    INT8,
    INT16,
    INT32,
    INT64,
    DOUBLE,
    DATE,
    BOOLEAN,
    OBJECT
}

The From:

package Generation;

public enum From {
    ATTRIBUTE,
    NODE,
    VALUE,
    RAW_VALUE
}

The ParserBooleanRule:

package Generation;

public class ParserBooleanRule {
    private String _true;
    private String _false;

    public ParserBooleanRule(String _true, String _false) {
        this._true = _true;
        this._false = _false;
    }

    public String get_true() {
        return _true;
    }

    public String get_false() {
        return _false;
    }
}

And the ParserIntegerRule:

package Generation;

public enum ParserIntegerRule {
    Minimal,
    AssumeInt,
    AssumeLong,
    AssumeDouble
}

All these components, together, implement the logic of the parser. Read the Node objects into a CodeObject array.

Additionally, one should note all of the Type functions in the Parser: these help determine type-rules: given two types, which type should the result be? We use these to determine what we need to do with the data-types, and how an attribute of 5 and 14.2 and test should be compared, to get the lowest-common type: the one that will accurately represent all values in our input.

Finally, the generation also has three more components relevant to this parser:

private LinkedHashMap<String, ParserIntegerRule> _parserIntegerRules;

public GenerateAction() {
    super("Hello");

    _parserIntegerRules = new LinkedHashMap<>();
    _parserIntegerRules.put("Minimal", ParserIntegerRule.Minimal);
    _parserIntegerRules.put("Assume Int", ParserIntegerRule.AssumeInt);
    _parserIntegerRules.put("Assume Long", ParserIntegerRule.AssumeLong);
    _parserIntegerRules.put("Assume Double", ParserIntegerRule.AssumeDouble);
}    

@NotNull
private ParserIntegerRule getIntegerRule(String rule) {
    return _parserIntegerRules.getOrDefault(rule, ParserIntegerRule.Minimal);
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Program to Interfaces, not Implementations

The code base heavily depends on ArrayList. For example in Reading

ArrayList<Node> readDocument(String document) throws XMLStreamException {
   // ..
   ArrayList<Node> nodes = new ArrayList<>()
   // ..
       ArrayList<Node> nodeAddition = getActiveNode(nodes, indentation)
}

private static ArrayList<Node> getActiveNode(ArrayList<Node> nodes, int indentation) { /* .. */ }

The only methods that are used on that instances of ArrayList are add, get and size.

nodeAddition.add(node);
nodeAddition.get(nodeAddition.size() - 1)//..

Since only methods get used that are defined on the interface List, your code should rather depend on the interface than on the implementation:

List<Node> readDocument(String document) throws XMLStreamException {
    // ..
    List<Node> nodes = new ArrayList<>()
    // ..
        List<Node> nodeAddition = getActiveNode(nodes, indentation)
}

private static List<Node> getActiveNode(List<Node> nodes, int indentation) { /* .. */ }

With program to interfaces, not implementations you do not limit your self to a concrete implementation, because now it would be easier to switch from an ArrayList to a LinkedList because of the use of the common interface.


Accessors Are Evil

Public accessors indicate that the data and the behavior of a class are not kept together.

This is seen as a an indication of higher coupling and lower coherence.

Objects vs. Data Structures

The "objects" Value, CodeObject, ParserBooleanRule and maybe more make have use of getters and setters. The are not objects in terms of oop - rather they are data structures.

In Parser inside the method getType is the following if-statement

if (tempValue.equals(rule.get_true()) || tempValue.equals(rule.get_false())) {
   return Type.BOOLEAN;
}

We can see that the Parser uses rule (ParserBooleanRule) as a data structure whilst it uses the methods get_true and get_false to look if a value is a boolean. Much better would be, when ParserBooleanRule knows if a value is a boolean:

if (rule.fulfilledBy(value)) {
    return Type.BOOLEAN;
}

Don't Use Exceptions For Flow Control

if (_integerRule == ParserIntegerRule.Minimal) {
   try {
       //noinspection ResultOfMethodCallIgnored
       Byte.parseByte(value);
       return Type.INT8;
   } catch (NumberFormatException ignored) { }
}

if (_integerRule == ParserIntegerRule.Minimal) {
   try {
       //noinspection ResultOfMethodCallIgnored
       Short.parseShort(value);
       return Type.INT16;
   } catch (NumberFormatException ignored) { }
}

// ..

At the first time I thought that the second if-statement will never be reached because it has the same condition as the first if-statement. Than I saw that you actually use an try-catch as condition.

Instead of you could check first if a value is a number trough a regex, count the number of figures and parse it to the data type you aspect..


Feature Envy

A classic [code] smell is a method that seems more interested in a class other than the one it is in. The most common focus of the envy is the data.

I already wrote that you have a lot of getter and setter methods witch makes it easy do access the data of an object. The Parser reads the data for many of your objects and do some logic on it - this is a Feature Envy. The object itself should do the operation and the Parser should only ask for it (Tell don't ask)

The following loop is inside Parser exist to manipulate a CodeObject

for (CodeObject object : objects) {
   CodeObject existingObject = objectWithName(result, object.get_name());
   if (existingObject == null) {
       existingObject = new CodeObject(object.get_name(), object.get_type());
       existingObject.set_from(object.get_from());
       existingObject.set_isArray((_allNodesAreArrays && existingObject.get_from() == From.NODE) || object.is_isArray());
       existingObject.add_objects(reduceObjects(object.get_codeObjects()));
       result.add(existingObject);
   } else {
       existingObject.set_isArray(true);
       object.set_isArray(true);
       reduceObjects(existingObject, object);
   }
}

The Parser should request the change by an CodeObject or in this case by an CodeObjectFactory

When we look at the following method-names, we will see that they all include the class name on which they interact:

objectWithName, getMainObjects, reduceObjects, compareTypes and lowestCommonNumericType

They should go into the classes envied by the parser.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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