8
\$\begingroup\$

Linked: Google reCAPTCHA Validator: Iteration II


I have also created a simple Google Recaptcha Validation class to handle verification.

I used some code from CodingFusion's post Google New reCaptcha I am not a robot using asp .net, but I have altered it so that it fit my use case, and to make it extendable and reusable.

Can I clean it up further?

public class ReCaptchaValidator
{
    private readonly string _ReCaptchaSecret;
    private readonly string _ReCaptchaSiteKey;
    public List<string> ErrorCodes { get; set; }

    public ReCaptchaValidator(string reCaptchaSecret)
    {
        _ReCaptchaSecret = reCaptchaSecret;
        this.ErrorCodes = new List<string>();
    }
    public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
    {
        _ReCaptchaSecret = reCaptchaSecret;
        _ReCaptchaSiteKey = reCaptchaSiteKey;
        this.ErrorCodes = new List<string>();
    }

    public bool ValidateCaptcha(HttpRequest request)
    {
        var sb = new StringBuilder();
        sb.Append("https://www.google.com/recaptcha/api/siteverify?secret=");
        sb.Append(_ReCaptchaSecret);
        sb.Append("&response=");
        sb.Append(request.Form["g-recaptcha-response"]);

        //client ip address
        sb.Append("&remoteip=");
        sb.Append(GetUserIp(request));

        //make the api call and determine validity
        using (var client = new WebClient())
        {
            var uri = sb.ToString();
            var json = client.DownloadString(uri);
            var serializer = new DataContractJsonSerializer(typeof(RecaptchaApiResponse));
            var ms = new MemoryStream(Encoding.Unicode.GetBytes(json));
            var result = serializer.ReadObject(ms) as RecaptchaApiResponse;

            if (result == null)
            {
                return false;
            }
            else if (result.ErrorCodes != null)
            {
                foreach(var code in result.ErrorCodes)
                {                        
                    this.ErrorCodes.Add(code.ToString());
                }
                return false;
            }
            else if (!result.Success)
            {
                return false;
            }
            else //-- If successfully verified.
            {
                return true;
            }
        }
    }

    //--- To get user IP(Optional)
    private string GetUserIp(HttpRequest request)
    {
        var visitorsIpAddr = string.Empty;

        if (request.ServerVariables["HTTP_X_FORWARDED_FOR"] != null)
        {
            visitorsIpAddr = request.ServerVariables["HTTP_X_FORWARDED_FOR"];
        }
        else if (!string.IsNullOrEmpty(request.UserHostAddress))
        {
            visitorsIpAddr = request.UserHostAddress;
        }
        return visitorsIpAddr;
    }

}
[DataContract]
public class RecaptchaApiResponse
{
    [DataMember(Name = "success")]
    public bool Success;

    [DataMember(Name = "error-codes")]
    public List<string> ErrorCodes;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I never thought to use DataContracts for mine, that's a good write! \$\endgroup\$ – Der Kommissar Aug 11 '15 at 22:24
10
\$\begingroup\$
private readonly string _ReCaptchaSecret;
private readonly string _ReCaptchaSiteKey;

private fields are either lowerCamelCase or _lowerCamelCase, both conventions are common.


public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
    this.ErrorCodes = new List<string>();
}
public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
{
    _ReCaptchaSecret = reCaptchaSecret;
    _ReCaptchaSiteKey = reCaptchaSiteKey;
    this.ErrorCodes = new List<string>();
}

Don't repeat work in your constructors: use chaining instead.

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
    this.ErrorCodes = new List<string>();
}

public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey) 
    : this (reCaptchaSecret)
{
    _ReCaptchaSiteKey = reCaptchaSiteKey;
}

With C# 6, you can also use a property initializer to.. initialize.. your property.

public List<string> ErrorCodes { get; set; }

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
    this.ErrorCodes = new List<string>();
}

becomes

public List<string> ErrorCodes { get; set; } = new List<string>();

public ReCaptchaValidator(string reCaptchaSecret)
{
    _ReCaptchaSecret = reCaptchaSecret;
}

I would also advise to use a private setter for your ErrorCodes like this:

public List<string> ErrorCodes { get; private set; } = new List<string>();

Likewise, with C# 6, you might as well make it a readonly property:

public List<string> ErrorCodes { get; } = new List<string>();

