I'm trying to clean up my code a little bit and create well-organized code.

I'm trying to transfer some functions that are related to the database from my controller to my repository.

Unfortunately, some functionality was lost due to my changes.

Specifically, the method that looks for existing emails

The old Code - controller (works but not organized):

    public async Task<ActionResult<UserDto>> Register(RegisterDto registerDto)
    {
        if (await _userRepository.UserExists(registerDto.Email))
        { 
            return BadRequest("The Email is already in use"); 
        }

        // rest of the code
    }

The repository

    public async Task<bool> UserExists(string email)
    {
        return await _userManager.Users.AnyAsync(x => x.Email == email.ToLower());
    }

What I have tried - controller: (organized but not working as intended

    public async Task<ActionResult<UserDto>> Register(RegisterDto registerDto)
    {
        await _userRepository.UserExists(registerDto.Email);

    // rest of the code
    }

The repository

    public async Task<ActionResult> UserExists(string email)
    {
        var user = await _userManager.Users.AnyAsync(x => x.Email == email.ToLower());

        if (user =! false)
        {
            return BadRequest("The Email is already in use");
        }
        
        return Ok();
    }

The issue is that in the case of an existing email, The app keeps running and doesn't throw the error - "The Email is already in use"

What am I doing wrong here? And why does it happen?

CodePudding user response：

user =! false

This is not an evaluation, it's an assignment. What you are doing here is:

user = !false

In other words:

user = true

In other words you are not checking if user is true, you are setting user to true

What you want here is

if (user)
{
    return BadRequest("The Email is already in use");
}

The second issue here is that your repository does return BadRequest("The Email is already in use");, but your controller never does anything with the returned value:

await _userRepository.UserExists(registerDto.Email);

To work, this should be:

return await _userRepository.UserExists(registerDto.Email);

However, the new code you wrote here introduced problems that did not exist before.

• user is not a great name. userExists would be better.
• A repository should never return BadRequest, that's a controller's job.
• It's unclear to me why you rewrote UserExists to begin with. It was doing its correct job, and you've somehow now foisted the controller's responsibility into the repository, which is introducing a mistake and not solving one.

I am convinced that the changes you made to the code need to be undone. The old code is actually well written. What you've done with it is introducing issues.