♻️ Remove redundant return statements #266
Conversation
| else | ||
| super | ||
| end | ||
| super |
There was a problem hiding this comment.
Former construction was odd, since it could be mistakenly read as calling super twice. Keeping super in the else statement prevents this misreading.
No behavior change.
| else | ||
| [] | ||
| end | ||
| return [] |
There was a problem hiding this comment.
This allows a lack of clarity around potential fall through of unhandled cases. Using the explicit else makes the code more declarative.
No behavior change.
| true | ||
| when :start_element | ||
| return true if event[1] == @event_arg | ||
| event[1] == @event_arg |
There was a problem hiding this comment.
This is a slight behavior change, in that this would previously return nil when the condition was not met, since it would exit the case statement without returning early, and exit the method without any explicit return, thus defaulting to void / nil return value.
This seems like it was unintentional, and thankfully, since this method appears to be intended as a predicate method, which should always return true or false, callers are likely using the boolean-ish result, where nil is falsey.
Returning actual false here then should be backwards compatible with the explicit intent of the ? predicate method.
| true | ||
| when :start_attribute | ||
| return true if event[1] == @event_arg | ||
| event[1] == @event_arg |
There was a problem hiding this comment.
Same as above comment.
Returning actual false here, instead of nil after falling out of the case statement, should be backwards compatible with the explicit intent of the ? predicate method.
|
Thanks. |
2 participants
Split from #261; expanded and improved.
REXML::Valdiation::Event#matches?, which is to align the predicate method's return value with the expected behavior of a predicate method (which is to return one of true or false). See comments for more details.