Tool Rental Application

Question

I have recently done some functional java programming at https://github.com/zmacomber/zm0823 and I would like feedback for what could be improved in my coding approach.

Here is one of of the classes from that repo:

package org.example;

import java.time.LocalDate;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Optional;

public class DayUtil {
    public static boolean isHoliday(LocalDate localDate) {
        if (localDate == null) {
            throw new IllegalArgumentException("localDate must be supplied");
        }
        return Arrays.stream(Day.Holiday.values()).anyMatch(holiday -> holiday.getDayPredicate().matches(localDate));
    }

    public static boolean shouldNotCharge(LocalDate localDate, Tool tool) {
        if ((localDate == null) || (tool == null)) {
            throw new IllegalArgumentException("localDate and tool must be supplied");
        }
        return Arrays.stream(Day.values()).anyMatch(day ->
                noChargeToolTypes(day).contains(tool.getToolType()) && day.getDayPredicate().matches(localDate)
        );
    }

    private static List<Tool.ToolType> noChargeToolTypes(Day day) {
        if (day == null) {
            throw new IllegalArgumentException("day must be supplied");
        }
        return Optional.ofNullable(day.getNoChargeToolTypes())
                .map(Arrays::asList)
                .orElse(Collections.emptyList());
    }
}

Answer 1

There go some of my thoughts:

if (localDate == null) {
    throw new IllegalArgumentException("localDate must be supplied");
}

Why not:

Objects.requireNonNull(localDate, "localDate is null.");

Same for:

if (day == null) {
    throw new IllegalArgumentException("day must be supplied");
}

Also, instead of

 if ((localDate == null) || (tool == null)) {
     throw new IllegalArgumentException("localDate and tool must be supplied");
 }

you could omit superfluous parentheses:

 if (localDate == null || tool == null) {
        throw new IllegalArgumentException("localDate and tool must be supplied");
    }

Even better, why not:

Objects.requireNonNull(localDate, "localDate is null.");
Objects.requireNonNull(tool, "tool is null.");

Answer 2

no negativism

    public static boolean shouldNotCharge(LocalDate localDate, Tool tool) {
        ...
    private static List<Tool.ToolType> noChargeToolTypes(Day day) {

Avoid putting negatives in identifiers, especially when it's part of the Public API you're designing for other to use. Humans are capable of correctly reasoning about negatives, but it's harder than reasoning about positives.

Conveniently there's an antonym available to us. We could name those isFree() and freeToolTypes().

The Day.values() appear to actually be holiday values.

---

De Moivre's identity

If you adopt the "positive" advice above, yet no antonym is available, apply De Moivre to deal with the negation.

---

a reason for each attribute

    ... day.getDayPredicate().matches(localDate)

That just seems like a weird expression. Why invent that predicate attribute, what burden is it carrying for us? I would much rather see the .matches() predicate implemented directly within Day:

    ... day.matches(localDate)

The rule here is you should be able to write a /** javadoc */ sentence describing the Single Responsibility of each class, e.g. the day predicate class. If you can't articulate that, it calls into question whether the class is pulling its own weight, whether we need to introduce that level of abstraction at all.

Sometimes we don't write that sentence because it's obvious just from the class name. Here, I feel it would be hard to justify it, and the exercise of committing that sentence to the source code repo would be well worth the effort. And if that effort doesn't pan out, then you have an opportunity to refactor so we have simpler expressions.

---

EDIT

code snippet that shows "instead of this, do this"?

Ok, first, my knee-jerk reaction upon reading an identifier was "no negativism", but upon delving deeper into your repo I see you've already gone pretty far down that particular path. I no longer feel it would be sensible to refactor this usage, and I offer it just as advice to keep in mind for future projects.

Let's try a simple example in python, different from your tools setup. We will examine scheduling academic classes in rooms, subject to mutual exclusion, no double-booking.

There are perfectly good antonyms available, like a room being "available" vs "busy", but for the moment we choose to ignore that possible way out, leading to slightly awkward expressions.

If the first two are "original" way of stating it, then the third would be "same thing after applying De Moivre".

    def has_conflict1(room, slot) -> bool:
        """Predicate that tells if proposed room and time slot conflicts with existing classes."""
        for busy_room, busy_slot in get_scheduled_classes():
            if busy_room == room and busy_slot == slot:
                return True
        return False

    def has_conflict2(room, slot) -> bool:
        # "any" computes a disjunction over many 2-term conjuncts.
        return any(busy_room == room and busy_slot == slot
                   for busy_room, busy_slot in get_scheduled_classes())

    def has_conflict3(room, slot) -> bool:
        # "all" computes conjunct of many 2-term disjuncts
        # "not x == y" is a slightly odd way of expressing "x != y"
        return not all(not busy_room == room or not busy_slot == slot
                       for busy_room, busy_slot in get_scheduled_classes())

Now we can return to antonyms, or equivalently to the decision to represent a concept using boolean True or boolean False. Depending on your initial decisions, you may find negations within, or a final negation on the outside. I made some arbitrary choices and wound up with all the negation in the third function, but it can shake out in several ways. (In this example it turns out the original seems the more natural way to express it.)

The return not ... would suggest ditching the not and renaming as is_available3().