Zwracanie informacji o wyniku operacji.

Witam. Zastanawiam się jak w najlepszy sposób zwrócić informację o wyniku operacji (sukces, błąd itd.) w programie. Mam klasę dodającą użytkownika:

    @Override
    public UserOperationState create(User user) {
        if ((user.getEmail() != null) && (findEmailExist(user.getEmail()))) {
            return UserOperationState.EMAIL_EXIST;
        } else if ((user.getPersonalId() != null) && (findPersonalIdExist(user.getPersonalId()))) {
            return UserOperationState.PERSONALID_EXIST;
        } else if ((user.getPhone() != null) && (findPhoneExist(user.getPhone()))) {
            return UserOperationState.PHONE_EXIST;
        } else {
            try {
                userDao.create(user);
                return UserOperationState.CREATE_SUCCESS;
            } catch (EJBException e) {
                return UserOperationState.CREATE_ERROR;
            }
        }
    }

Wcześniej zwracałem Stringi zamiast tak jak tutaj stanu obiektu. Poniżej fragment enuma state:

public enum UserOperationState {
    CREATE_SUCCESS {

        @Override
        public String getMessage() {
            return "Konto stworzone";
        }
    }, EDIT_SUCCESS {

        @Override
        public String getMessage() {
            return "Konto zedytowane";
        }
    };

    public abstract String getMessage();
}
 

Następnie w metodzie wywołującej powyższą metodę create mam coś takiego:

    public String createUser() {
        user.setRole(requestParameterMap.get("userRole").toString());
        user.setActive(Boolean.parseBoolean(requestParameterMap.get("active").toString()));
        UserOperationState result = userService.create(user);
        
        if (result == UserOperationState.CREATE_SUCCESS) {
            sendInfoMessageToUser(result.getMessage());
            return user.getRole() + "List";
        } else {
            sendErrorMessageToUser(result.getMessage());
            return null;
        }
    } 

Wcześniej kiedy były tutaj stringi tych ifów było więcej bo może zostać zwrócona informacja: success, emailexist, phoneexist itd. Zastanawiam się jednak czy moje rozwiązanie jest dobre i czy może istnieje jakieś lepsze? Może ktoś bardziej doświadczony zna jakiś lepszy sposób.

Przecież możesz wykorzystać switcha.
Generalnie podejście masz dobre, choć możesz mieć problem z i18n.

Edit: no i ja napisałbym raczej ERR_PHONE_EXISTS/ERROR_PHONE_EXISTS zamiast PHONE_EXIST, ponieważ wyraźniej mówi, że coś jest problemem.

1. Podziel to na kawałki. Cały ten początek to jest waliadtor i walidacja czy można dodać coś takiego do bazy. Dopiero potem masz faktyczne dodanie do bazy.
2. Wyjątki. Jak coś jest niepoprawne to możesz zawsze rzucić wyjątek.
3. Ja bym w ogóle zrobił sobie interfejs UserDataValidator z metodą validate(User user) throws ValidationException a potem sobie zaimplementował klasy według tego interfejsu np.

class EmailValidator implements UserDataValidator{
    @Override
    public void validate(User user) throws ValidationException{
        if ((user.getEmail() != null) && (jakiśtamService.findEmailExist(user.getEmail()))) {
            throw new InvalidEmailException();
        }
    }
}

Możesz sobie wtedy zrobić kolekcję walidatorów i ją wywołać dla pobranego obiektu.

@Shalom pomysł z walidatorami naprawdę dobry, dzięki. Napisałem coś takiego:

Interfejs dla walidatorów

public interface UserDataValidator {
    
    void validateCreateUser(User user) throws ValidationException;
    
    void validateUpdateUser(User user) throws ValidationException;
}

ma on 3 implementacje PhoneValidator, EmailValidator, PersonalIdValidator, wrzucam jedną z nich poglądowo:

public class EmailValidator implements UserDataValidator{

    @PersistenceContext
    private EntityManager EntityManager;
    
    @Override
    public void validateCreateUser(User user) throws InvalidEmailException{
        if ((user.getEmail() != null) && (findEmailExist(user.getEmail()))) {
            throw new InvalidEmailException();
        }
    }
    
    @Override
    public void validateUpdateUser(User user) throws InvalidEmailException {
        if ((user.getEmail() != null) && (findEmailExist(user.getEmail(), user.getId()))) {
            throw new InvalidEmailException();
        }
    }
    //tutaj metody findEmailExist w 2 wariantach
} 

Dodatkowo zrobiłem te wyjątki i tutaj mam pewne wątpliwości czy jest to dobre rozwiązanie:

Klasa abstrakcyjna wyjątków:

public abstract class ValidationException extends Exception{
    
    public ValidationException() {
    }
    
    public ValidationException(String message) {
        super(message);
    }
} 

Również ma 3 implementacje InvalidPhoneException, InvalidPersonalIdException oraz PersonalIdException. Jedna poglądowa klasa:

public class InvalidEmailException extends ValidationException{
    
    public InvalidEmailException() {
    }
    
    public InvalidEmailException(String message) {
        super(message);
    }
} 

