4
\$\begingroup\$

I've decided to make a wrapper class for the java.util.Properties class since, in its current state, it only allows for storing and reading String values.

Is there any way I can improve the following code?

public final class Prefs {

    private static final Properties properties = new Properties();
    private static OutputStream out;
    public static final File test = new File("settings.properties");

    public static void main(String[] args) throws IOException {
        try {
            set("hello", "5");

            int hello = get("hello", 1337);
            System.out.println(hello == 5);

            set("hello", "34");
            hello = get("hello", 1337);
            System.out.println(hello == 34);

            remove("hello");
            hello = get("hello", 1337);
            System.out.println(hello == 1337);
        } finally {
            save();
        }
    }

    static {
        try {
            if (!test.exists()) {
                test.createNewFile();
            }
            properties.load(new FileInputStream(test));
            out = new FileOutputStream(test);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public static <T> T get(String key, T defaultValue) {
        if (!properties.containsKey(key)) {
            return defaultValue;
        }
        String value = properties.getProperty(key);
        if (defaultValue instanceof Long) {
            return (T) new Long(value);
        } else if (defaultValue instanceof Integer) {
            return (T) new Integer(value);
        } else if (defaultValue instanceof Short) {
            return (T) new Short(value);
        } else if (defaultValue instanceof Byte) {
            return (T) new Byte(value);
        } else if (defaultValue instanceof Double) {
            return (T) new Double(value);
        } else if (defaultValue instanceof Float) {
            return (T) new Float(value);
        } else if (defaultValue instanceof Boolean) {
            return (T) new Boolean(value);
        }
        return (T) value;
    }

    public static void set(String key, String value) {
        properties.setProperty(key, "" + value);
    }

    public static void remove(String key) {
        properties.remove(key);
    }

    private static final ReentrantLock lock = new ReentrantLock();

    public static void save() {
        lock.lock();
        try {
            properties.store(out, "");
            out.flush();
        } catch (Throwable e) {
            e.printStackTrace();
        } finally {
            lock.unlock();
        }
    }

}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$
  1. Is the try-finally code block really required in your main() method?

  2. Your get() fails if the default value is not any of those primitive wrapper classes. What if I want a BigDecimal instance? "1.0" can't be casted to that.

  3. On a related note, you are relying on auto-boxing, which may lead to unexpected consequences:

    // code compiles, but throws NullPointerException here
    int npe = getValue("invalid-key", (Integer) null);
    
  4. Maybe this is just a quick prototyping, but if you do decide to make this a full-fledged library class, you should not be having the main() method, the static initialization block, or your static fields. The fields should become class fields where Prefs needs to be instantiated. Testing should go into its own class.

  5. On another related note, you don't necessarily need to create temporary files for testing. There is a Properties.load(Reader) method that will work with a StringReader, which can be used in turn to wrap your test Strings/lines.

  6. I don't think there's a benefit to instantiating a FileOutputStream object immediately after you have load()-ed the file. Things may happen between the loading and storing of properties, so regardless of whether you have this object or not, the storing process may still fail afterwards. Also, it seems like you do not need to explicitly flush() after calling store() as that is already done according to the Javadoc:

    After the entries have been written, the output stream is flushed. The output stream remains open after this method returns.

\$\endgroup\$
0
\$\begingroup\$

I would prefer primitive getters:


    public long get(String key, long defaultValue) {
        if (!p.containsKey(key)) {
            return defaultValue;
        }
        String val = p.getProperty(key);
        try {
            return Long.parseLong(val);
        } catch (NumberFormatException e) {
            throw new RuntimeException("Failed to read property " + key + ". Value is not a long.", e);
        }
    }

````
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! Please include a proper explanation for your preference, so others can learn from it. Also please note that the question is from 2015. \$\endgroup\$
    – AlexV
    May 4, 2019 at 12:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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