ParameterValue.toXYZ() returns Object instead of primitive got numeric, boolean, chart

[decebals]

My goal is to simplify the parameter value read process for null or empty values.

Before this PR:

// the correct approach for test null or empty value 
ParameterValue idParam = routeContext.getParameter("id");
if (idParam.isNull() || idParam.isEmpty()) {
    // no id parameter
} else {
    long id = idParam.toLong();   
    // do my job
}

// the short version
```java
long id = routeContext.getParameter("id").toLong();
if (id > 0) {
    // do my job
}

The problems with last snippet are:

• I cannot know if the request parameter is null or is 0 (or other default value)
• I am in trouble if the request parameter is empty (I am encountered some situations) because a "NumberFormatException" is thrown; this situation can be avoided if we change isNull() from check if (isNull()) { return defaultValue; } with a more comprehensive isNullOrEmpty()

With current PR we can write:

Long id = routeContext.getParameter("id").toLong();
// id is null if the parameter value is null or empty
if (id != null) {
    // do my job
}

For my tests this PR does't not affect existing applications built with Pippo.

What do you think, brings value or not this PR?

[gitblit]

I think this will break some (all?) existing apps that use controllers with primitive types.

public void getItems(int page, int size, boolean includeMetadata) {
}

In the above example we'll get ClassCastExceptions because null can't be cast to a primitive. I'd rather we think more about how to improve the API to address your need without breaking the above use-case.

[decebals]

I have the same problem in PippoSettings. I don't like (for example in UndertowServer) to use something like:

if (getSettings().getWorkerThreads() > 0) {
    builder.setWorkerThreads(getSettings().getWorkerThreads());
}

I prefer this construction:

if (getSettings().getWorkerThreads() != null) {
    // I am sure that "workerThreads" doesn't exist in settings  
    ...
}

It's a little tricky to assign a default value for a null (missing) parameter/setting. From my point of view the code is more clean with object (Integer, Boolean) instead of primitives.

I think this will break some (all?) existing apps that use controllers with primitive types.

Sure, I see this thing and it's a problem for controllers. I don't know is it's not possible to make a compromise for controllers (the idea is to use toXYZ(name, defaultValue) to be sure that I don't have a null value).

[gitblit]

I'd really prefer not to lose the simplicity of primitive values.

Your proposed syntax:

Long id = routeContext.getParameter("id").toLong();
// id is null if the parameter value is null or empty
if (id != null) {
    // do my job
}

My syntax counter-proposal:

Long id = null;
if (routeContext.hasParameter("id")) {  // empty value is acceptable
    id = routeContext.getParameter("id").toLong(); // primitive auto-boxed to object
}
// OR
if (routeContext.hasParameterValue("id")) {  // empty value is not acceptable
    id = routeContext.getParameter("id").toLong(); // primitive auto-boxed to object
}

if (id != null) {
    // do my job
}

Looks like we need to fixup things like toLong(long defaultValue) to gracefully handle isEmpty instead of just isNull while still defaulting to 0.

From my point of view the code is more clean with object (Integer, Boolean) instead of primitives.

Interesting. I have the exact opposite opinion. When reviewing code a deeper understanding of runtime behavior is required when you use the object peers of primitives and you introduce more surface area for NPEs. Switching to objects also adds value sanity checks in addition to normal range checks:

if (getSettings().getWorkerThreads() != null) {
    // I am sure that "workerThreads" doesn't exist in settings  
    if (getSettings().getWorkerThreads() > 0) {
        // you can't assign 0 (or fewer) worker threads
        ...
    }
}