How to refactor this code to get a source from property files?

I want to get the source of these files, but I don't know if I'm efficient.

I get the source of the file, put the source in a list, so I put this in a hashmap with the key is the version of file and the values are the list.

private Map> loadXmlNodes() {

    Map> mapNodes = new HashMap<>();
    List nodes = XmlUtil.loadNodes1_10();

    mapNodes.put("1.10", nodes);
    nodes = XmlUtil.loadNodes2_00();
    mapNodes.put("2.00", nodes);

    return mapNodes;
}

public static List loadNodes1_10() {

    List node = new ArrayList<>();

    try {

        InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes1.10.properties");
        Properties properties = new Properties();
        properties.load(inputStream);

        for (Object value : properties.keySet()) {
            node.add((String) value);
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return node;
}

public static List loadNodes2_00() {

    List node = new ArrayList<>();

    try {

        InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes2.00.properties");
        Properties properties = new Properties();
        properties.load(inputStream);

        for (Object value : properties.keySet()) {
            node.add((String) value);
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return node;
}

There are a couple of inefficient things you do, and there's some better-practice items too.

First, let's take one of the methods:

public static List loadNodes2_00() {

    List node = new ArrayList<>();

    try {

        InputStream inputStream = XmlUtil.class.getClassLoader().getResourceAsStream("nodes2.00.properties");
        Properties properties = new Properties();
        properties.load(inputStream);

        for (Object value : properties.keySet()) {
            node.add((String) value);
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return node;
}

node is not a great name for something that is a list. It is typical in computer programs for node to point to just a particular element in a list, stack, or whatever, not the whole thing.

Something like result, or names would be better.

Then, you are not closing the InputStream. This is bad practice, and is a bad habit to be in. Java7 allows the try-with-resources statement, which is ideal for this code.

Now, there is the less-commonly-used method stringPropertyNames() available on Properties. It is useful here, instead of iterating on the keyValues().

Why are the loadNodes1_10() and loadNodes2_00() methods public? Do they need to be? They should be private to the class if they are not used.

Constant values embedded inside code are often called 'Magic values'. In this case, the values are Strings. You should declare a constant for your resource names....

Putting it all together, how about this method:

private static final String RESOURCE_NODES_2_00 = "nodes2.00.properties";
private static List loadNodes2_00() {

    List names = new ArrayList<>();
    try (InputStream inputStream = XMLUtil.class.getClassLoader()
                .getResourceAsStream(RESOURCE_NODES_2_00)) {

        Properties properties = new Properties();
        properties.load(inputStream);
        names.addAll(properties.stringPropertyNames());

    } catch (IOException e) {
        e.printStackTrace();
    }

    return names;
}

This is neater, and more efficient, and it closes all the streams correctly.... but... apart from the method name, and the file name, your two methods are identical. What you should do is generalize the method:

private static List loadNodes(String resourcename) {

    List names = new ArrayList<>();
    try (InputStream inputStream = XMLUtil.class.getClassLoader()
                .getResourceAsStream(resourcename)) {

        Properties properties = new Properties();
        properties.load(inputStream);
        names.addAll(properties.stringPropertyNames());

    } catch (IOException e) {
        e.printStackTrace();
    }

    return names;
}

Now, in your calling method, you can change your code to:

private Map> loadXmlNodes() {

    Map> mapNodes = new HashMap<>();
    mapNodes.put("1.10", loadNodes(RESOURCE_NODES_1_10);
    mapNodes.put("2.00", loadNodes(RESOURCE_NODES_2_00);

    return mapNodes;
}

This should simplify the code a whole lot.

If you need the two methods loadNodes1_10() and loadNodes2_00() to be public, I would make them simply:

public static List loadNodes1_10() {
    return loadNodes(RESOURCE_NODES_1_10);
}

All the best, and enjoy.

EDIT:

If you want to return a Map> instead, it will look something like:

private static Map loadNodes(String resourcename) {

    Map names = new HashMap<>();
    try (InputStream inputStream = XMLUtil.class.getClassLoader()
                .getResourceAsStream(resourcename)) {

        Properties properties = new Properties();
        properties.load(inputStream);
        for (String key : properties.stringPropertyNames()) {
            names.put(key, properties.getProperty(key));
        }

    } catch (IOException e) {
        e.printStackTrace();
    }

    return names;
}

and then you would have to adjust the other methods that call this respectively.

StackExchange Code Review Q#40347, answer score: 8