Java 8 Functional VS Imperative method

Question

I have created a method to dynamically build rest URI based on Bean properties, initially it was imperative then I have refactored it to functional style, it's my first time doing functional programming. both imperative and functional are working as expected, but I am not happy by the functional readability, functional seams an over kill for this method or it could be because i am still a novice functional programmer!

How would you refactor this method to more clean functional way?

Or would you keep it Imperative?

import java.beans.PropertyDescriptor;
import java.lang.reflect.InvocationTargetException;
import java.text.SimpleDateFormat;
import java.util.Arrays;
import java.util.Date;
import java.util.List;
import java.util.function.BiConsumer;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.lang.reflect.Method;

import org.springframework.beans.BeanUtils;
import org.springframework.format.annotation.DateTimeFormat;
import org.springframework.web.util.UriComponentsBuilder;

public String functionalBuildRestUri() throws Exception {

    final UriComponentsBuilder uriBuilder = UriComponentsBuilder.newInstance().scheme("https")
            .host("foo.com").path("/offers");
    //here is the functional 
    List<PropertyDescriptor> propDescList = Arrays.asList(BeanUtils.getPropertyDescriptors(getClass()));

    //this part is readable and precis, but to enable it had to add 4 methods 
    propDescList.stream().filter(notClassProp())
                         .filter(notNullPropValue())
                         .collect(Collectors.toMap(PropertyDescriptor::getName, propValue()))//conversion to map doesn't feel good to me how can I avoid it?
                         .forEach(buildRestParam(uriBuilder));

    return uriBuilder.build().toUriString();
}


public String imperativeBuildRestUri() throws Exception {
     final UriComponentsBuilder uriBuilder = UriComponentsBuilder.newInstance().scheme("https")
                .host("foo.com").path("/offers");



    PropertyDescriptor[] propDescArray = BeanUtils.getPropertyDescriptors(getClass());
    for (PropertyDescriptor propDesc : propDescArray) {

        String propName = propDesc.getName();
        if (!propName.equals("class")) {
            Method getPropMethod = propDesc.getReadMethod();
            Object propValue = getPropMethod.invoke(this);
            if (propValue != null) {
                if(propValue instanceof Date){
                    String dateStr = new SimpleDateFormat(DATE_FORMAT).format((Date)propValue);
                    uriBuilder.queryParam(propName, ":"+dateStr);
                }else{
                    uriBuilder.queryParam(propName, propValue);
                }
            }
        }
    }

    return uriBuilder.build().toUriString();
}

All Those methods has been added after functional refactoring

// I couldn't avoid being imperative here, how can we refactor it to more functional style
 private BiConsumer<String, Object> buildRestParam(final UriComponentsBuilder uriBuilder) {
    return (propName, propValue) -> {
        if (propValue instanceof Date) {
            String dateStr = new SimpleDateFormat(DATE_FORMAT).format((Date) propValue);
            uriBuilder.queryParam(propName, ":" + dateStr);
        } else {
            uriBuilder.queryParam(propName, propValue);
        }
    };
}

private Predicate<? super PropertyDescriptor> notNullPropValue() {
    return propDesc -> {

        return propValue().apply(propDesc) != null;

    };
}


private Predicate<? super PropertyDescriptor> notClassProp() {
    return propDesc -> {
        return !propDesc.getName().equals("class");
    };
}

private Function<? super PropertyDescriptor, ? extends Object> propValue() {
    return (propDesc) -> {
        try {
            return propDesc.getReadMethod().invoke(HotelOfferSearchCommand.this);
        } catch (IllegalAccessException e) {
            e.printStackTrace();
            throw new RuntimeException(e);
        } catch (IllegalArgumentException e) {
            e.printStackTrace();
            throw new RuntimeException(e);
        } catch (InvocationTargetException e) {
            e.printStackTrace();
            throw new RuntimeException(e);
        }
    };
}

Answer 1

