Justin Patrin [2006-10-16 18:33 UTC] What's the point of the following code in doSubtract?
if ($this->firstNumber < $this->secondNumber) {
$this->setFirstNumber();
$this->generateOperation();
}
This doesn't ensure that firstNumber is less than secondNumber as the newly generated number could be less. If this code is needed you should make that while, not if.
I think setFirstNumber, setSecondNumber, setOperator, and setOperation should have their names changed as they are not setters, but generators. generateFirstNumber and so on would be more appropriate I think.
Peter Beckman [2006-10-16 18:40 UTC] Where are the .phpt or PHPUnit regression tests? And the End-User documentation?
David Coallier [2006-10-16 19:07 UTC] Justing:
'This doesn't ensure that firstNumber is less than secondNumber as the 'newly generated number could be less. If this code is needed you 'should make that while, not if.
Nop, but that ensures the firstNumber is always bigger than the second number.
And yes I do agree that they are generators but they are also setters as they are setting the variables, perhaps "setGeneratedFirstNumber()" but I think this is too long and useless.
Peter, if you want end user documentation, you can run phpdoc on it, the comments are phpdoc compliant and a package doens't need end user documentation in order to be accepted. The end user doc will be online when the package is in cvs, it will be on pear.php.net as the other packages. I will put up a couple phpt's later on today.
itrebal [2006-10-16 19:29 UTC] [snip]
private function doSubstract()
{
/**
* Check if firstNumber is smaller than secondNumber
*/
if ($this->firstNumber < $this->secondNumber) {
$this->setFirstNumber();
$this->generateOperation();
}
$answer = $this->firstNumber - $this->secondNumber;
$this->setAnswer($answer);
}
I would suggest instead of continuing on with the function, as the rest of the code is not required, return'ing or using an if/else statement. Reducing the overhead of useless code being run.
David Coallier [2006-10-16 20:08 UTC] You are right Graham, I have updated the package and updated the online source.
Thanks
Christian Weiske [2006-10-16 20:53 UTC] I implemented this driver half a year ago and sent it to the Text_Captcha maintainer. He promised to add it, but apparently never did.
David Coallier [2006-10-16 20:57 UTC] Well the package not being in cvs.php.net's repository that's already a problem, although, if this package gets accepted, I'll just put it there, it's pretty straightforward and works well. Karma is already taken care of, so this should be quick if it ever happens.
Justin Patrin [2006-10-16 20:59 UTC] 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.
Christian Weiske [2006-10-16 21:00 UTC] The files are:
http://xml.cweiske.de/Figlet.phps
http://xml.cweiske.de/TextEquation.phps
http://xml.cweiske.de/text_captcha_equation.phps
http://xml.cweiske.de/text_captcha_figlet.phps
Lorenzo Alberton [2006-10-16 21:07 UTC] maybe I'm being naive, but in this case:
if ($this->firstNumber < $this->secondNumber) {
//here
}
can't you just *swap* the two numbers instead of generating them over and over till the condition is true?
David Coallier [2006-10-16 21:19 UTC] Ok, I thought about it and it does make sense that those are not setters, and really generators. I have changed this in the code and I have used get*Number() to retrieve my values that is true that I went a little too fast. I am updating now, and the md5sums are also wrong btw.
David Coallier [2006-10-16 22:18 UTC] I have made it use
setFirstNumber()
setSecondNumber()
changed the set to generateFirstNumber() which calls setFirstNumber passing generateNumber()
same goes for generateSecondNumber() -> setSecondNumber(generateNumber())
Although, the while is impossible in that case, I have to re-call the generateOperation if I change the firstNumber in any matter. I have made calculations generating + and - using array_rand, I have tested to see if it would ever create a seg fault and I haven't seen it yet.
Let me know if you find it more clean now.
Justin Patrin [2006-10-16 23:23 UTC] I still don't understand why you're making recursive calls. As Lorenzo said, just swap the 2 numbers. Much simpler. There's no reason for this to be recursive.
if ($this->firstNumber < $this->secondNumber) {
$tmp = $this->getFirstNumber();
$this->setFirstNumber($this->getSecondNumber());
$this->setSecondNumber($tmp);
}
Firman Wandayandi [2006-10-16 23:42 UTC] I agreed with Justin, simply just swap it.
Also a bit change, on method setAnswer(). It's unnecessary needed to cast a value into integer, because it was already integer and would be always integer.
$this->answer = $answerValue;
David Coallier [2006-10-17 00:56 UTC] Justin:
Hehe thanks for teaching me how to make a temporary variable.. anywoo, I have made it without calling generateOperation now, I only setOperation() on a substract and stick the first value into the second one and the second one into the first one.
Firman:
Thanks for your reply and I have removed the second casting, you are right firstNumber and secondNumber are already casted as integer, thus shouldn't be different.
Also, I have updated the package to version 0.1.4 on the site (alpha) and updated the http://dev.agoraproduction.com/pear/Numeral/?file=full
Let me know if that fits more the kind of thing you want now.
Thanks
David Coallier [2006-10-17 13:56 UTC] Firman and I were discussing on IRC and we thought about making this a standalone package instead of a driver to Text_CAPTCHA, first, we would gain in speed and would be easier for everyone to install..
The only dep they need is php5 and that's about it.
We were thinking of the names:
Text_CAPTCHA_Numeral
Numeral_CAPTCHA
Which do you think would be best and would it be better with you guys if it wouldn't be a Text_CAPTCHA driver (since Text_CAPTCHA) is not even in the php's repository.
David Coallier [2006-10-17 15:08 UTC] Ok, I have now made it it's own package, no more a driver of Text_CAPTCHA, I have discussed it with couple people and this could rather be it's own package and later on perhaps doing another package "CAPTCHA" that would have the driver Text_CAPTCHA_Image and Text_CAPTCHA_Numeral but not for now.
Right now, since we wanted to increase speed, we are just using one file instead of instantiating thru the Text_CAPTCHA factory.
The package is updated and the examples are as well.
|
