Google reCAPTCHA Validator: Iteration III

Question

This is the third iteration of my reCAPTCHA validator. Iteration II can be found at: Google reCAPTCHA Validator: Iteration II

It does almost everything required to do reCAPTCHA validation on anything .NET.

I removed all the XML docs to shorten it up a bit.

GitHub commit of the version in this thread (with all the docs added that I omitted here): ReCaptchaValidator.cs

The only comments I do not wish to hear of are any comments relating to my use of fully-qualifying static/const members. I do this primarily to remind myself of where they originate.

using System;
using System.Collections.Generic;
using System.Collections.Specialized;
using System.Linq;
using System.Net;
using System.Web;
using System.Web.Script.Serialization;
namespace Evbpc.Framework.Integrations.Google
{
 public class ReCaptchaValidator
 {
 private const string _headScriptInclude = "<script src='https://www.google.com/recaptcha/api.js'></script>";
 private const string _bodyDivInclude = "<div class=\"g-recaptcha %EXTRACLASSES%\" data-sitekey=\"%SITEKEY%\"></div>";
 private const string _reCaptchaFormCode = "g-recaptcha-response";
 private const string _googleApiEndpoint = "https://www.google.com/recaptcha/api/siteverify";
 private readonly string _reCaptchaSecret;
 private readonly string _reCaptchaSiteKey;
 private readonly List<string> _extraClasses = new List<string>();
 public string HeadScriptInclude => ReCaptchaValidator._headScriptInclude;
 public List<string> ExtraClasses => _extraClasses;
 public string BodyDivInclude => GetBodyDivContent(this.ExtraClasses);
 public ReCaptchaValidator(string reCaptchaSecret, string reCaptchaSiteKey)
 {
 _reCaptchaSecret = reCaptchaSecret;
 _reCaptchaSiteKey = reCaptchaSiteKey;
 }
 public ReCaptchaResponse Validate(NameValueCollection form, string remoteIp = null)
 {
 string reCaptchaSecret = _reCaptchaSecret;
 string reCaptchaResponse = form[ReCaptchaValidator._reCaptchaFormCode];
 using (WebClient client = new WebClient())
 {
 NameValueCollection postParameters = new NameValueCollection() { { "secret", reCaptchaSecret }, { "response", reCaptchaResponse } };
 if (remoteIp != null)
 {
 postParameters.Add("remoteip", remoteIp);
 }
 byte[] response = client.UploadValues(ReCaptchaValidator._googleApiEndpoint, postParameters);
 string reCaptchaResult = System.Text.Encoding.UTF8.GetString(response);
 ReCaptchaResponse result = new ReCaptchaResponse();
 if (result.ParseJson(reCaptchaResult))
 {
 return result;
 }
 return null;
 }
 }
 public string GetBodyDivContent(List<string> extraClasses)
 {
 string result = ReCaptchaValidator._bodyDivInclude;
 result = result.Replace("%SITEKEY%", _reCaptchaSiteKey);
 if (extraClasses != null)
 {
 result = result.Replace("%EXTRACLASSES%", string.Join(" ", extraClasses));
 }
 return result;
 }
 }
 public class ReCaptchaResponse
 {
 private bool _success = false;
 private ReCaptchaErrors _errors = ReCaptchaErrors.None;
 public bool Success => _success; 
 public ReCaptchaErrors Errors => _errors;
 public bool ParseJson(string jsonResponse)
 {
 JavaScriptSerializer jss = new JavaScriptSerializer();
 dynamic deserializedJson = jss.DeserializeObject(jsonResponse);
 if (deserializedJson.ContainsKey("success"))
 {
 _success = deserializedJson["success"];
 _errors = ReCaptchaErrors.None;
 if (deserializedJson.ContainsKey("error-codes"))
 {
 foreach (string error in deserializedJson["error-codes"])
 {
 // Our `ReCaptchaErrors` enum contains the exact same strings as the returned `error` text would be, with the following transformations:
 // 1. The words are transformed to PascalCase;
 // 2. The dashes are stripped;
 string[] errorWords = error.Split('-');
 string errorEnumName = "";
 foreach (string errorWord in errorWords)
 {
 errorEnumName += errorWord[0].ToString().ToUpper() + errorWord.Substring(1);
 }
 if (Enum.IsDefined(typeof(ReCaptchaErrors), errorEnumName))
 {
 _errors = _errors | (ReCaptchaErrors)Enum.Parse(typeof(ReCaptchaErrors), errorEnumName);
 }
 else
 {
 return false;
 }
 }
 }
 }
 else
 {
 return false;
 }
 return true;
 }
 }
 [Flags]
 public enum ReCaptchaErrors
 {
 None = 0x00,
 MissingInputSecret = 1 << 0,
 InvalidInputSecret = 1 << 1,
 MissingInputResponse = 1 << 2,
 InvalidInputResponse = 1 << 3,
 }
}

