Problem
I wonder if there is a better approach to validate the object fields. For specific types of objects some fields cannot be empty:
public class CredentialsValidator {
public boolean validateCredentialsRequest(ValidationCredentialsRequest credentialsRequest) {
switch (credentialsRequest.getCredentialType()) {
case TYPEA:
return validatetypeACredentials(credentialsRequest);
case TYPEB:
return validateTypeBCredentials(credentialsRequest);
case TYPEC:
return validateTypeCCredentials(credentialsRequest);
}
return true;
}
private boolean validateTypeCCredentials(ValidationCredentialsRequest credentials) {
if (credentials.getIsNewCredential()) {
return nonNull(credentials.getUsername())
&& !credentials.getUsername().isEmpty()
&& nonNull(credentials.getPassword())
&& !credentials.getPassword().isEmpty();
}
return nonNull(credentials.getCredentialsId());
}
private boolean validateTypeBCredentials(ValidationCredentialsRequest credentials) {
if (credentials.getIsNewCredential()) {
return nonNull(credentials.getHostname())
&& !credentials.getHostname().isEmpty()
&& nonNull(credentials.getPort())
&& !credentials.getPort().isEmpty()
&& nonNull(credentials.getUsername())
&& !credentials.getUsername().isEmpty()
&& nonNull(credentials.getPassword())
&& !credentials.getPassword().isEmpty();
}
return nonNull(credentials.getCredentialsId())
&& nonNull(credentials.getHostname())
&& !credentials.getHostname().isEmpty()
&& nonNull(credentials.getPort())
&& !credentials.getPort().isEmpty();
}
private boolean validatetypeACredentials(ValidationCredentialsRequest credentials) {
if (credentials.getIsNewCredential()) {
return (nonNull(credentials.getDomain())
&& !credentials.getDomain().isEmpty()
&& nonNull(credentials.getUsername())
&& !credentials.getUsername().isEmpty()
&& nonNull(credentials.getPassword())
&& !credentials.getPassword().isEmpty())
|| (nonNull(credentials.getSharePoint().getUsername())
&& !credentials.getSharePoint().getUsername().isEmpty()
&& nonNull(credentials.getSharePoint().getPassword())
&& !credentials.getSharePoint().getPassword().isEmpty());
}
return nonNull(credentials.getCredentialsId())
&& nonNull(credentials.getDomain())
&& !credentials.getDomain().isEmpty();
}
My main class that invokes Validator:
public Mono<ValidationCredentialsResponse> validateCredentials(
Mono<ValidationCredentialsRequest> validationCredentials) {
return validationCredentials
.filter(credentialsValidator::validateCredentialsRequest)
.switchIfEmpty(Mono.error(new InvalidCredentialsException(MESSAGE)))
.flatMap(
validCredentials ->
securityService
…
…
I use Spring Boot and Webflux for reactive programming
Solution
From an object-oriented point of view there is a better way you can handle this scenario.
Feature Envy
Currently the code snipped has a Code Smell that is called Feature Envy:
The whole point of objects is that they are a technique to package data with the processes used on that data. A classic [code] smell is a method that seems more interested in a class other than the one it is in. The most common focus of the envy is the data.
— Martin Fowler, Refactoring: Improving the Design of Existing Code, Chap. 3, p. 80
The class CredentialsValidator
only interacts with data from ValidationCredentialsRequest
by accessing it via all the getters:
nonNull(credentials.getUsername())
&& !credentials.getHostname().isEmpty()
/* ... */
Instead, this logic could be in ValidationCredentialsRequest
, and CredentialsValidator
only interacts with the validation methods instead of the getters:
public boolean validateCredentialsRequest(ValidationCredentialsRequest credentialsRequest) {
switch (credentialsRequest.getCredentialType()) {
case TYPEA:
return credentialsRequest.isTypeAValid();
case TYPEB:
return credentialsRequest.isTypeBValid();
case TYPEC:
return credentialsRequest.isTypeCValid();
}
return true;
}
class ValidationCredentialsRequest {
/* ... */
public boolean isTypeAValid() {/* ... */ }
public boolean isTypeBValid() {/* ... */ }
public boolean isTypeCValid() {
if (newCredential) {
return nonNull(username)
&& !username().isEmpty()
&& nonNull(password)
&& !password.isEmpty();
}
return nonNull(credentialsId);
}
}
Polymorphism
Polymorphism is a concept where one thing can be something else at the same time: for example, a Shape
can be a Rectangle
at the same time.
With Polymorphism
we can achieve that we no longer need the switch case. This would have the advantage that we don’t have to change the CredentialsValidator
class when adding new validation logic:
class CredentialsValidator {
public boolean validateCredentialsRequest(ValidationCredentialsRequest credentialsRequest) {
return credentialsRequest.isValid();
}
}
From here all different “shapes” of ValidationCredentialsRequest
need to share the same interface – all need the isValid
method:
abstract class ValidationCredentialsRequest {
/* ... */
abstract boolen isValid();
}
class TypeARequest extends ValidationCredentialsRequest {
/* ... */
public boolean isValid() { /* ... */ }
}
class TypeBRequest extends ValidationCredentialsRequest {
/* ... */
public boolean isValid() { /* ... */ }
}
class TypeCRequest extends ValidationCredentialsRequest {
/* ... */
public boolean isValid() {
if (newCredential) {
return nonNull(username)
&& !username().isEmpty()
&& nonNull(password)
&& !password.isEmpty();
}
return nonNull(credentialsId);
}
}
Now when a new kind of ValidationCredentialsRequest
is needed, we just have to create a new “form” of it, and it works without changing the CredentialsValidator
.
Edit
Use in Main
In the code for the main you provided nothing needs to be changed.
But if you would like to add the Polymorphism to your code, you need to change it where you create the ValidationCredentialsRequest
objects. There you would create the different “shapes” of ValidationCredentialsRequest
instead of ValidationCredentialsRequest
itself.