Walidacja danych i tworzenie konta z Either - lepszy zapis

Mam klasę AccountCreator z metodą create, która przyjmuje DTO z danymi potrzebnymi do utworzenia konta. Na początku jest próba stworzenia 2 value objectów (UserName i Password), potem walidacja unikalności user name, utworzenie encji Account, która przyjmuje w konstruktorze te 2 value objecty i zapisanie w repo. Oczywiście mogą zostać zwrócone błędy typu nieprawidłowa długość hasła itd. Użyłem do tego Eitherów i teraz pytanie czy ten kod jest ok czy może da się to jakoś lepiej zapisać?

public Either<Error, AccountDto> create(AccountCreateDto accountCreateDto) {

        final var errorType = ErrorType.ACCOUNT_PERSISTENCE_ERROR;

        final var errorMessage = "Not unique user name: " + accountCreateDto.userName;

        final var error = new Error(errorType,errorMessage);

        return UserName
                .create(accountCreateDto.userName)
                .flatMap(userName ->

                        userNameUniquenessChecker.isUnique(userName.text) ?

                                Password
                                        .create(accountCreateDto.password)
                                        .flatMap(password -> {

                                            final var createdAccount = new Account(
                                                    userName,
                                                    password,
                                                    AccountStatus.OPEN,
                                                    LocalDateTime.now(),
                                                    new ArrayList<>());

                                            final var addedAccount= accountRepository.add(createdAccount);

                                            final var accountDto = new AccountDto(
                                                    addedAccount.userName.text,
                                                    addedAccount.password.text,
                                                    addedAccount.status,
                                                    addedAccount.creationDate,
                                                    (long) addedAccount.tasks.size());

                                            return Either.right(accountDto);

                                        }) : Either.left(error));

Ty tak poważnie? Ja bym spytał czy da się to zapisać jakoś mniej czytelnie :D

1. Po co są ci te value objecty? Jeśli juz są, to czemu to nie one przeprowadzają walidacje tylko jest ona jakoś obok?
2. Nie rób wielolinijkowych lambd!
3. Czemu nie zwrócisz Either<Error, Account> na którym następnie zawołasz .flatMap(AccountRepository::add) a później .flatMap(Account::toDto) ? Będzie 100 razy czytelniej
4. Te errory zdefiniowane na początku to tez jakiś dziwny pomysł.

edit: oczywiście nie raczyłeś dać stubów do reszty klas, zeby dało sie z tym wygodnie coś poczarować. Moze np.:

import io.vavr.control.Either;

class AccountDto {
}

record AccountCreateDto(String userName, String password) {
}

class Error {
}

class UsernameUniquenessChecker {
    Either<Error, String> validateUnique(String name) {
        // if unique....
        return Either.right(name);
    }
}

class UniqueUsernameCreator {
    private final UsernameUniquenessChecker checker = null

    public Either<Error, UniqueUserName> create(String name) {
        return checker.validateUnique(name)
                .map(UniqueUserName::new);
    }
}

class ValidPasswordCreator {
    public Either<Error, ValidPassword> create(String name) {
        return Either.right(new ValidPassword(name));
    }
}

record UniqueUserName(String username) {
}

record ValidPassword(String password) {
}

record Account(UniqueUserName username, ValidPassword password) {
    public AccountDto toDto() {
        return new AccountDto();
    }
}

class AccountRepository {
    public Either<Error, Account> add(Account account) {
        return Either.right(account);
    }
}

public class Accounty {
    private final AccountRepository accountRepository = null;
    private final UniqueUsernameCreator uniqueUsernameCreator = null;
    private final ValidPasswordCreator passwordCreator = null;

    public Either<Error, AccountDto> create(AccountCreateDto accountCreateDto) {
        return uniqueUsernameCreator
                .create(accountCreateDto.userName())
                .flatMap(username -> passwordCreator.create(accountCreateDto.password())
                        .map(password -> new Account(username, password))
                )
                .flatMap(accountRepository::add)
                .map(Account::toDto);
    }
}

@Shalom: @PerlMonk :

Teraz może być?

public final class AccountCreator {

    private final UserNameUniquenessChecker userNameUniquenessChecker;
    private final AccountRepository accountRepository;

    public AccountCreator(UserNameUniquenessChecker userNameUniquenessChecker, AccountRepository accountRepository) {
        this.userNameUniquenessChecker = userNameUniquenessChecker;
        this.accountRepository = accountRepository;
    }
    
    private Account createAccount(UserName userName, Password password) {
        
        return new Account(userName,password,AccountStatus.OPEN,LocalDateTime.now(),new ArrayList<>());
    }
    
    private AccountDto toDto(Account account) {
        
        return new AccountDto(
                account.userName.text,
                account.password.text,
                account.status,
                account.creationDate,
                (long) account.tasks.size());
    }

    public Either<Error, AccountDto> create(AccountCreateDto accountCreateDto) {
        
        return UserName
                .create(accountCreateDto.userName,userNameUniquenessChecker)
                .flatMap(userName -> Password
                        .create(accountCreateDto.password)
                        .map(password -> createAccount(userName,password))
                        .map(accountRepository::add)
                        .map(this::toDto));
    }
}

Ja osobiście nadal nie rozumiem sensu istnienia tych klas UserName oraz Password.Co one tutaj wnoszą konkretnie? Przepisujesz jedno DTO (wejściowe) na drugie a potem na trzecie (bazodanowe). Jaki sens ma to środkowe?

@Shalom: UserName i UserPassword to value objecty z logiką biznesową.

Może jestem ślepy, ale tutaj zupełnie jej nie widzę. Wziałeś DTO, przepisałeś do obiektów domenowych, tylko po to żeby następnie przepisać to do kolejnego DTO które leci do bazy. Jeszcze pal licho jakbyś tam miał jakiś obiekt domenowy Credentials który ma jakąś metodę Account toAccount() która zamienia go w ten bazodanowy obiekt, to może miałoby to minimalny sens, ale tutaj nie bardzo rozumiem po co jest przelotka przez kolejne obiekty.

W tym fragmencie kodu może sensu nie widać na UserName i Password, ale to akurat normalne(*) żeby wydzielić takie typy. Nie każdy String sie nadaje na user name, nie każda tablica bajtów nadaje się na hasło. Do tego zwykle są jakieś specyficzne metody związane z tymi typami.

* - normalne to jest w językach, które nie karają za wprowadzanie typów bo mają jakieś wrappery typu value class lub typealiasy.

@Shalom: @jarekr000000:

Tak wyglądają UserName i Password.

public final class UserName {

    public final String text;

    public UserName(String text) {
        this.text = text;
    }

    public static Either<Error,UserName> create(String userName,UserNameUniquenessChecker userNameUniquenessChecker) {

        final var minUserNameLength = 3;

        final var maxUserNameLength = 10;

        final var isUserNameLengthWrong =
                userName.length() < minUserNameLength || userName.length() > maxUserNameLength;

        if (isUserNameLengthWrong) {

            return Either.left(ErrorFactory.createWrongUserNameLength(
                    userName,
                    minUserNameLength,
                    maxUserNameLength));
        }

        else if (userNameUniquenessChecker.isDuplicate(userName)) {

            return Either.left(ErrorFactory.createNotUniqueUserNameError(userName));
        }

        else {

            return Either.right(new UserName(userName));
        }
    }
}

public final class Password {

    public final String text;

    private Password(String text) {
        this.text = text;
    }

    public static Either<Error, Password> create(String text) {

        final var minPasswordLength = 5;

        return text.length() < minPasswordLength ?

                Either.left(ErrorFactory.createWrongPasswordLengthError(text,minPasswordLength)) :

                Either.right(new Password(text));
    }
}

Tak jak napisałem. To są value objecty.

Przyznam że ja bym w kodzie tak nie hardkodował tych zakresów na min i max length, tylko wyciągnął to do jakiegoś konfiga.

@Shalom: Tzn? Np. abstrakcyjna klasa ze stałymi?