A tak nie będzie czytelniej?
public async Task ValidateUser(User user, bool isEditing)
{
var doesExistByName = await _dbContext.Users.AnyAsync(u => u.Login == user.Login);
if (!isEditing && doesExistByName)
throw new LoginIsAlreadyUsedException();
if (isEditing)
{
var dbUserLogin = await _dbContext.Users
.Where(u => u.Id == user.Id)
.Select(u => u.Login)
.FirstOrDefaultAsync()
?? throw new NoSuchUserException(user.Id);
if (dbUserName != user.Login)
throw new CantUpdateLoginException();
}
}
- Dodaj dedykowane wyjątki.
- Nie zliczaj użytkowników po danym loginie, tylko sprawdź, czy jest chociaż jeden (porównaj koszt z planu zapytania sql
select count(*) from ... where ... z select top 1 (1) from ... where ...).
- Nie komplikuj warunków, które można rozbić i zapisać w trywialny sposób.
- Unikaj wbudowanych wyjątków - zamiast
First() wywołaj FirstOrDefault() i obsłuż swoim wyjątkiem null, zamiast później zastanawiać się przy lekturze logów, skąd te InvalidOperationException.
- Jak już przedmówca wspomniał, używaj wersji async metod materializujących dane, acz nie dlatego, że blokujesz główny wątek aplikacji, tylko dlatego, że blokujesz jeden z wątków aplikacji (który w pewnych przypadkach może być głównym wątkiem procesu), marnując zasoby. Kiedy wołasz await, wątek jest zwalniany i może zająć się inną robotą; kiedy wołasz metodę synchronicznie, wątek czeka śpiąc, aż operacja I/O się zakończy.
BTW cały kod wydaje się przekomplikowany i chyba wkradł się tam błąd, który nie pozwala istniejącym użytkownikom na zmianę loginu. Ja bym to widział tak (bazując na już przepisanym powyższym kodzie):
public async Task ValidateUser(User user, bool isEditing)
{
var userId = isEditing ? (int?)user.Id : null;
var doesExistByNameExcludingCurrentUser = await _dbContext.Users
.AnyAsync(u => u.Login == user.Login && u.Id != userId);
if (doesExistByNameExcludingCurrentUser)
throw new LoginIsAlreadyUsedException();
if (userId.HasValue)
{
var doesUserExist = await _dbContext.Users.AnyAsync(u => u.Id == userId);
if (!doesUserExist)
throw new NoSuchUserException(userId);
}
}
Przy czym sprawdzenie w isEditing jedynie weryfikuje, czy jest użytkownik z takim id, na upartego możesz to usunąć. Ten kod pozwala istniejącemu użytkownikowi na zmianę loginu.
