Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Java semantics - Is there a way to write this better?

I'm building a Spring backend. I've got a controller which gets a "search object" - an object with like 10 fields which only one of them should be filled, so the search function (which I did not write but need to make changes to and refactor) is written like this:

if( param1 != null ) user = getUserByParam1(param1);
else if ( param2 != null ) user = getUserByParam2(param2);
.
.
.
else if(lastName != null || lastName != null) user = getUserByName(firstName, lastName);
else user = getUserById(id);

if(user == null) throw costumException;
return user;

Notice the 2 special cases in the end- one of them checks the availability of 2 parameters instead of one and sends them both to same function (which can handle null in one of the fields, but not both), and the default case which assumes an ID is being passed (in case its not- it gets handled by the if (user == null) check after to throw exception).

Is there a way to refactor this piece of code to be more readable/good looking? Is there any design pattern or a known method I can use to do this? Or is it actually the best way to write this kind of functionality?

I've done a lot of thinking but couldn't find a nicer way to do so. I've had an idea to somehow send the field name that is filled and its value to another function which will "switch-case" on the field name and send the value to the appropriate function, but it doesn't really save much code (as I still need to manually iterate on all fields to find the one which is filled) and I'm not so sure it'll more readable.

I'm also quite new to Java so I do not know all the available APIs and interfaces, maybe there is something you can throw at me for help.

like image 548
Gibor Avatar asked Jan 09 '20 07:01

Gibor


Video Answer


1 Answers

Note: This is simply a workaround for your quite monstrous method. As mentioned in the comments, your endpoint tries to do too much. It would be much better to create many different endpoints which all only need the required parameters. Makes it a lot easier to understand than just to see 10+ if-else statements


You could create a data-class containing all the possible parameters:

public class UserRequest {
    private String lastName;
    private String firstName;

    // more fields, constructors, getters, setters etc.
}

Then have an interface looking something like this:

public interface UserRequestResolver {
    User resolve(UserRequest request);
}

This interface can then be implemented to depend on a given param, if it exists. Return a found User or simply null.

Next step is, create a List<UserRequestResolver> resolvers, and add the different implementations, with Java8+ you can use lambdas:

resolvers.add(r -> r.getParam1() != null ? getUserByParam1(r.getParam1()) : null);
resolvers.add(r -> {
    if(r.getFirstName() == null && r.getLastName()) return null;
    return getUserByName(r.getFirstName(), r.getLastName());
});
// etc.

Then when receiving a UserRequest you can simply iterate over resolvers and take the first return value which is not null:

for(UserRequestResolver resolver : resolvers) {
    User user = resolver.resolve(request);
    if(user != null) return user;
}
throw costumException;

If you're a fan of Streams, then you can replace the for-loop above with this:

return resolvers.stream()
    .map(resolver -> resolver.resolve(request))
    .filter(Objects::nonNull)
    .findFirst()
    .orElseThrow(() -> costumException);
like image 57
Lino Avatar answered Oct 17 '22 18:10

Lino