Most of the verbosity of the new code has nothing to do with functional programming. You have refactored the code to put every lambda expression into a method of it’s own, which, of course, destroys one of the main advantages of lambda expressions, the compactness. Even if code is complex enough to justify the creation of a method, that method should perform actual work, then, you could use a method reference where a function is required.

The methods further suffer from an unnecessary (even discouraged, as being in a return type) use of wild cards. You also used the verbose syntax parameter -> { return expression; } where parameter -> expression would be possible.

There are other issues, like unnecessarily creating a distinct catch clause for each exception type, when all do the same or wrapping the array into a List before creating the Stream instead of streaming over the array directly or having code duplication, the last point applies to both, the imperative variant and the functional one.

You can just write:

public String functionalBuildRestUri() throws Exception {
    final UriComponentsBuilder uriBuilder = UriComponentsBuilder.newInstance()
        .scheme("https").host("foo.com").path("/offers");
    Function<PropertyDescriptor, Object> propValue = propDesc -> {
        try { return propDesc.getReadMethod().invoke(HotelOfferSearchCommand.this); }
        catch(ReflectiveOperationException e) { throw new RuntimeException(e); }
    };
    Arrays.stream(BeanUtils.getPropertyDescriptors(getClass()))
          .filter(propDesc -> !propDesc.getName().equals("class"))
          .filter(propDesc -> propValue.apply(propDesc) != null)
          .forEach(propDesc -> {
              Object value = propValue.apply(propDesc);
              if (value instanceof Date)
                  value = ":"+new SimpleDateFormat(DATE_FORMAT).format(value);
              uriBuilder.queryParam(propDesc.getName(), value);
          });
    return uriBuilder.build().toUriString();
}

without any extra method.

This might not be the best option, as there is indeed one flaw, the absence of a tuple or pair type to hold two values to be passed through the stream. By using Map.Entry as a stand-in, but not populating a Map, we can express the operation as

public String functionalBuildRestUri() throws Exception {
    final UriComponentsBuilder uriBuilder = UriComponentsBuilder.newInstance()
        .scheme("https").host("foo.com").path("/offers");
    Function<PropertyDescriptor, Object> propValue = propDesc -> {
        try { return propDesc.getReadMethod().invoke(HotelOfferSearchCommand.this); }
        catch(ReflectiveOperationException e) { throw new RuntimeException(e); }
    };
    Arrays.stream(BeanUtils.getPropertyDescriptors(getClass()))
          .filter(propDesc -> !propDesc.getName().equals("class"))
          .map(propDesc -> new AbstractMap.SimpleImmutableEntry<>(
                               propDesc.getName(), propValue.apply(propDesc)))
          .filter(entry -> entry.getValue() != null)
          .forEach(entry -> {
              Object value = entry.getValue();
              if (value instanceof Date)
                  value = ":"+new SimpleDateFormat(DATE_FORMAT).format(value);
              uriBuilder.queryParam(entry.getKey(), value);
          });
    return uriBuilder.build().toUriString();
}

or, alternatively

    Arrays.stream(BeanUtils.getPropertyDescriptors(getClass()))
          .filter(propDesc -> !propDesc.getName().equals("class"))
          .map(propDesc -> new AbstractMap.SimpleImmutableEntry<>(
                               propDesc.getName(), propValue.apply(propDesc)))
          .filter(entry -> entry.getValue() != null)
          .map(e -> e.getValue() instanceof Date?
                  new AbstractMap.SimpleImmutableEntry<>(e.getKey(),
                        ":"+new SimpleDateFormat(DATE_FORMAT).format(e.getValue())):
                  e)
          .forEach(entry -> uriBuilder.queryParam(entry.getKey(), entry.getValue()));

With these two variants, the propValue function is evaluated only once per element instead of two times as in the first variant and your original code, where both, the check for null property value and the terminal operation evaluated it.

Note that there’s still room for improvement, e.g. there’s no reason to add the ":" after the format operation when you could make the colon a part of the format pattern string in the first place.

Whether this is an improvement over the loop, is something you have to decide yourself. Not every code has to be rewritten to a functional style. At least, as shown by the the examples above, it doesn’t have to be bigger than the imperative code…