HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Dealing with multiple types in an excel sheet

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
excelwithsheetdealingmultipletypes

Problem

I need to parse an excel sheet in order to produce an statistic, and some of the columns in the sheet can have different value types within the same column. For example:

A        B
String  String
String  String
Date    String


I cannot force the columns to have a consistent type because the input sheet belongs to one of our clients (the ones with the money), so I'm stuck with possible multiple types of values in the same column. To address this issue I made this class:

public final class MultiTypeValue {

private Map, Optional> values;

public static MultiTypeValue newInstance(Class... classes) {
    return new MultiTypeValue(classes);
}

private MultiTypeValue(Class... classes) {
    if (classes == null || classes.length == 0) {
        throw new IllegalArgumentException("The classes cannot be empty");
    }

    values = new ConcurrentHashMap<>();
    for (Class clazz : classes) {
        values.put(clazz, Optional.empty());
    }
}

public void put(Class clazz, Object value) {
    values.put(clazz, Optional.ofNullable(value));
}

@SuppressWarnings("unchecked")
public  T value(Class clazz) {
    return (T) values.get(clazz);
}

}


The idea was that for each type a column can have, to have an Optional associated to that type which may or may not have a value.

In terms of performance I imagine this is pretty bad, but I couldn't think another solution. Does anyone know a framework or a design pattern better than this solution?

Solution

A few nitpicks in general:

Your indentation is off

I'm not sure whether that's just a copy paste-error or actually intended, but it's convention to indent the class body by 4 Spaces.

public class MultiTypeValue {

    private Map //...


Eager initialization saves you some pain

Instead of initializing your values in the constructor, you might prefer to eagerly initialize the Map as a final field:

public class MultiTypeValue {

    private final Map, Optional> values = 
      new ConcurrentHashMap<>();
    // ...


Why do you disallow an empty map?

I don't think it's a wise choice to disallow an empty Map if you don't validate the class-object pairs you put in anyways. As such initializing the map with an empty Optional seems... unhelpful.

Either you stop requiring classes for construction or you make use of the classes by validating you're only updating a value of an allowed class instead of just silently taking it:

public void put(Class clazz, Object value) {
    if (values.containsKey(clazz)) {
       values.put(clazz, Optional.ofNullable(value));
    }
}


Use generics when putting a value

You should really do some more checking when you put a value. Consider the following:

public void  put(Class clazz, T value) {


this is where it gets really interesting, since you can just extract the relevant class from your argument:

public void  put(T value) {
    Class clazz = value.getClass();


I'm not quite clear on how your requiremnts / usage of this class is though, so this may just completely miss your usecase.

The factory method is unnecessary

As of now you're simply misusing your factory method as a proxy for your constructor. Usually you'd implement a factory method to allow for caching and instance control. Since you don't do either of that here, the factory method isn't necessary and you should just expose the constructor directly to simplify things

Missing documentation

One of the reasons I wasn't sure what exactly you intend this class for is that it's completely undocumented.

You explain nothing on this class and as an external developer on your project I'd be very confused by it I daresay. Then again I haven't seen it in usage context, but either way:

This seems like a core class for your functionality and it should be documented accordingly

Code Snippets

public class MultiTypeValue {

    private Map //...
public class MultiTypeValue {

    private final Map<Class<?>, Optional<?>> values = 
      new ConcurrentHashMap<>();
    // ...
public void put(Class<?> clazz, Object value) {
    if (values.containsKey(clazz)) {
       values.put(clazz, Optional.ofNullable(value));
    }
}
public void <T> put(Class<T> clazz, T value) {
public void <T> put(T value) {
    Class<T> clazz = value.getClass();

Context

StackExchange Code Review Q#109173, answer score: 5

Revisions (0)

No revisions yet.