Enhancement: Extract phpweb\Spam\Challenge
#664
Conversation
localheinz
force-pushed
the
feature/spam-challenge
branch
from
a076df1 to
dedb915
Compare
There was a problem hiding this comment.
I love this. It helps with the readability a lot.
I am not a fan of static methods though. They make sense for create and isValidAnswer but everything else doesn't really need to be a public static method. I believe that we should test all public method with a dedicated test if possible. In this case, we don't access these methods from anywhere else outside the scope of the class.
I would also encourage you to specify all parameter and return types. This should also allow you to change $strict to true.
src/Spam/Challenge.php
Outdated
| $a = rand(0, 9); | ||
| $argumentOne = self::NUM[$a]; |
There was a problem hiding this comment.
One-letter variable names aren't very good. Would you be able to come up with more descriptive name?
There was a problem hiding this comment.
I think it's okay here (besides the variables are local, they're only used in the next statement). Although, we could inline the rand() call, and drop $a altogether (probably not a good for $b below).
src/Spam/Challenge.php
Outdated
| public static function plus($a, $b) { | ||
| return $a + $b; | ||
| } | ||
|
|
||
| public static function gen_plus($a) { | ||
| return rand(0, 9 - $a); | ||
| } | ||
|
|
||
| public static function minus($a, $b) { | ||
| return $a - $b; | ||
| } | ||
|
|
||
| public static function gen_minus($a) { | ||
| return rand(0, $a); | ||
| } | ||
|
|
||
| public static function print_infix($name, $a, $b) { | ||
| return "$a $name $b"; | ||
| } | ||
|
|
||
| public static function print_prefix($name, $a, $b) { | ||
| return "$name($a, $b)"; | ||
| } |
There was a problem hiding this comment.
These do not need to be public or even static (except minus and plus). Could you make them all instance methods and then call them from the getters or from the constructor?
localheinz
force-pushed
the
feature/spam-challenge
branch
8 times, most recently
from
22bb34e to
05aef42
Compare
localheinz
force-pushed
the
feature/spam-challenge
branch
from
05aef42 to
a88faf2
Compare
|
🚀 Deployed on https://web-php-pr-664.preview.thephp.foundation |
This pull request
phpweb\Spam\Challenge