Conversation
mboiev
changed the title
| * @param string|null $message | ||
| */ | ||
| public function __construct( | ||
| ?int $numberCharacters = 1, |
There was a problem hiding this comment.
Can you validate that numberCharacters is a positive integer or zero?
Also, why did you make it accept null?
| */ | ||
| public function __construct( | ||
| ?int $numberCharacters = 1, | ||
| ?bool $strict = false, |
There was a problem hiding this comment.
Why did you make it accept null?
| if($this->message){ | ||
| return $this->message; | ||
| } | ||
| else if($this->strict){ |
There was a problem hiding this comment.
The conditional workflows rely on return statements.
No neeed for else and elseif -> change to if, if, return
| $lowerCase = strtolower($message); | ||
| $similar = similar_text($message, $lowerCase); | ||
| return strlen($message)-$similar; |
jwillp
left a comment
jwillp
left a comment
There was a problem hiding this comment.
Hi Mykhailo,
Please correct the issues I have mentioned in the comments.
| 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->countUpperCase($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 uppercase characters exceeds ".${$this->numberCharacters}; |
There was a problem hiding this comment.
The message here should be:
"The value '{$v}' should have exactly {$this->numberCharacters} upper cased characters"| if($this->strict){ | ||
| return "Number of uppercase characters exceeds ".${$this->numberCharacters}; | ||
| } | ||
| return "Number of uppercase 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} lower cased characters"| $ruleFirst = new ContainsUppercaseCharacters(1,true); | ||
| $ruleSecond = new ContainsUppercaseCharacters(1,false); | ||
| $ruleThird= new ContainsUppercaseCharacters(1,false,"Custom Message"); |
There was a problem hiding this comment.
It would be better to use different values for number of characters in the tests.
The value 1 causes some false positives here.
2 participants
No description provided.