patternjavaMinor
Dealing with multiple types in an excel sheet
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:
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:
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?
A B
String String
String String
Date StringI 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.
Eager initialization saves you some pain
Instead of initializing your
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:
Use generics when putting a value
You should really do some more checking when you put a value. Consider the following:
this is where it gets really interesting, since you can just extract the relevant class from your argument:
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
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.