Non-testable class wrapping Java Properties with basic logic

Question

I have written a simple java.util.Properties wrapping class. Unfortunately, in my opinion, it isn't testable at the moment and I don't have any idea how to improve it. I could use PowerMockito to change properties after loading, or add a setter for them, or inject some class that provides properties, but I don't like any of that. I would like to avoid exposing properties by making it public or by getter/setter.

My application details:

• Spring boot
• my class is injected by @Bean mechanic
• CONF_FILE_NAME contains key=value formatted properies with names same as in Keys subclass.

I would like to unit-test logic in this class (both public method) without relying on values in file.

PropertiesManager.java:

package pl.propertiesdemo;

import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Properties;

public class PropertiesManager 
{
    private final String secretToken = "SECRET";

    public final class Keys 
    {
        public final static String KEY1 = "KEY1";
        public final static String KEY2 = "KEY2";
    }

    private final String CONF_FILE_NAME = "conf_file.conf";

    private Properties properties;

    public PropertiesManager() throws IOException
    {
        properties = new Properties();
        InputStream in = new FileInputStream(CONF_FILE_NAME);
        properties.load(in);
    }

    public String getProperty(String name)
    {
        String property = properties.getProperty(name);
        if (property == null) {
            System.out.println("Property " + name + " not found in properties file");
        }
        return property;
    }

    public HashMap<String, String> getAllPublicProperties()
    {
        HashMap<String, String> map = new HashMap<>();

        properties.keySet().stream().map(f -> (String)f).filter(f -> !f.contains(secretToken)).forEach(f -> map.put(f, properties.getProperty(f)));

        return map;
    }
}

PropertiesDemo.java: (main method and usage of both public methods)

package pl.propertiesdemo;

import java.io.IOException;

public class PropertiesDemo 
{
    public static void main(String[] args) throws IOException 
    {
        PropertiesManager manager = new PropertiesManager();
        System.out.println(manager.getProperty(PropertiesManager.Keys.KEY1));
        System.out.println(manager.getAllPublicProperties());
    }
}

conf_file_conf: (file loaded by PropertyManager)

KEY1=value_of_key1
KEY2=value_of_key2

PropertiesManager.java: (JUnit test I have so far)

package pl.propertiesdemo;

import java.io.IOException;
import java.util.HashMap;

import junit.framework.Test;
import junit.framework.TestCase;
import junit.framework.TestSuite;

public class PropertiesManagerTest extends TestCase 
{
    public PropertiesManagerTest(String testName)
    {
        super(testName);
    }

    public static Test suite()
    {
        return new TestSuite(PropertiesManagerTest.class);
    }

    private PropertiesManager getPropertiesManager()
    {
        PropertiesManager prop = null;
        try {
            prop = new PropertiesManager();
        } catch (IOException ioe) {
            fail();
        }
        return prop;
    }

    public void testGetProperty() 
    {
        PropertiesManager prop = getPropertiesManager();
        String value = prop.getProperty(PropertiesManager.Keys.KEY1);
        assertEquals("value_of_key1", value);
    }

    public void testGetAllProperties() 
    {
        PropertiesManager prop = getPropertiesManager();
        HashMap<String,String> values = prop.getAllPublicProperties();

        assertTrue(values.size() > 0);
        assertEquals(0, values.keySet().stream().filter(k -> k.contains("SECRET")).count());
    }
}

I don't like my tests, because right now they are dependant of value in real configuration file.

Answer 1

Your class is non-testable mainly because :

1. it reads datas from disk/network
2. it doesn't use any injection

Firstly, please note that Spring has some utility to read property file and fills java beans automaticly. You should check this if you don't want to reinvent the wheel. ;) Also, Properties is a fairly old school class in Java and isn't used that much recently.

If you do want to have your own implementation, the let's see what we can do line-by-line, shall we ?

private final String secretToken = "SECRET";

Either you plan on making this variable (and then it should be injected in the constructor) or this is a constant (so it should be private static final).

public final class Keys 
{
    public final static String KEY1 = "KEY1";
    public final static String KEY2 = "KEY2";
}

I don't really get what you were trying to accomplish here. Why didn't you put those constants in the main classes ? I'd remove this code altogether IMHO.

private Properties properties;

This is set in the constructor so this is a good candidate for final ;)

public PropertiesManager() throws IOException
{
    properties = new Properties();
    InputStream in = new FileInputStream(CONF_FILE_NAME);
    properties.load(in);
}

Maybe some people will disagree but I'm no big fan of constructor that do I/O.
Also this is non-configurable : what if you want to change the file name ? what if you want to read not from a file but from a string or from the console ?
You should read about dependency injection if you've never heard of it before.
For a quick introduction, it's a principle that some codes delegates the configuration of the object to the calling code allowing for more and easier code reuse.

The InputStream is not closed which may lead to ressource-leak. See https://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html#close() ;)

IMO I'd have a constructor that takes a Properties object. However, this may pose others problems (such as the Properties object still being accessed by another object and be modified directly).
So, I'd consider using a static factory method that takes a Reader and returns the PropertiesManager. The calling code will be the one responsible to close the stream. This allows you to inject any time of Reader making your code more reusable.

public String getProperty(String name)
{
    //...

This is fine. As a side-note : you shouldn't use System.out.println for log in production code.

public HashMap<String, String> getAllPublicProperties()

Your return type shouldn't be HashMap ; when possible you should code against the interface that most suits your need, in this case : Map.

HashMap<String, String> map = new HashMap<>();

properties.keySet().stream().map(f -> (String)f).filter(f -> !f.contains(secretToken)).forEach(f -> map.put(f, properties.getProperty(f)));

This code may be dangerous as it is not thread-safe due to the forEach that modify a non thread-safe object. From a FP pov, you should try to delegate to the collect method (check https://docs.oracle.com/javase/9/docs/api/java/util/stream/Collectors.html).

This leads us to the following code :

package pl.propertiesdemo;

import static java.util.stream.Collectors.toMap;

import java.io.IOException;
import java.io.Reader;
import java.util.Map;
import java.util.Properties;
import java.util.function.Function;

public class PropertiesManager {
    public static final String DEFAULT_CONF_FILE_NAME = "conf_file.conf";

    private static final String SECRET_TOKEN = "SECRET";

    private final Properties properties;

    public static PropertiesManager createFrom(final Reader r) throws IOException {
        var properties = new Properties();
        properties.load(r);
        return new PropertiesManager(properties);
    }

    protected PropertiesManager(final Properties properties) {
        this.properties = properties;
    }

    public String getProperty(final String name) {
        String property = properties.getProperty(name);
        if (property == null) {
            System.out.println("Property " + name + " not found in properties file");
        }
        return property;
    }

    public Map<String, String> getAllPublicProperties() {
        return properties.keySet()
                .stream()
                .map(f -> (String) f)
                .filter(f -> !f.contains(SECRET_TOKEN))
                .collect(toMap(Function.identity(), f -> properties.getProperty(f)));
    }
}

Now that your object don't read from disk and can be easily injected, the object instantiation in the test class may look like this :

PropertiesManager prop = null;
try {
    prop = PropertiesManager.createFrom(new StringReader("someKey=someValue"));
} catch (IOException ioe) {
    fail();
}