2Captcha API wrapper

Question

So I've created an API wrapper for 2captcha in c# and I wonder what I can do do improve the code, make it cleaner?

Code:

using System;
using System.IO;
using System.Net;
using System.Text;
using System.Threading;

namespace API2Captcha
{
    public class TwoCaptchaApi
    {
        string key = null;
        public string captchaId;
        public TwoCaptchaApi(string key)
        {
            this.key = key;
        }

        public float getBalance()
        {
            string response = "";

            using (WebClient client = new WebClient())
            {
                client.QueryString.Add("key", key);
                client.QueryString.Add("action", "getbalance");
                response = client.DownloadString(settings.url_response);
            }


            float balance = -1;

            if (!float.TryParse(response, out balance))
            {
                throw new Exception($"2Captcha - Error while checking balance: {response}");

            }
            return balance;



        }

        public string solveReCaptcha(string googleKey, string pageUrl)
        {
            captchaId = sendReCaptcha(googleKey, pageUrl);
            Thread.Sleep(15 * 1000);


            return getResult(captchaId);

        }

        private string sendReCaptcha(string googleKey, string pageUrl)
        {
            string response = "";
            using (WebClient client = new WebClient())
            {
                client.QueryString.Add("key", key);
                client.QueryString.Add("method", "userrecaptcha");
                client.QueryString.Add("googlekey", googleKey);
                client.QueryString.Add("pageurl", pageUrl);
                response = client.DownloadString(settings.url_request);
            }

            if (response.Substring(0, 3) != "OK|")
                throw new Exception($"Captcha sending error: {response}");

            return response.Remove(0, 3);
        }
        public string solveCaptcha(string path)
        {
            captchaId = uploadCaptcha(path);
            Thread.Sleep(10 * 1000);
            return getResult(captchaId);


        }
        private string uploadCaptcha(string path)
        {
            if (!File.Exists(path))
            {
                throw new Exception("File doesn't exist");
            }
            byte[] image = File.ReadAllBytes(path);
            string response = "";
            using (WebClient client = new WebClient())
            {

                client.QueryString.Add("key", key);
                response = Encoding.Default.GetString(client.UploadFile(settings.url_request, path));
            }

            if (response.Substring(0, 3) != "OK|")
            {
                throw new Exception($"Captcha uploading error: {response}");
            }
            return response.Remove(0, 3);




        }
        public string getResult(string captchaId)
        {
            string response = "";
            for (int i = 0; i <= 10; i++)
            {
                using (WebClient client = new WebClient())
                {
                    client.QueryString.Add("key", key);
                    client.QueryString.Add("action", "get");

                    client.QueryString.Add("id", captchaId);
                    //   string url = String.Format("{0}?key={1}&action=get&id={2}", settings.url_response, key, captchaId);
                    response = client.DownloadString(settings.url_response);
                }

                if (response.Substring(0, 3) == "OK|")
                {
                    return response.Remove(0, 3);
                }
                else if (response.Contains("ERROR"))
                {
                    throw new Exception($"Captcha solve error: {response}");
                }
                Thread.Sleep(5 * 1000);
            }


            throw new Exception($"Captcha solve error: {response}");
        }


        public bool reportBadCaptcha(string captchaId)
        {
            string response = "";
            using (WebClient client = new WebClient())
            {
                client.QueryString.Add("key", key);
                client.QueryString.Add("action", "reportbad");
                client.QueryString.Add("id", captchaId);

                response = client.DownloadString(settings.url_response);
            }

            if (response.Contains("OK_REPORT_RECORDED"))
            {
                return true;
            }
            return false;


        }

    }


    static class settings
    {
        public const string url_request = "http://2captcha.com/in.php";
        public const string url_response = "http://2captcha.com/res.php";
    }
}

Answer 1

1) There are irritating new-lines everywhere (or missing), please format your code consistently.

2) The most important: make your API async, otherwise you are blocking threads with Thread.Sleep. But then all methods must be async, so here is just an example:

public async Task<CaptchaResponse> SolveCaptcha(string path)
{
    captchaId = await uploadCaptcha(path);
    await Task.Delay(TimeSpan.FromSeconds(10));
    return getResult(captchaId);
}

3) The WebClient always uses the "key" in the Query-String. Just wrap the creation of the WebClient in a helper method that initializes the "key".

private WebClient GetApiWebClient()
{
    WebClient client = new WebClient();
    client.QueryString.Add("key", key);
    return client;
}

and then

public async Task<bool> ReportBadCaptcha()
{
    using (WebClient client = GetApiWebClient())
    {
        client.QueryString.Add("action", "reportbad");
        client.QueryString.Add("id", captchaId);

        string response = await client.DownloadStringTaskAsync(settings.url_response);
        return response.Contains("OK_REPORT_RECORDED");
    }
}

4) Use TimeSpan-Methods instead of multiplying milliseconds, makes it more readable:

Thread.Sleep(TimeSpan.FromSeconds(10));

5) Extract the hard-coded waits and retry-loops into a Configuration object so that the user can specify the timeouts and number of retries.

6) Convert the response from a string to an enum for easier handling:

public enum CaptchaResponse
{
    Unknown,
    Ok,
    Error,
    CaptchaNotReady
}

private static CaptchaResponse ConvertResponse(string response, string exceptionMessage)
{
    string status = response.Split('|')[0];

    switch (status)
    {
        case "OK":
            return CaptchaResponse.Ok;
        case "ERROR":
            return CaptchaResponse.Error;
        case "CAPCHA_NOT_READY":
            return CaptchaResponse.CaptchaNotReady;
        default:
            throw new CaptchaException($"{exceptionMessage}: {response}");
    }
}

7) Why do you need byte[] image = File.ReadAllBytes(path) ?

8) settings should be called Settings and you shouldn't use underscores in the constants, better use CamelCase (UrlRequest and UrlResponse).

Answer 2

There are few things you could improve.

---

string key = null;
public string captchaId;

Public mutable fields are a no-go in C#. Use properties instead or make them private.

---

public float getBalance()

• Public members should have PascalCase.
• You can do everything inside the using to avoid the variables outside of it unless you wrap it with a try/catch then you'll need to keep it at the top to be able to pass the response to the custom exception.
• If you can use C#7 you don't need the helper variable.
• You should not throw the Exception but a more specific one like BalanceException with an inner exception. You can pass the response to it and implement the message there.

Example:

public float GetBalance()
{
    var response = default(string);
    try
    {
        using (var client = new WebClient())
        {
            client.QueryString.Add("key", key);
            client.QueryString.Add("action", "getbalance");
            var response = client.DownloadString(settings.url_response);        
            return float.Parse(response);
        }
    }
    catch(Exception ex)
    {
        throw new BalanceException(response, ex);
    }
}

These suggestions apply to almost all of your methods and you can improve them all in a similar way.

---

if (response.Substring(0, 3) != "OK|")

Try not to use magic values like the Substring. It's better to create a helper variable for it so that you know what it was in few weeks or sometimes even only days:

 var httpResult = response.Substring(0, 3);

---

return response.Remove(0, 3);

Operations like this one should be encapsulated. In few days nobody will know what it actually does. Try something like this:

private string ExtractSomeData(..) {..}

or

private string RemoveHttpResult(..) {..}

whatever makes more sense for you.

You perform both operations multiple times. This means it's about time to think about encapsulating them.

---

if (response.Contains("OK_REPORT_RECORDED"))
{
    return true;
}
return false;

This is the same as

 return response.Contains("OK_REPORT_RECORDED");

---

 public string getResult(string captchaId)

The intention of some of your methods isn't clear. This one for example requires a captchaId but you have a field with the same name. If it should use the parameter then you should not have the field and vice versa. It's a little bit confusing. Some methods use the captchaId whereas others don't.