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.