1
\$\begingroup\$

This is a remote validation method for a registration form, fired when the user enters an email address. It checks that the domain portion exists in a given list of valid domains, and then also checks the email address has not already been registered.

    [AllowAnonymous]
    [AcceptVerbs("Get", "Post")]
    public async Task<JsonResult> CheckEmail(string Email)
    {
        bool validDomain = false;
        string s = Email; //e.g. myname@my.org
        int idx = s.LastIndexOf('@');
        Console.WriteLine(s.Substring(0, idx)); //local half (not needed) e.g myname
        string domain = s.Substring(idx + 1).ToLower();    //domain half e.g. my.org

        IEnumerable<Organisation> organisations = _context.Organisations.ToList();

        foreach (var organisation in organisations)
        {
            if (domain == organisation.EmailDomain)
            {
                validDomain = true;
                break;
            }
        }

        if (validDomain == false)
        {
            return Json(
                data: "" + domain + " accounts cannot be used");
        }

        var result = await _userManager.FindByEmailAsync(Email);
        if (result == null)
        {
            return Json(data: true);
        }
        return Json(data: $"Email address already registered</a>");

    }

I also tried to use this line instead of the foreach loop but wouldn't get it to work:

//string result = organisations.FirstOrDefault(o => o.Contains(domain));

I got "symbol Contains cannot be resolved."

Would appreciate feedback on improving the rest of the code if possible.

\$\endgroup\$
  • 2
    \$\begingroup\$ Please post only working code. Code Review is not about fixing bugs. We'll gladly take a look at your code when you edited the question and removed the broken part(s). \$\endgroup\$ – t3chb0t Sep 1 '17 at 9:48
  • \$\begingroup\$ I disagree that it's not working code. The code as presented does work and there is a comment adding something they did try which didn't work. The comment is off topic but the question isn't. \$\endgroup\$ – RobH Sep 1 '17 at 10:44
2
\$\begingroup\$

There are a few things that jump out at me in your code:

public async Task<JsonResult> CheckEmail(string Email)

Email should be email because it is conventional for parameters to be camelCase in C#.


Why are you making a copy of the email address?

string s = Email;

Strings are immutable, you can just use your parameter directly.


This line:

Console.WriteLine(s.Substring(0, idx)); //local half (not needed) e.g myname

First, you should never be using Console.WriteLine in a action method of a controller. Generally, you shouldn't be using it in a web app at all. If you want to debug it, add a log or just step through with an actual debugger using breakpoints.

Second, if you don't need the local half, why do you compute it at all?

Third, you haven't validated that the email address even contains an @. If it doesn't, this line will throw an exception.

Validate your index:

var atCharPosition = email.LastIndexOf('@');
if (atCharPosition == -1) 
{
   return Json(data: "Invalid email address");
}

This line is the most worrying:

IEnumerable<Organisation> organisations =  _context.Organisations.ToList();

Is that _context an Entity Framework Data Context? If it is, you're getting all of the organisations back from the database to see whether the domain exists. Let the database do it:

// requires a using System.Linq;
var isValidDomain =  _context.Organisations.Any(o => o.EmailDomain == domain);

Finally, why are you returning a closing anchor tag in this line?

return Json(data: $"Email address already registered</a>");

I can't see any reason why you'd want to return malformed html here.

\$\endgroup\$
  • \$\begingroup\$ Apart from the closing anchor tag (it was originally <a ...>Reset password</a> and I removed it so I wouldn't get ripped for it here, alas I rushed the removal and accidentally left the </a> portion in) the response to your "why" questions is that I'm new, and the logic I have implemented has been peiced together as a result of me googling specific things and getting bits of code from various sources (mostly stack overflow answers.) In my view model, email is validated first so it will be an email address. It is also capital E "Email" in the view model and so I thought it had to be the same. \$\endgroup\$ – egmfrs Sep 1 '17 at 13:12
  • \$\begingroup\$ Thanks for your feedback. You are right about _context and I look forward to using the ".Any" suggestion! \$\endgroup\$ – egmfrs Sep 1 '17 at 13:17
  • \$\begingroup\$ @egmfrs - fair enough. Sorry if I sounded harsh, I didn't mean to. We all start somewhere. \$\endgroup\$ – RobH Sep 1 '17 at 13:45
1
\$\begingroup\$

Variables

Parameter names follow camel casing rules as they are narrow scope variables.

public async Task<JsonResult> CheckEmail(string Email) // does not fit naming conventions
public async Task<JsonResult> CheckEmail(string email)

Class members

The class member variable should be renamed from _context to context. The underscore is a relic of denoting private member variables. The better way of accessing members of a class is using the this qualifier (e.g. this.context).

Implicit types

As a side note, use the keyword var when declaring variables when the right hand side (rhs) of the assignment is obvious as it reduces repetition.

Email

I recommend using the System.Net.Mail.MailAddress class whenever you are using email addresses. During construction, it will check if it is a valid email address (by throwing an exception). This will allow us to access parts of an email (User@Host) and abstract the validation process (as previously you don't even check if the email passed is null, string.Empty, or even contains the @ symbol). It will also allow the display parts of an email ("John Doe"<john.doe+service@example.com> is a valid email).

try
{
    var anEmail = new MailAddress(email);
}
catch (ArgumentNullException ane)
{
    // email is null
}
catch (ArgumentException ae)
{
    // email is an empty string (string.Empty)
}
catch (FormatException fe)
{
    // email is not valid, see https://msdn.microsoft.com/en-us/library/system.net.mail.mailaddress.aspx#Remarks
}

Collections

Whenever you are working with collections, even Entity Framework sets, you should familiarize yourself with the Enumerable Methods and LINQ Expressions.

If we use MailAddress, we can use the Any method to see "whether any element of a sequence satisfies a condition". In this example, the host part of an email address matches any in the Organisations.EmailDomain (true if it matched, false otherwise):

if (this.context.Organisations.Any(o => o.EmailDomain == anEmail.Host))
{
    // code when Host matched
}

Logging

Since this method looks like it's part of a MVC controller, you should look into using a logging framework. There is a lot of information out there about logging and how to set them up, but here's a place to start: Introduction to Logging in ASP.NET Core

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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