A small extension to the reCaptcha library

Question

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?

Answer 1

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

2. 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?

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

Answer 2

include

Every time an object is created the recaptcha stuff is included. This means that if you try to make two objects, you're going to get some very nasty function already defined errors. include the recaptcha stuff at a file level instead of in the constructor. Also, consider using require_once. require because the file is required, and once because you have no way of knowing if it's already been included by any other code (though avoiding *_once is sometimes a good idea).

check()

Pass in the parameters instead of coupling the class to POST.

What if you want the fields to be named something else? What if by some weird chance you want to do it on an arbitrary array instead of $_POST? I would probably even take in the $_SERVER['REMOTE_ADDR'] just for completeness.

Never assume user input exists

I'm too lazy to explain it for the 3rd time in 2 days, so read the last section of this.

Don't bother with the config array

It's a lot more transparent if you just take in what you need explicitly. If you do end up taking in an array, then do like palacsint said and validate it.

Suggested design

By no means am I saying that this is perfect; this is just the approach I would most likely take.

<?php

//Having it in the include path means you don't have to depend on the library folder being in the right place
//(it can, however, mean more disk IO)
require_once 'recaptchalib.php';

class Captcha
{

    private $_publicKey;
    private $_privateKey;
    private $_ssl;

    private $_error = null;

    //(it's just a habit of mine to use $_ for private/protected)

    public function __construct($publicKey, $privateKey, $ssl = false)
    {
        $this->_publicKey = $publicKey;
        $this->_privateKey = $privateKey;
    }

    public function render()
    {
        return recaptcha_get_html($this->_publicKey, $this->_error, $this->_ssl);
    }

    public function check($challenge, $response, $ipAddr = null)
    {

        if ($ipAddr === null) {
            $ipAddr = $_SERVER['REMOTE_ADDR'];
        }

        $resp = recaptcha_check_answer ($this->_privateKey, $ipAddr, $challenge, $response);

        if (!$resp->is_valid) {
            $this->_error = $resp->error;
            return false;
        }

        return true;

    }

    public function getError()
    {
        return $this->_error;
    }

}