I've been taking a look at this issue, and Robert's already upgraded them both to Java 5 in terms of just using generics, but I wanted to see if there were any other improvements that could be made. ExecutionContext is pretty straight forward, there's a little less casting necessary because of generics (which I cleaned up), but only because of the key. JobParameters is a little more interesting. The one issue I've had with this class since I created it was that it's not really a shared keyspace. I can have two keys with the same name that exist as parameters as long as they're of different types, which is somewhat counterintuitive. However, there are some fairly nasty problems in fixing this. ExecutionContext is easy because it can handle any type, but will allow strong typing to certain types. JobParameters only supports String, Long, Double, and Date.

My first crack at fixing this was the following:

public class JobParametersPrototype {

private final Map<String, ? extends Object> parameters;


public JobParametersPrototype(Map<String, Object> parameters) {

for(Entry<String, Object> entry : parameters.entrySet()){
Object parameter = entry.getValue();
String key = entry.getKey();
Class<?> type = parameter.getClass();
if (!type.isInstance(String.class) || !type.isInstance(Long.class) ||
!type.isInstance(Double.class) || !type.isInstance(Date.class)) {
throw new ClassCastException("Value for key=[" + key + "] is not of type: [ String, Long, Double, or Date], it is ["
+ (parameter == null ? null : "(" + type + ")" + parameter) + "]");
}
}

this.parameters = new LinkedHashMap<String, Object>(parameters);
}

public long getLong(String key){
return ((Long)parameters.get(key)).longValue();
}

public String getString(String key){
return parameters.get(key).toString();
}

public Double getDouble(String key){
return ((Double)parameters.get(key)).doubleValue();
}

public Date getDate(String key){
return (Date)parameters.get(key);
}

public Map<String, ? extends Object> getParameters(){
return new LinkedHashMap<String, Object>(parameters);
}

}

The above is missing some code, because Dates need to be copied, etc, but it works. Then I got to thinking about it, and there's sort of a domain concept here of an individual JobParameter that can only be one of four types, and is immutable:

public class TestParameters {

private final Map<String,JobParameter> parameters;

public TestParameters(Map<String,JobParameter> parameters) {
this.parameters = parameters;
}

public long getLong(String key){
return ((Long)parameters.get(key).getValue()).longValue();
}

public String getString(String key){
return parameters.get(key).toString();
}

public Double getDouble(String key){
return ((Double)parameters.get(key).getValue()).doubleValue();
}

public Date getDate(String key){
return (Date)parameters.get(key).getValue();
}

public Map<String, JobParameter> getParameters(){
return new LinkedHashMap<String, JobParameter>(parameters);
}

private class JobParameter{

private final Object parameter;

public JobParameter(Object parameter) {

Class<?> type = parameter.getClass();
if (!type.isInstance(String.class) || !type.isInstance(Long.class) ||
!type.isInstance(Double.class) || !type.isInstance(Date.class)) {
throw new ClassCastException("Parameter is not of type: [ String, Long, Double, or Date], it is ["
+ (parameter == null ? null : "(" + type + ")" + parameter) + "]");
}

if(type.isInstance(Date.class)){
this.parameter = new Date(((Date)parameter).getTime());
}
else{
this.parameter = parameter;
}
}

public Object getValue(){

if(parameter.getClass().isInstance(Date.class)){
return new Date(((Date)parameter).getTime());
}
else{
return parameter;
}
}
}
}

Obviously, JobParameter wouldn't be an inner class, I put it there out of laziness. However, it handles the type checking at creation, so the JobParameters can garuntee that a JobParameter is one of the four types...it also handles copying of Dates.

There's one final problem, how the values are persisted:

private void insertJobParameters(Long jobId, JobParameters jobParameters) {

for (Entry<String, String> entry : jobParameters.getStringParameters().entrySet()) {
insertParameter(jobId, ParameterType.STRING, entry.getKey().toString(), entry.getValue());
}

for (Entry<String, Long> entry : jobParameters.getLongParameters().entrySet()) {
insertParameter(jobId, ParameterType.LONG, entry.getKey().toString(), entry.getValue());
}

for (Entry<String, Double> entry : jobParameters.getDoubleParameters().entrySet()) {
insertParameter(jobId, ParameterType.DOUBLE, entry.getKey().toString(), entry.getValue());
}

for (Entry<String, Date> entry : jobParameters.getDateParameters().entrySet()) {
insertParameter(jobId, ParameterType.DATE, entry.getKey().toString(), entry.getValue());
}

}

There's no way to get a separate map of each type, however, the dao could easily be changed to go through a larger list and check for typing on each object, there could perhaps even be a use for the ParamterType within the JobParameter itself that could make things easier.

Forgot how unreadable the code would be in a comment, so I have attached the two prototype classes. The other code is part of the JdbcJobInstanceDao implementation.

I have significantly refined the solution that was attached, I'm not sure how I missed it initially, but JobParameter now has 4 different constructors, one for each type supported, which is much cleaner. I also moved the ParamaterType enumeration from the JdbcInstanceDao to the new JobParameter class (upgrading it to Java 5 as well) JobParameters now has a single map : Map<String,JobParameter>, which seems more natural to me. I went ahead and committed the change and am marking this issue as complete.

(Note: I also updated BatchStatus to java 5 as well)

I'm closing this issue since it was already reviewed by everyone during the meeting last week.