1
\$\begingroup\$

For a Student object in the package entity, I have a control.studentpackage with classes that are relevant to the Student object.

The Student is a class with only private properties and getters/setters for every property. The setters do no validation on their own.

My project structure is as follows:

src/
  control/
    student/
      StudentControl.java
      StudentValidator.java
      ValidationError.java
  entity/
     Student.java

ValidationError is just a String wrapper with getters and setters.

The StudentValidator is a functional interface that extends Function<Student, Optional<ValidationError>>.

interface StudentValidator extends Function<Student, Optional<ValidationError>> {}

The StudentControl class is as follows:

package control.student;

import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import entity.Student;

public class StudentControl {

  private static final Pattern namePattern = Pattern.compile("[aA-zZ]+");

  /**
   * List of functions that validate a Student.
   */
  private static final List<StudentValidator> validatorFunctions = new ArrayList<>();
  static {
    validatorFunctions.add(validateNamesFunction());
  }

  /**
   * Validates a Student's fields.
   * 
   * @param student Student to validate
   * @return an Optional that may contain a List of ValidationError. Those ValidationError(s) will
   *         have error messages describing each error occurred while validating the student.
   */
  public static Optional<List<ValidationError>> validate(Student student) {
    List<ValidationError> errors = validatorFunctions.stream()
        .map(f -> f.apply(student))
        .filter(Optional::isPresent)
        .map(Optional::get)
        .collect(Collectors.toList());

    return errors.isEmpty() ? Optional.empty() : Optional.of(errors);
  }

  /**
   * Returns a function that validates a Student's name.
   */
  public static StudentValidator validateNamesFunction() {
    return student -> {
      return namePattern.matcher(student.getName()).matches()
          ? Optional.empty()
          : Optional.of(new ValidationError("Invalid name"));
    };
  }

  /**
   * Returns a function that validates a Student's birth date.
   */
  public static StudentValidator validateBirthDateFunction() {
    return student -> {
      return student.getBirthDate().getTime() > new Date().getTime()
          ? Optional.of(new ValidationError("Student was born in the future."))
          : Optional.empty();
    };
  }

  /**
   * Returns a function that validates a Student's join date.
   */
  public static StudentValidator validateJoinDateFunction() {
    return student -> {
      return student.getJoinDate().getTime() > new Date().getTime()
          ? Optional.of(new ValidationError("Student joined in the future."))
          : Optional.empty();
    };
  }
}

Is this code readable? Does it easily convey to the programmer what it is meant to do? Does it comply with the MVC pattern? How could I change my code to make it look, feel and run better?

\$\endgroup\$
0
\$\begingroup\$

Creating validators

I think it's perfectly readable. However, there's a lot of repetition.

To make creating validators simpler and more declarative, you could take it one step further and lift the test/return-Optional logic into a higher-order function:

public class StudentControl {

  public static final StudentValidator namesValidator = validator(
    student -> namePattern.matcher(student.getName()).matches(),
    "Invalid name"
  );

  ...

  private static StudentValidator validator(Predicate<Student> pred, String error) {
    return student -> pred.test(student)
      ? Optional.empty()
      : Optional.of(error);
  }
}

and then if you have other models besides Student, you might genericize the validators.

interface EntityValidator<T> extends Function<T, Optional<ValidationError>> {}

public abstract class EntityControl<T> {
  protected final List<EntityValidator<? super T>> validators = new ArrayList();

  protected EntityValidator<T> validator(Predicate<T> pred, String error) {
    return x -> pred.test(x)
      ? Optional.empty()
      : Optional.of(error);
  }

  // Or, if you want to allow any subclass to create a validator for any type,
  // it could be static:
  protected static <T> EntityValidator<T> validator(...)

  public Optional<List<ValidationError>> validate(T entity) {
    ...
  }
}

This feels a bit cleaner to me as far as separation of concerns, but if you're only ever going to validate Students it's probably overkill.

Other

You might also consider having validate() return an Optional<Set<ValidationError>> instead of the List, if order isn't important. Since validation errors are just strings, you can't effectively do any further handling of them, so I assume they just get piped out to the user somehow?

Does it comply with the MVC pattern?

I'm not an MVC expert, but: what is the view supposed to be? You haven't included anything that displays Students or errors, receives input, or otherwise does anything with Students, so it's hard to thoroughly assess this.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the suggestion. I definitely like the EntityControl class. However I would suggest changing the methods to be non-static as the generic class T is non-static, otherwise there will be a compilation error. \$\endgroup\$ – the duck wizard Sep 8 '17 at 3:06
  • \$\begingroup\$ Good catch, thanks, I've updated that with two working alternatives. \$\endgroup\$ – BenC Sep 8 '17 at 3:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.