Using Predicate to simplify multiple conditions

user_9401029 0
Viewed 130

I have the following method which works fine. But it has multple if / else
logic going on in there and there is likely going to be an additional few more if/else logic that needs to be added soon.

Is there a way I could write this more elegantly, possibly by using Predicate instead?

To note: replaceFunction is a functional interface I wrote myself with the method apply in it.
It takes in 3 Strings and return a String based on some logic.

private String getLabel(String endDate, Map<String, Object> details) {
    LocalDate offerEndDate = LocalDate.parse(endDate, DateTimeFormatter.ofPattern("dd-MM-yyyy"));
    LocalDate currentDate = ZonedDateTime.now(clock).withZoneSameInstant(zoneId).toLocalDate();
    long numberOfDays = DAYS.between(currentDate, offerEndDate);

    // can't use switch cos numberOfDays is long and don't want to perform any downcasting to an int just for that. 
    if (numberOfDays > 20) {
        return replaceFunction.apply((String)details.get("EXPIRY_DATE"), "~EXPIRY_DATE~", offerEndDate.format(DateTimeFormatter.ofPattern("MM-dd-yy")));
    } else if (numberOfDays > 13) {
        return replaceFunction.apply((String)details.get("EXPIRY_DAYS"), "~NO_OF_DAYS~", String.valueOf(numberOfDays));
    } else if (numberOfDays == 2) {
        return (String)details.get("EXPIRES_TOMORROW");
    } else if (numberOfDays == 1) {
        return (String)details.get("LAST_EXPIRY_DAY");
    } else {
        return null;
    }
}
java-8 java
2 Answers

Is there a way I could write this more elegantly, possibly by using Predicate instead?

I think the answer is No.

You could have dealt with the problem of the cast of a long to an int being lossy by testing that the value of numberOfDays is in the range Integer.MIN_VALUE to Integer.MAX_VALUE before casting to an int.

Or you could have observed that 231 days is a really long time. About 5.8 million years. I don't think that an "offer" needs to be open that long.

But I don't think a Java switch statement would be an improvement here anyway. (A switch cannot express ranges elegantly.)

There will always be things that you cannot express in a concise / elegant way in the programming language that you are using. My advice is to just accept that ... and write (if necessary) inelegant code that does the job1.

1 - The real purpose of code is to perform a function, not to be beautiful. My late father was a civil engineering lecturer. In his office at uni, he had a small sign thumb-tacked to one of his bookcase. It said "Time is Money". Good engineers are pragmatic, and that includes software engineers.

user_1399850

You can use a map lookup when you use a NavigableMap which knows the natural order of the keys and therefore can return the associated value for, e.g. an “equal or lower” relation.

static TreeMap<Long,BiFunction<LocalDate,Map<String, Object>,String>> LABELS = new TreeMap<>();
static {
    LABELS.put(1L, (d,m) -> (String)m.get("LAST_EXPIRY_DAY"));
    LABELS.put(2L, (d,m) -> (String)m.get("EXPIRES_TOMORROW"));
    LABELS.put(3L, (d,m) -> replaceFunction.apply((String)m.get("EXPIRY_DAYS"),
            "~NO_OF_DAYS~", String.valueOf(DAYS.between(LocalDate.now(clock), d))));
    LABELS.put(20L, (d,m) -> replaceFunction.apply((String)m.get("EXPIRY_DATE"),
            "~EXPIRY_DATE~", d.format(DateTimeFormatter.ofPattern("MM-dd-yy"))));
}
private String getLabel(String endDate, Map<String, Object> details) {
    LocalDate offerEndDate = LocalDate.parse(endDate, DateTimeFormatter.ofPattern("dd-MM-yyyy"));
    LocalDate currentDate = ZonedDateTime.now(clock).withZoneSameInstant(zoneId).toLocalDate();
    long numberOfDays = DAYS.between(currentDate, offerEndDate);
    return LABELS.floorEntry(numberOfDays).getValue().apply(offerEndDate, details);
}

Note that I changed the key values. Your original code does not handle values from three to twelve which can’t be intentional. With the floorEntry there can’t be any gaps; if I used the keys 1, 2, 13, and 20, all values from 2 to 12 would have been handled the same way. But obviously, the intent is to use the number of days from three upwards, until switching to a date.

I changed the handler code to repeat the DAYS.between(...) operation, to be able to use standard interfaces only. You seem to have a three-arg interface, so you could change to that interface and pass the already known number of days as function argument instead.

As another note, apparently you want to detach the code from the user messages, perhaps localize the messages, which is the good thing. Using the hard-coded date format MM-dd-yy is counter-acting the intent and can make things worse, e.g when showing a correctly translated message together with the non-localized date format where people are used to dd-MM-yy instead.

Maybe you can combine the details map with the other map, passing a map of formatting functions to the getLabel method.

user_27114880
Related