Return to Answer

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.

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.

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));

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.