StringBuilders see their most use when you're concatenating strings in a loop. In this scenario I would use a formatted string which turns this:

var sb = new StringBuilder();
sb.Append("https://www.google.com/recaptcha/api/siteverify?secret=");
sb.Append(_ReCaptchaSecret);
sb.Append("&response=");
sb.Append(request.Form["g-recaptcha-response"]);
sb.Append("&remoteip=");
sb.Append(GetUserIp(request));

into this:

var uri = string.Format("https://www.google.com/recaptcha/api/siteverify?secret={0}&response={1}&remoteip={2}",
    _ReCaptchaSecret, 
    request.Form["g-recaptcha-response"], 
    GetUserIp(request));

Or with C# 6:

var uri = $"https://www.google.com/recaptcha/api/siteverify?secret={_ReCaptchaSecret}&response={(request.Form["g-recaptcha-response"])}&remoteip={GetUserIp(request)}";

else if (!string.IsNullOrEmpty(request.UserHostAddress))

I prefer string.IsNullOrWhiteSpace because only rarely whitespace characters are considered good input.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I could not remember the chaining like this for some reason. I just read about it too! thank you! \$\endgroup\$ – Malachi Aug 12 '15 at 14:09
  • \$\begingroup\$ I was able to do this private readonly List<string> _errorCodes = new List<string>(); does that mean that I am running C# 6? \$\endgroup\$ – Malachi Aug 12 '15 at 16:37
  • 1
    \$\begingroup\$ No, not necessarily. That's a field, not a property. Fields can be instantiated inline, properties only can since C# 6. \$\endgroup\$ – Jeroen Vannevel Aug 12 '15 at 16:47
  • \$\begingroup\$ you need to escape your quotes in the interpolated string \$\endgroup\$ – Malachi Jan 16 '17 at 16:11
  • \$\begingroup\$ it appears that you need to have that expression inside of parenthesis. like so --> {(request.Form["g-recaptcha-response"])} via this Stack Overflow Answer \$\endgroup\$ – Malachi Jan 16 '17 at 19:44
6
\$\begingroup\$

I'm not a fan of properties in the form of:

public List<string> ErrorCodes { get; set; }

as someone can do this:

var validator = new ReCaptchaValidator("secretstuff");

validator.ErrorCodes = new List<string> { "Bogus error!" };

or some such. I might code it as such:

private readonly IList<string> _ErrorCodes = new List<string>();

public ReCaptchaValidator(string reCaptchaSecret) : this(reCaptchaSecret, null)
{
}

public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
{
    _ReCaptchaSecret = reCaptchaSecret;
    _ReCaptchaSiteKey = reCaptchaSiteKey;
}

public IEnumerable<string> ErrorCodes
{
    get
    {
        return new ReadOnlyCollection<string>(this._ErrorCodes);
    }
}

Then replace all your use of ErrorCodes with _ErrorCodes within the class.

| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$
        if (result == null)
        {
            return false;
        }
        else if (result.ErrorCodes != null)
        {
            foreach(var code in result.ErrorCodes)
            {                        
                this.ErrorCodes.Add(code.ToString());
            }
            return false;
        }
        else if (!result.Success)
        {
            return false;
        }
        else //-- If successfully verified.
        {
            return true;
        }  

Well this is ugly.

First the else is redundant, because for each of the other conditions you are returning a value hence the else will be reached only if the previous conditions aren't met.

The last else if is superfluous too, because you have something like this:

if(condition)
{
    return true;
}
else
{ 
    return true;
}  

which should be compacted to return condition;.

This will lead to:

        if (result == null)
        {
            return false;
        }
        else if (result.ErrorCodes != null)
        {
            foreach(var code in result.ErrorCodes)
            {                        
                this.ErrorCodes.Add(code.ToString());
            }
            return false;
        }

        return result.Success;

But hey, we can do one better.

  • because result.ErrorCodes is a List<string> there is no need to call ToString() on any item of this list.

  • because this.ErrorCodes is a List<string> too, we can take advantage of the AddRange() method.

This will lead to:

        if (result == null)
        {
            return false;
        }
        else if (result.ErrorCodes != null)
        {
            this.ErrorCodes.AddRange(result.ErrorCodes);
            return false;
        }

        return result.Success;
| improve this answer | |
\$\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.