[PEPr] Comment on Text::Text_CAPTCHA_Driver_Numeral

Justin Patrin (http://pear.php.net/user/justinpatrin) has commented on the proposal for Text::Text_CAPTCHA_Driver_Numeral. Comment: Sorry, my comment was incorrect. I do see now that it does cause the first number to be more than the second number. However, this is the wrong way to do such a thing. This will cause recursive calls of generateOperation and doSubtract. This just isn't the right way to do things, however. First of all, this will cause the call stack to be incremented by 2 every time this fails and could cause an out-of-memory error before it finds a firstNumber > secondNumber (not likely, but possible). Second, the code beneath this if will be called a number of times. To see it happen put an echo in setAnswer. It looks like this all only works by accident as a simple re-generation of the operator (setOperator) in generateOperation would have caused incorrect answers to be set. I suggest you change doSubtract to: private function doSubstract() { /** * Check if firstNumber is smaller than secondNumber */ while ($this->getFirstNumber() < $this->getSecondNumber()) { $this->setFirstNumber(); } $answer = $this->getFirstNumber() - $this->getSecondNumber(); $this->setAnswer($answer); } As this is much cleaner. Also notice I switched to the getter functions above. Generally if you have getters and setters the internal code should also be using them. I also see that these set* functions are private, which means that you're more free to choose your own names, but I still don't like the set* names. They mean that they are setter functions to me. Setter functions are functions which allow you to set a value. I understand that these do set a value, but they don't allow the caller to set a value. For them to be true setters they would be basically the invrse of the getters (such as setFirstNumber($num)). If you want to keep the set* names I would suggest adding a parameter to allow manual setting of the values and have it do the current thing (random setting) if nothing is passed in. Also, API docs (docblocks) are not end-user docs. Then again, a proposal doesn't need end-user docs. A stable release is what is supposed to have end-user docs. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=451 -- Sent by PEPr, the automatic proposal system at http://pear.php.net