HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

A small extension to the reCaptcha library

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
therecaptchaextensionsmalllibrary

Problem

I have been writing a small extension to the reCaptcha library.

class Captcha
{
    private $config;

    public $error;

    public function __construct(array $config)
    {
        include './library/recaptchalib.php';
        $this->config = $config;
    }

    public function render($error = null, $use_ssl = false)
    {
        return recaptcha_get_html($this->config['recaptcha']['public_key'], 
            $error, $use_ssl);
    }

    public function check()
    {
        $resp = recaptcha_check_answer ($this->config['recaptcha']['private_key'],
                                    $_SERVER["REMOTE_ADDR"],
                                    $_POST["recaptcha_challenge_field"],
                                    $_POST["recaptcha_response_field"]);
        if (!$resp->is_valid) {
            $this->error = $resp->error;
            return false;
        }
    }
}


What do you think? Is it anything to have? Anything I can improve/change?

Solution

-
The $error field should be private (Why use getters and setters?). Currently users of the Captcha class can modify the value of the $error field directly which. It's not a good idea to allow them to do this.

-
What's the point of returning with false in the check function when the response is not valid while if it's valid the function does not return with true?

-
The render and check functions seems somehow temporarily coupled. See: Clean Code (by Robert C. Martin), G31: Hidden Temporal Couplings

References:

  • Effective Java Second Edition, Item 38: Check parameters for validity (I know that this is a Java book but this chapter applicable to PHP too.)



  • Function/method argument validation standards

Context

StackExchange Code Review Q#12636, answer score: 3

Revisions (0)

No revisions yet.