Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRITIQUE #1

Closed
vmcall opened this issue Dec 26, 2018 · 2 comments
Closed

CRITIQUE #1

vmcall opened this issue Dec 26, 2018 · 2 comments

Comments

@vmcall
Copy link

vmcall commented Dec 26, 2018

Here's some advice, as i 'noticed' you were new to C#:

This isn't C# specific, but i'd suggest splitting up functions like Strawpoll_Bot::Program::VoteRequest to prevent the over-indentation/nesting as seen in the respective function.

You should be using string interpolation to format strings instead of concating like here.

Eg:

$"http://2captcha.com/in.php?key={captchakey}&method=userrecaptcha&googlekey={sitekey}&invisible=1&pageurl={currenturl}"; 

I don't have any experience with 2captcha, only anti-captcha, but from the information on 2captchas site, it seems very overpriced (anti-captcha is $1 cheaper per 1000 solves). I've never had any issues with anti-captcha, anyway, and it was definitely solving faster than 30 seconds.

KeyValuePairs can be implicitly converted using { }, eg.

var formcontent = new FormUrlEncodedContent(new[]
{
     { "security-token", sectoken },
     { authtoken, "" },
     { "g-recaptcha-response", capresponse },
     { "options", option }
});

This is a really 'dirty' function that could be improved a lot (readability). I assume you have coded legacy c before? (You're defining a ton of variables at the top of the function where they are not actually needed. :)

You really should be using a proper html-parser like AgileHtml instead of splitting strings like you are doing here.

Don't do this or this, you're needlessly bottlenecking :)
Don't do this or this, you should be using the using statement to automatically dispose when objects leave scope.
Eg:

using (var voteclient = new HttpClient(handler))
{
    ...
}

Use TryParse instead of converting like this.

I'm not quite sure whether or not it's a problem, but your code style might be hard to read when the variable names get longer, as local variables do not have a distinct delimiter. I would suggest snake_case, but you can also just do partial CamelCase, eg. (camelCase) for local variables 👍 (You also are not completely consistent in your naming convention)

Code looks clean besides all of the above. You're not consistent in using var but that's not really an issue 🙉

@bditt
Copy link
Owner

bditt commented Dec 26, 2018

Here's some advice, as i 'noticed' you were new to C#:

This isn't C# specific, but i'd suggest splitting up functions like Strawpoll_Bot::Program::VoteRequest to prevent the over-indentation/nesting as seen in the respective function.

You should be using string interpolation to format strings instead of concating like here.

Eg:

$"http://2captcha.com/in.php?key={captchakey}&method=userrecaptcha&googlekey={sitekey}&invisible=1&pageurl={currenturl}"; 

I don't have any experience with 2captcha, only anti-captcha, but from the information on 2captchas site, it seems very overpriced (anti-captcha is $1 cheaper per 1000 solves). I've never had any issues with anti-captcha, anyway, and it was definitely solving faster than 30 seconds.

KeyValuePairs can be implicitly converted using { }, eg.

var formcontent = new FormUrlEncodedContent(new[]
{
     { "security-token", sectoken },
     { authtoken, "" },
     { "g-recaptcha-response", capresponse },
     { "options", option }
});

This is a really 'dirty' function that could be improved a lot (readability). I assume you have coded legacy c before? (You're defining a ton of variables at the top of the function where they are not actually needed. :)

You really should be using a proper html-parser like AgileHtml instead of splitting strings like you are doing here.

Don't do this or this, you're needlessly bottlenecking :)
Don't do this or this, you should be using the using statement to automatically dispose when objects leave scope.
Eg:

using (var voteclient = new HttpClient(handler))
{
    ...
}

Use TryParse instead of converting like this.

I'm not quite sure whether or not it's a problem, but your code style might be hard to read when the variable names get longer, as local variables do not have a distinct delimiter. I would suggest snake_case, but you can also just do partial CamelCase, eg. (camelCase) for local variables (You also are not completely consistent in your naming convention)

Code looks clean besides all of the above. You're not consistent in using var but that's not really an issue

Thank you for your input.
I really appreciate it.
I come from mainly working in C++ and C.

I didn't use a proper html-parser like AgileHtml mainly because I was trying to keep this very minimal in the dependencies department.

Is TryParse basically a try catch for conversion? So if the conversion fails it doesn't error?

You are right about my naming schemes. I have a really hard time coming up with names and just use what ever comes to mind at first.

As for the whole bottle neck part, I did that due to 2Captcha's terms stating that you can get a cooldown if you spam their API to much. While yes I could do a pingback, this seemed to be an easier way to go about it.

Also, I didn't go with anti-captcha mainly because of how much more work I would have needed to do use the anti-captcha api. 2Captcha is way simpler in my opinion. The pricing is alright for me for now, but now that you bring it up, I may switch over soon.

Does string interpolation really improve performance that much? I keep being told it is really good to do it and will save a lot of performance but I keep forgetting to do it.

Thanks for the information about the using statement, I can see how that will be very helpful/useful in the future.

Once again, thank you for your input. I do appreciate the feedback and will use it to improve my future project.
If you have any more suggestions/critiques, I'll be here to listen to them.

@vmcall
Copy link
Author

vmcall commented Dec 26, 2018

anti-captcha has an open-source library for C# 👍

@vmcall vmcall closed this as completed Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants