2
\$\begingroup\$

The idea is that a user will dynamically edit simple key/value settings that will customize their instance of the application, e.g. the welcome banner text. I am trying to implement everything in a generic way, but the more I think about it the more I feel that I've over thought it since in order for one of these values to even be used, a developer will have to alter code in the back end to actually refer to it, maybe it doesn't need to be as generic.

My main concerns are 1. Usage of the values in the backend code by developers and 2. I was trying to think of a way to have the Setting.getValue() function return the data type of the value so no casting would be needed. The Setting object knows the object class (type) of the value and the value itself, it would be nice if I could get the usage to be Boolean showBanner = setting.getValue(); instead of Boolean showBanner = (Boolean) setting.getValue(); I don't like the consumer being responsible for casting.

POJO

package net.lab.beans;
public class Setting<T> implements Storable, NameableBean{

private String name;
private String description;
private String value;
private SettingDataType type;
private Long id;

public enum SettingDataType{
    BOOLEAN("boolean", Boolean.class), INTEGER("integer", Integer.class),
    STRING("string", String.class), LONG("long", Long.class);

    private String type;
    private Class clazz;

    SettingDataType(String type, Class clazz){
        this.type = type;
        this.clazz = clazz;
    }

    private String getType(){
        return this.type;
    }

    private Class getTypeClass(){
        return this.clazz;
    }

    /**
     * Returns the object representation of the data type for an Setting
     * @param type String representation of the simplified class name, defaults to String.class
     * @return SettingDataType
     */
    public static SettingDataType parse(String type){
        for(SettingDataType dt : SettingDataType.values()){
            if(dt.getType().equalsIgnoreCase(type)){
                return dt;
            }
        }
        return SettingDataType.STRING;  //Values are stored as VARCHAR
    }
}

public Setting(String name, String description, String value, SettingDataType type){
    this.name = name;
    this.value = value;
    this.type = type;
    this.description = description;
}

@Override
public String getName() {
    return name;
}

@Override
public Long getId(){
    return this.id;
}

public void setName(String name) {
    this.name = name;
}

public String getDescription(){
    return this.description;
}

public void setDescription(String description){
    this.description = description;
}

public void setValue(String value){
    this.value = value;
}

public T getValue(){
    T retVal = null;
    if(this.value != null){
        retVal = parseValue();
    }
    return retVal;
}

@SuppressWarnings("unchecked")
private T parseValue(){
    T val = (T) value;
    switch (type){
        case BOOLEAN:
            val = (T) Boolean.valueOf(value);
            break;
        case INTEGER:
            val =  (T) new Integer(value);
            break;
        case LONG:
            val = (T) new Long(value);
            break;
        default: //Values are already saved as strings
            break;
    }
    return val;

}

public void setType(String type){
    this.type = SettingDataType.parse(type);
}

public SettingDataType getType(){
    return this.type;
}

public void setId(Long id){
    this.id = id;
}

}

Simple Quick Test Class

package net.lab.beans;

import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class SettingTest {

/** boolean Test Vals **/
private Setting booleanSetting;
private Boolean expectedBoolean = true;
private String expectedBooleanDescription = "Test Description for primitive boolean";
private String expectedBooleanName = "primitiveBoolean";
private String testBooleanValue = "tRuE";

/** Null test Vals **/
private Setting nullSetting;


@Before
public void setup(){
    booleanSetting = new Setting(expectedBooleanName, expectedBooleanDescription, testBooleanValue, Setting.SettingDataType.BOOLEAN);
    nullSetting = new Setting(null, null, null, null);
}

@Test
public void testRetrieveBoolean(){
    Boolean testRetrievedValue = (Boolean) booleanSetting.getValue();

    Assert.assertEquals(expectedBoolean, testRetrievedValue);
    Assert.assertEquals(expectedBooleanDescription, booleanSetting.getDescription());
    Assert.assertEquals(expectedBooleanName, booleanSetting.getName());
}

@Test
public void NullTest(){
    Object testRetrievedValue = nullSetting.getValue();
    Assert.assertNull(testRetrievedValue);
}

@Test
public void testRetrieveString(){
    String expectedValue = "asdof3orroinwf~~~!1!!!....  ih";
    Setting stringSetting = new Setting("String Val", "Description", expectedValue, Setting.SettingDataType.STRING);

    Assert.assertEquals(expectedValue, stringSetting.getValue());
}

@Test(expected = NumberFormatException.class)
public void misMatchDataTypeException(){
    Setting misMatch = new Setting("Name", "Description", "12l", Setting.SettingDataType.INTEGER);
    Integer testRetrievedValue = (Integer) misMatch.getValue();
}
}

Usage:

After rubber-ducking this I'm concerned about how a developer will retrieve a specific value to use. I have a simple Service class that will retrieve the Setting from the database

....
//Example usage that has me worried with casting and potential NPE

