1
\$\begingroup\$

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";
    }
}
\$\endgroup\$
1
\$\begingroup\$

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

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your response, I think I've improved my code a little, however I wasn't able to use your GetBalance() with response variable inside the using, here is a preview: github.com/Mike97M/2Captcha-API/blob/master/API2Captcha.cs Do you have any other suggestions? \$\endgroup\$ – Mike Apr 27 '17 at 16:20
  • \$\begingroup\$ @Mike oh, you're right, now with the exception it must remain at the top so that the response can be passed to it. My bad, sorry. \$\endgroup\$ – t3chb0t Apr 27 '17 at 16:22
  • \$\begingroup\$ Yes, do you have other suggestions? \$\endgroup\$ – Mike Apr 27 '17 at 19:39

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.