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.
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 Stream
s, then you can replace the for-loop above with this:
return resolvers.stream()
.map(resolver -> resolver.resolve(request))
.filter(Objects::nonNull)
.findFirst()
.orElseThrow(() -> costumException);
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With