Loading configuration properties from XML

Question

I would like a review of the transformation of the first method to the updated one below:

 private static ArrayList<String> getValuesfromXML(String key1, String key2) {
    ArrayList<String> valueArray = new ArrayList<String>();
    try {

        String scriptPath = getScriptPath("prop.xml");
        File file = new File(scriptPath);
        FileInputStream fileInput = new FileInputStream(file);
        Properties properties = new Properties();
        properties.loadFromXML(fileInput);
        fileInput.close();
        valueArray.add(properties.getProperty(key1));
        valueArray.add(properties.getProperty(key2));

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

    return valueArray;
}

It looks as if it's GC'd so there's no memory leaks. And is not using a finally on the filehandle really an issue?

    /**
     * read property values from the configuration file. Using standard Java Properties class.
     *
     * @param key1
     *            - Property key
     * @param key2
     *            - Property Key
     * @return - ArrayList values of properties based on keys
     */

    private static ArrayList<String> getPropertyValue (String key1, String key2){

        final ArrayList<String> valueArray = new ArrayList<>();
        String scriptPath = getScriptPath(propertiesFileName);

        try (FileInputStream fileInput = new FileInputStream(scriptPath)) {
            Properties properties = new Properties();
            properties.loadFromXML(fileInput);
            valueArray.add(properties.getProperty(key1));
            valueArray.add(properties.getProperty(key2));

        } catch (Exception  ex) {
            ex.printStackTrace();
        }

        return valueList;
    }

Welcome to CR! Please note we review your code; that top snippet written by someone else would have to be submitted by its author to be reviewable - it's there just as additional context. You might want to edit out the part asking to critique it, since that may be frowned upon. — Mathieu Guindon
– Mathieu Guindon, Commented Feb 28, 2017 at 2:21

rolfl · Accepted Answer · 2017-02-28 03:23:06Z

Your conversion is pretty good. I would make some suggestions but on the whole it would "pass" a code review - depending on the code that's calling this function (yes, that does make a difference). If I was to suggest changes they would be...

Interfaces over concrete classes.

Use interfaces instead of concrete types for return values and parameters. In your case, your function returns ArrayList<String>, but I would convert that to List<String>. This may not be possible without changing other code in your system, though. That would also imply changing the variable final ArrayList<String> valueArray to be final List<String> valueArray.

Error Handling

Your handling sucks.... but if you're locked in to the function signature because of other code, then you can't do much to change it, easily. Still, printing the stack trace is a very poor-man's solution.

Note that you can use a multi-exception catch now, and do:

} catch (IOException | FileNotFoundException e) {
    // ... handle e
}

Use `java.nio.file.*`

The newer NIO functions typically are more efficient, so use them when you can.

In your case, I would have:

Path script = Paths.get(getScriptPath(propertiesFileName));
try (InputStream input = new BufferedInputStream(Files.newInputStream(scritpt))) {
    // .....
} catch (.....) {
    // .....
}

Note that the input is an InputStream and not a FileInputStream ... use the highest form of the instance where you can.

Declare variables where they are used....

You declare the result variable valueArray outside the try-block. This is because of how you do the error handling and you may want to return it when it has an empty value. This is not the best solution though, I would have:

try (InputStream input = new BufferedInputStream(Files.newInputStream(scritpt))) {
    // .... get properties
    List<String> valueArray = new ArrayList<>();
    // .... populate it....
    return valueArray;
} catch (....) {
    // ... report the error
    e.printStackTrace();
    // ... return an empty array.
    return Collections.emptyList();
}

The finally....

You are using a finally on the file handle close... that's what the try-with-resources does for you.

Stack Exchange Network

Loading configuration properties from XML

1 Answer 1

Interfaces over concrete classes.

Error Handling

Use `java.nio.file.*`

Declare variables where they are used....

The finally....

You must log in to answer this question.

Hot Network Questions

Loading configuration properties from XML

1 Answer 1

Interfaces over concrete classes.

Error Handling

Use java.nio.file.*

Declare variables where they are used....

The finally....

You must log in to answer this question.

Related

Hot Network Questions

Use `java.nio.file.*`