Yes, this is the first question I have asked here with all the braces included.

This is part of an open-source library I have which, as usual, anyone is free to use.

Answer 1

The strings "success" and "error-codes" should be const string.

---

Why is the construction of postParameters inside using (WebClient client = new WebClient())? On that same topic: considering client isn't used after it has filled response, why keep it open? Basically, only response = client.UploadValues(ReCaptchaValidator._googleApiEndpoint, postParameters); should be inside using (WebClient client = new WebClient()) (with byte[] response defined outside of it).

You also don't need the empty argument at the end of NameValueCollection().

---

Can remoteIp be an empty string? Otherwise, why not use string.IsNullOrEmpty instead of if (remoteIp != null)?

---

There's no need to do this:

string result = ReCaptchaValidator._bodyDivInclude;
result = result.Replace("%SITEKEY%", _reCaptchaSiteKey);

Just do it in one line:

string result = ReCaptchaValidator._bodyDivInclude.Replace("%SITEKEY%", _reCaptchaSiteKey);

---

I'm not a fan of an enum with a plural as a name: ReCaptchaErrors. Especially since you then named the field _errors, which suggests a collection. Same for Errors.

---

Why do you use JavaScriptSerializer and not Json.NET?

---

You can reduce the indentation by re-thinkign your if-else blocks:

if (!deserializedJson.ContainsKey("success"))
{
 return false;
}
_success = deserializedJson["success"];
_errors = ReCaptchaErrors.None;
if (!deserializedJson.ContainsKey("error-codes"))
{
 return true;
}

---

It's customary to use string.Empty instead of "": string errorEnumName = "";.

---

Move errorWord[0].ToString().ToUpper() + errorWord.Substring(1); to a method called Capitalize and rewrite it slightly:

private string Capitalize(string s)
{
 if (string.IsNullOrEmpty(s))
 {
 return string.Empty;
 }
 return char.ToUpper(s[0]) + s.Substring(1);
}

That way your for loop can become:

string errorEnumName = errorWords.Aggregate(string.Empty, (current, errorWord) => current + Capitalize(errorWord));

---

I would consider converting deserializedJson to a case insensitive Dictionary<K, V> to avoid the foreach. Also, that way you can use TryGetValue.

• Der Kommissar: I ended up renaming Capitalize to ToPascalCase, made it an extension method, and put it in a different class. :)
• Dan Lyons: Re: the plural enum name - the Enumeration Type Naming Guidelines suggest this is correct, since it is a flags enum.

Answer 2

Just one quick comment:

ReCaptchaResponse result = new ReCaptchaResponse();
if (result.ParseJson(reCaptchaResult))
{
 return result;
}
return null;

This would work better as static method following the familiar TryParse pattern:

ReCaptchaResponse result;
if (ReCaptchaResponse.TryParseJson(reCaptchaResult, out result))
{
 return result;
}
return null;

Answer 3

Some changes I made (after noticing a few issues with it):

• ExtraClasses has no need to be a field and a property; replaced both with public List<string> ExtraClasses { get; } = new List<string>();.
• Since GetBodyDivContent is not static, I made removed extraClasses from the parameter, and made it directly used in the method.
• I extracted a lot of Validate out to other protected methods.
• I replaced ParseJson with TryParseJson as per RobH's answer.

Version as of this answer: ReCaptchaValidator.cs

• BCdotWEB: That looks a lot nicer, yes. Invalidates plenty of my review, though. ;-)
• Der Kommissar: @BCdotWEB Although I am making substantial changes as a result of your review still. :)