@Inject
private SettingsSerivce settingsService;

@RequestMapping(value="/getBannerText", method = RequestMethod.GET)
public void getBannerText(){

    Setting bannerTextSetting = settingsService.getSetting(SettingEnum.BANNER_TEXT.getId());

    //I would like to use the `.getValue()` without casting if possible
    return (String) bannerTextSetting.getValue();
{

....

Then I'm thinking the developer will have to use a pre-created enum to specify the value they want, and then use the POJO's Setting.getValue() and cast that value to what it should be..... but that sounds risky.

Service Class (barebones right now)

@Component
public class SettingsService {

    private static final Logger LOGGER = Logger.getLogger(Setting.class);

    @Autowired
    private SettingDAO settingsDAO;

    //Originally was going to pre-load simple values into map on bean
    // but due to how the app is setup they cannot be pre-loaded
    public Setting getSetting(long id) {
        return SettingsDAO.read(id, UserSessionHolder.getCurrentLabId());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to CR! That stray closing brace means one of two things: either your indentation is broken, or you've had issues pasting your code into the question. I'd suggest you edit the post to reflect exactly what's in your IDE before a reviewer points it out ;-) remember, you can paste from the IDE, select the whole code block and hit Ctrl+K to automatically add the 4 leading spaces on all lines, that will format it as a code block in the post. \$\endgroup\$ – Mathieu Guindon Jan 17 '17 at 3:32
  • 1
    \$\begingroup\$ This feels like a lot of work to duplicate the Java type system. There's even a section in Joshua Bloch's Effective Java on implementing your own type systems. Basically it says "don't." It's hard to know how to improve this without your requirements; all we can do is speculate on what you might do instead. I think if you can find a way to just remove all the code above, it would be best. Otherwise, take a look at some Java based languages like Expression Language to see how they handle runtime dynamic typing. \$\endgroup\$ – markspace Dec 13 '17 at 19:57
1
\$\begingroup\$

What you try to achieve cannot be done using an enum for lack of generic possibilities. If you want to get rid of the casts and have compile-time checking, you'll have to implement a generic key class and use predefined constants of that class. These constants can use their own generic info to induce type-safety.

Example:

private static class MyKey<ValueType> {
    private final Class<ValueType> cl;
    private final String           key;

    private MyKey(Class<ValueType> cl, String key) {
        this.cl = cl;
        this.key = key;
    }

    private ValueType get(Map<String, Object> backingMap) {
        Object v = backingMap.get(key);
        if(v == null)
            return null;
        if(cl.isAssignableFrom(v.getClass()))
            return cl.cast(v);
        throw new RuntimeException("Wrong type in map!");
    }

    private void set(Map<String, Object> backingMap, ValueType val) {
        backingMap.put(key, val);
    }
}

// Constant definitions to be used by the client
public static final MyKey<String> NAME = new MyKey<>(String.class, "name");
public static final MyKey<Integer> AGE = new MyKey<>(Integer.class, "age");


// Client code
Map<String, Object> map = new HashMap<>();
NAME.set(map, "Just Me");
AGE.set(map, 123);

String theName = NAME.get(map);
int theAge = AGE.get(map);

.... you may consider wrapping the get/set this in additional methods, so that you get back to the usual syntax of someMap.put(someKey, someValue) instead of someKey.set(someMap, someValue).

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

Take a look at this: https://github.com/vanillasource/config/blob/master/config/src/main/java/com/vanillasource/config/Configuration.java

It is a generic configuration library that does (apparently) what you are trying to achieve. It works on the same principle as the other commenter indicated. You have to pass a "Key" to the "get()" method to get a typed value.

Retrieving the "banner text" for example would look like this:

private static final Key<String> BANNER_KEY = new StringKey("BannerText", "Default Banner");
private static final Key<Boolean> SHOW_BANNER_KEY = new BooleanKey("ShowBanner", false);

private String banner = configuration.get(BANNER_KEY);
private Boolean showBanner = configuration.get(SHOW_BANNER_KEY);

And the Configuration is an interface, so you can plug in the backend (file, database, etc.) you need.

Here is an article detailing the design decisions made: https://dzone.com/articles/requirements-for-a-configuration-library

\$\endgroup\$
  • \$\begingroup\$ This is an alternative implementation, not a "code review" of the posted code. \$\endgroup\$ – janos Jan 17 '17 at 11:54
  • 1
    \$\begingroup\$ It addresses both concerns, and it's close enough for inspiration how to change the code above. \$\endgroup\$ – Robert Bräutigam Jan 17 '17 at 12:09
  • \$\begingroup\$ I'm going to agree with Robert here. The OP's approach seems fundamentally broken. Any review would be remiss not to point that out. Reusing existing libraries and simplifying the code from a user perspective are good things. @janos \$\endgroup\$ – markspace Dec 13 '17 at 20:03

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.