6
\$\begingroup\$

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<String, List<String>> loadXmlNodes() {

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

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

    return mapNodes;
}

public static List<String> loadNodes1_10() {

    List<String> 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<String> loadNodes2_00() {

    List<String> 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;
}
\$\endgroup\$

3 Answers 3

8
\$\begingroup\$

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<String> loadNodes2_00() {

    List<String> 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<String> loadNodes2_00() {

    List<String> 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<String> loadNodes(String resourcename) {

    List<String> 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<String, List<String>> loadXmlNodes() {

    Map<String, List<String>> 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<String> loadNodes1_10() {
    return loadNodes(RESOURCE_NODES_1_10);
}

All the best, and enjoy.


EDIT:

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

private static Map<String,String> loadNodes(String resourcename) {

    Map<String,String> 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.

\$\endgroup\$
5
  • \$\begingroup\$ I´ve changed my collection to a hashmap, is there any way to put the register like in list? \$\endgroup\$ Jan 30, 2014 at 16:09
  • \$\begingroup\$ @DiegoMacario not sure what you are asking... what's a register? \$\endgroup\$
    – rolfl
    Jan 30, 2014 at 16:12
  • \$\begingroup\$ I want to put the pair of properties files like this...map<String, Map<String, String>> the key of the map I´m going to put the version...and in the value, I´m going to put properties source file. \$\endgroup\$ Jan 30, 2014 at 16:14
  • 1
    \$\begingroup\$ @DiegoMacario See the edit... \$\endgroup\$
    – rolfl
    Jan 30, 2014 at 16:18
  • \$\begingroup\$ great, I read about try-with resources \$\endgroup\$ Jan 30, 2014 at 19:54
4
\$\begingroup\$
  1. Instead of Map<String, List<String>> you could use Guava's Multimap (doc, javadoc) which was designed exactly for that.

  2. Are you sure that in case of an IOError your clients want an empty list? If that's some optional data it can be fine but it could be possible that instead of this you should propagate the exception to the caller and signal the error to avoid processing invalid data.

  3. See also:

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I don´t know Guava´s project...I´m going to see \$\endgroup\$ Jan 29, 2014 at 10:42
  • 2
    \$\begingroup\$ The advice in itself is good, but this feels more like a "quick random note" (i.e. comment) than an answer. \$\endgroup\$ Jan 29, 2014 at 17:27
  • 1
    \$\begingroup\$ @SimonAndréForsberg thanks for your comment, but it can help me. I´m gonna read. \$\endgroup\$ Jan 30, 2014 at 9:31
0
\$\begingroup\$

Iterating over the Properties.keySet() should give you keys, not values. Therefore, these loops are either buggy or misleading:

for (Object value : properties.keySet()) {
    node.add((String) value);
}
\$\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.