Better way to write the checkOrElseThrow generic function

Question

I have two function calls for Employee and Address DAO class where I check if the employee name or address is already in used

For making it generic to check and throw exception I have created the following generic function

checkOrElseThrow in CommonUtil.java

public static <R, C, T extends Throwable> R checkOrElseThrow(R rtn, C chk, Supplier<? extends T> ex) throws T
{
    if (chk != null)
    {
        throw ex.get();
    }
    return rtn;
}

and the above generic function is been called in EmployeeDAO.java and AddressDAO.java like as shown below

checkAndReturnEmployee in EmployeeDAO.java

public Employee checkAndReturnEmployee(Employee employee) {
    return checkOrElseThrow(
        employee,
        employee.getAddressName(),
        () -> new EntityNotFoundException("Employee already in use for another address"));
}

checkAndReturnAddress in AddressDAO.java

public Address checkAndReturnAddress(Address address) {
    return checkOrElseThrow(
        address,
        address.getEmployeeName(),
        () -> new EntityNotFoundException("Address already in use for another address"));
}

Question

My solution is working fine, but I would like to know if there is any other better way to rewrite the generic function (checkOrElseThrow) which I have written

How is `employee.getAddressName() != null` a check for *"address is already in used"*? You're not checking the database to see if that address is already used by another employee. — Andreas, Apr 04 '20 at 22:30

weston · Accepted Answer · 2020-04-04T22:06:19.877

5

The best way to write this is to not.

public Employee checkAndReturnEmployee(Employee employee) {
    if (employee.getAddressName() == null) {
      throw new EntityNotFoundException("Employee already in use for another address"));
    }
    return employee;
}

The code above is just as short, but far more readable. It's clearer what the condition is, and what happens when it is not met.

Your custom function only serves to attempt to create a new syntax for Java, one that other people will not understand, and you may soon forget also.

edited Apr 04 '20 at 22:06

answered Apr 04 '20 at 21:56

weston

54,145
21
145
203

don't forget to check for employee nullability – mstzn Apr 04 '20 at 22:03
1

@mstzn The original code doesn't check that either. – weston Apr 04 '20 at 22:04
How abt `Optional.ofNullable(carDO) .filter(e -> e.getAddressName() != null) .orElseThrow(() -> new EntityNotFoundException("Employee already in use for another address"));` – Alex Man Apr 05 '20 at 03:50
What does that give you over simple, regular Java? – weston Apr 05 '20 at 03:54
since `Optional.ofNullable(employee) .filter(e -> e.getAddressName() != null)`, it does null check for both `employee` and `e.getAddressName` and returns `Employee` object in a single line – Alex Man Apr 05 '20 at 04:12
Thanks for accepting. You can do that if you like of course. It is better to use optional than your own thing, but I still question the benefit. Is 1 line so important to you? The optional does a lot more work, including allocations. I don't like to prematurely optimize, but this feels like an unnecessary waste. It also places a mental overhead on the reader, harder to follow than an if. – weston Apr 05 '20 at 11:40

Mher Davtyan · Answer 2 · 2020-04-06T12:18:49.737

Consider using java.util.Optional since the behavior that you are trying to achieve is already there. I find it far more elegant than if (smth != null) checks.

Optional.ofNullable(employee)
    .map(Employee::getAddressName)
    .orElseThrow(() -> new EntityNotFoundException("Employee already in use for another address");

In general, I prefer Optional mainly because one would probably nest multiple ifs or chain the conditions if null check for entity was also needed (not the case for this question). Then you would need something like if (entity != null && entity.getAddress() == null) {throw ...} which is ugly and far less readable than the chained version with Optional. The latter statement, of course, is also a bit of syntactic taste.

score 1 · Answer 3 · answered Apr 05 '20 at 03:29

Since the question was more around the generic implementation, you could modify your existing implementation to make use of a Predicate to test out any criteria and work it out as:

public <R, T extends Throwable> R checkOrElseThrow(R returnValue, Predicate<R> successCriteria,
                                                   Supplier<? extends T> ex) throws T {
    if (successCriteria.test(returnValue)) {
        return returnValue;
    }
    throw ex.get();
}

and further invoke this in corresponding places as:

public Employee checkAndReturnEmployee(Employee employee) throws EntityNotFoundException {
    return checkOrElseThrow(employee, emp -> emp.getAddressName() != null,
            () -> new EntityNotFoundException("Employee already in use for another address"));
}

public Address checkAndReturnAddress(Address address) throws EntityNotFoundException {
    return checkOrElseThrow(address, add -> add.getEmployeeName() != null,
            () -> new EntityNotFoundException("Address already in use for another address"));
}

the generic function now looks not needed since we are checking the null in the function. How abt `Optional.ofNullable(carDO) .filter(e -> e.getAddressName() != null) .orElseThrow(() -> new EntityNotFoundException("Employee already in use for another address"));` — Alex Man, Apr 05 '20 at 03:53
@AlexMan the actual value of the generic method would evolve when you can perform `public Employee checkAndReturnEmployee(Employee employee) throws EntityNotFoundException { return checkOrElseThrow(employee, emp -> emp.getAddressName().equals("success"), () -> new EntityNotFoundException("Employee already in use for another address")); }` at the same time as that of the `null` checks which is the case currently. — Naman, Apr 05 '20 at 06:38

Better way to write the checkOrElseThrow generic function

Question

3 Answers3