Wtedy mój serwis wygląda tak:

    private void initValidator() {
        userDataValidatorList = new ArrayList<>();
        userDataValidatorList.add(new EmailValidator());
        userDataValidatorList.add(new PersonalIdValidator());
        userDataValidatorList.add(new PhoneValidator());
    }

    @Override
    public UserOperationState create(User user) {
        for(UserDataValidator validator : userDataValidatorList ) {
            try {
                validator.validateCreateUser(user);
            } catch (InvalidEmailException ex) {
                return UserOperationState.EMAIL_EXIST;
            } catch (InvalidPersonalIdException ex) {
                return UserOperationState.PERSONALID_EXIST;
            } catch (InvalidPhoneException ex) {
                return UserOperationState.PHONE_EXIST;
            } catch (ValidationException ex) {
                Logger.getLogger(UserService.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
        try {
            userDao.create(user);
            return UserOperationState.CREATE_SUCCESS;
        } catch (EJBException e) {
            return UserOperationState.CREATE_ERROR;
        }
    } 

i tutaj właśnie to co mi się nie podoba to te 4 catche, w dodatku muszę obsługiwać wyjątek ValidationException bo mam listę typu interfejsu. Pomyślałem, że lepszym rozwiązaniem niż te 4 catche będzie pominięcie wyjątków. Ogólnie czy te wyjątki to dobry pomysł? Przecież wprowadzenie przez użytkownika błędnego adresu email, telefonu czy pesel nie jest żadną sytuacją wyjątkową, tylko normalnym błędem na porządku dziennym. Nie mylę się, że te wyjątki byłyby trochę na wyrost? Postanowiłem to trochę zmodyfikować wyrzucając w ogóle wyjątki a klasy walidatorów w taki sposób, że zamiast rzucać wyjątek będą zwracać odpowiedni UserOperationState. Kod poniżej:

Przykładowa zmodyfikowana klasa walidatora, nie rzuca już wyjątków, tylko w wypadku gdy napotka na błąd zwraca odpowiedni stan w przeciwnym wypadku zwraca nulla(tu mam wątpliwości czy takie zwracania nulla jest ok)

public class EmailValidator implements UserDataValidator{

    @PersistenceContext
    private EntityManager EntityManager;
    
    @Override
    public UserOperationState validateCreateUser(User user){
        if ((user.getEmail() != null) && (findEmailExist(user.getEmail()))) {
            return UserOperationState.EMAIL_EXIST;
        }
        return null;
    }
    
    @Override
    public UserOperationState validateUpdateUser(User user){
        if ((user.getEmail() != null) && (findEmailExist(user.getEmail(), user.getId()))) {
            return UserOperationState.EMAIL_EXIST;
        }
        return null;
    } 

Wtedy klasa w serwisie wygląda tak:

    @Override
    public UserOperationState create(User user) {
        for(UserDataValidator validator : userDataValidatorList ) {
            UserOperationState userOperationState = validator.validateCreateUser(user);
            if (userOperationState != null) {
                return userOperationState;
            }
        }
        try {
            userDao.create(user);
            return UserOperationState.CREATE_SUCCESS;
        } catch (EJBException e) {
            return UserOperationState.CREATE_ERROR;
        }
    } 

Wydaje mi się być zwięźlejsza. Lecimy po kolekcji walidatorów i jeśli, któryś z nich zwróci coś innego niż null to kończymy metodę poprzez zwrócenie odpowiedniego UserOperationState.

1. Arrays.asList((new EmailValidator(), new PersonalIdValidator(), new PhoneValidator()) ;)
2. Wydziel tą walidację osobno bo nie jest częścią zapisywania obiektu. Zróbże metodę validate :P
3. Ale po co w ogóle zwracasz ten state? Ja tego zupełnie nie rozumiem. Co z tego niby u ciebie potem wynika? Masz potem jakiegoś switcha który to obsluguje? Po co? Bo ja rozumiem że potem w kontrolerze obsługujesz te potencjalne błędy. Ja bym w takim razie np. komunikat błędu upakowal w tych klasach wyjątków a w kontrolerze łapał wyjątek a nie sprawdzał jakieś kody. Ot masz w kontrolerze:

try{
    serwis.create(user);
}catch(ValidationException ex){
    return model and view z ex.getErrorMessage();
}
    return wszystko ok;

W kontrolerze (beanie, korzystam z JSF) mam coś takiego:

    public String createNewAccount() {
        user.setRole(requestParameterMap.get("userRole").toString());
        user.setActive(Boolean.parseBoolean(requestParameterMap.get("active").toString()));
        UserOperationState result = userService.create(user);
        
        if (result == UserOperationState.CREATE_SUCCESS) {
            return "newAccountCreated";
        } else {
            sendErrorMessageToUser(result.getMessage());
            return null;
        }
    } 

W sumie podobnie do twojego rozwiązania, tylko ja mam te komunikaty w enumState a nie w wyjątkach.

No to ja bym to zrobił jeszcze inaczej bo teraz to robisz sporego wtfa z tymi switcham i ifami :D
Weź zrób jedną klasę wyjątku ValidationError i wrzucaj do niej komunikat błędu w walidatorze po prostu. Tzn np. new ValidationError("zły email kmiocie!"); i tyle. A ta metoda serwisu niech propaguje wyjątek aż do kontrolera jeśli wyskoczy a w kontrolerze robisz, tak jak pokazałem wyżej, try i catch i tyle. I nagle nie ma znaczenia ile jest walidatorów bo nie ma żadnych ifów ani cudów na kiju ;]