Conversation
| * @param string|null $message | ||
| */ | ||
| public function __construct( | ||
| ?int $numberCharacters = 1, |
There was a problem hiding this comment.
Can you ensure that the value of the parameter is always a positive integer or zero?
| if($this->strict){ | ||
| return $this->countDigits($v)<=$this->numberCharacters; | ||
| } | ||
| else{ |
There was a problem hiding this comment.
Since the control flows rely on return, the else is not necessary.
| if($this->message){ | ||
| return $this->message; | ||
| } | ||
| else if($this->strict){ |
There was a problem hiding this comment.
Since the control flow (if, elseif, else) relies on return statements, all the else workflows are unecessary, you can replace with if, if and a direct return.
Co-authored-by: hellReuter_ <hellreuter@gmail.com>
| int $numberCharacters, | ||
| bool $strict, | ||
| ?string $message = null |
There was a problem hiding this comment.
Could you also set the default parameter values I had provided in the issue?
| ) | ||
| { | ||
| if($numberCharacters<0) | ||
| throw new InvalidArgumentException(); |
There was a problem hiding this comment.
When throwing an exception, a message should always be provided in order to inform the user why the exception was thrown without needing to look at the source code.
| public function validate($v): bool | ||
| { | ||
| if($this->strict){ | ||
| return $this->countDigits($v)<=$this->numberCharacters; |
There was a problem hiding this comment.
There is an error here. Can you find it?
| return $this->message; | ||
| } | ||
| if($this->strict){ | ||
| return "Number of numeric characters exceeds ".${$this->numberCharacters}; |
There was a problem hiding this comment.
The message here should be:
"The value '{$v}' should have exactly {$this->numberCharacters} numeric characters"| if($this->strict){ | ||
| return "Number of numeric characters exceeds ".${$this->numberCharacters}; | ||
| } | ||
| return "Number of numeric characters should exceed ".${$this->numberCharacters}; |
There was a problem hiding this comment.
The message here should be:
"The value '{$v}' should have at least {$this->numberCharacters} numeric characters"| public function testValidate(){ | ||
| $ruleFirst = new ContainsNumericCharacters(1,true); | ||
| $ruleSecond = new ContainsNumericCharacters(1,false); | ||
| $ruleThird= new ContainsNumericCharacters(1,false,"Custom Message"); |
There was a problem hiding this comment.
Testing with various values instead of only '1' would be better for test coverage.
2 participants
No description provided.