3
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$
  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:

\$\endgroup\$
4
  • \$\begingroup\$ 1. True 2. if (!$captcha->check()) { } Will only be executed If the captcha isn't correctly entered, thus If I enter a correct code, it would not be FALSE = thus the condition wouldn't be executed, I am quite sure it's opposite way, but for more convenience I'll add the return true at the bottom. 3. Where can I find that reference? 4. Sure. But what exactly do I need to validate? \$\endgroup\$ – josmith Jun 17 '12 at 10:31
  • \$\begingroup\$ 3. It's a book, I don't know any online resource for that but here is similar article: Temporal Coupling in Java and Some Solutions \$\endgroup\$ – palacsint Jun 18 '12 at 16:36
  • \$\begingroup\$ @josmith: I've removed the 4th issue from the answer because I missed the array keyword. Sorry for the inconvenience. \$\endgroup\$ – palacsint Jun 18 '12 at 16:59
  • 1
    \$\begingroup\$ +1 just for the reference to "Clean Code". That book is worth a read for sure. \$\endgroup\$ – Ingo Bürk Sep 7 '14 at 18:13
2
\$\begingroup\$

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;
    }

}
\$\endgroup\$
4
  • \$\begingroup\$ Include: How about If I Include the File somewhere else and not in the class like in a bootstrap file? check() - I guess so, but I'd make the OPTIONAL parameters, agreed? I'll have a look over the user input... How should I validate the config array? Suggested design: Thanks for your contribution, I'll make some changes. \$\endgroup\$ – josmith Jun 17 '12 at 10:36
  • \$\begingroup\$ Added a snippet on validation, the question is How much validation do I need to do? Have a look on updated question. \$\endgroup\$ – josmith Jun 17 '12 at 10:58
  • \$\begingroup\$ @josmith It's fine to include the file elsewhere, as long as you can be certain that the code has been loaded by the time the check or render methods are called. As for the check params, I would probably leave them non-optional. I like to separate objects from their data source. Leaving the extraction from post outside of the object makes it much more explicit. And, it seems more logical to group that data extraction with the rest of your data extraction. \$\endgroup\$ – Corbin Jun 17 '12 at 18:22
  • \$\begingroup\$ Also, leaving anything $_POST related outside of the class creates a better, less coupled class, but allowing for defaults would certainly allow easier use of the object, and ease of use is part of design after all. (Have to go at the moment, but will address validation later -- the short answer is that you need to be 100% certain that the object is in a usable state.) \$\endgroup\$ – Corbin Jun 17 '12 at 18:23

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.