Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In a user registration form, the validator currently checks not only user input, but also whether or not the user exists. This seems like business logic, which in terms of design should be in the service layer in a "save user" type method. On the other hand, it is highly convenient to keep it in the validator.

Comments on best practice?

public void validate(Object target, Errors errors) {
    User user = (User) target;
    ValidationUtils.rejectIfEmpty(errors, "name", "name.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(errors, "username", "username.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(errors, "password", "password.required", "Error msg...");

    // check if user already exists
    String username = user.getUsername();
    if (username != null && userService.userExists(username)) {
        errors.rejectValue("username", "username.exists", "Username is taken.");
    }
}
share|improve this question

2 Answers 2

as it probably hits a database, i would call userExists from outside the validate function. So, registration method first checks the input. Then calls userService.Register(User user, Errors errors). Register detects that username already exists and throws. And validation does not need to know about usernames and that they are unique or non-unique.

share|improve this answer
    
And you can get in all kinds of trouble depending on the framework, maybe there are some optimizations in place, which make DB access from a validator quite slow? –  Falco Aug 14 at 13:48
public void validate(Object target, Errors errors) {
    User user = (User) target;
    ValidationUtils.rejectIfEmpty(errors, "name", "name.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(errors, "username", "username.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(errors, "password", "password.required", "Error msg...");
    // ... 
}

It's strange that neither target nor user is parameter of the ValidationUtils.rejectIfEmpty calls, even though it seems that the method checks for missing attributes. With no visible connection between target and ValidationUtils.rejectIfEmpty, this code looks like magic. It would be better to make target a parameter of the method:

public void validate(Object target, Errors errors) {
    ValidationUtils.rejectIfEmpty(target, errors, "name", "name.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(target, errors, "username", "username.required", "Error msg...");
    ValidationUtils.rejectIfEmpty(target, errors, "password", "password.required", "Error msg...");
    // ... 
}
share|improve this answer
    
Good point, although to some extent the usage of any large framework seems to require a certain amount of faith. :) –  Captain Hindsight Aug 14 at 12:14
    
Maybe you're referring to things like @Autowired? You need to trust the framework that it will fill it properly, right? But I think that's different from what's happening here, because the @Autowired annotation, although not very specific, but at least it indicates a connection. In this code there is no such explicit indication. –  janos Aug 14 at 12:20
    
+1 I was reading this code 3 times, wondering what I missed - no reference whatsoever and strings as parameters, although you later use the much more expected user.getUsername() –  Falco Aug 14 at 13:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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