url: refactor URLPattern::{Exec,Test}#63667
Conversation
Refactor these `URLPattern` methods and extract common logic into a helper function, reducing code duplication. Signed-off-by: Tobias Nießen <[email protected]>
tniessen
added
c++
nodejs-github-bot
added
needs-ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63667 +/- ##
==========================================
+ Coverage 90.32% 90.33% +0.01%
==========================================
Files 732 732
Lines 236435 236428 -7
Branches 44527 44519 -8
==========================================
+ Hits 213563 213589 +26
+ Misses 14589 14541 -48
- Partials 8283 8298 +15
🚀 New features to boost your workflow:
|
| THROW_ERR_OPERATION_FAILED(env, "Failed to exec URLPattern"); | ||
| return; | ||
| R result{}; | ||
| if (((url_pattern->*func)(env, input, baseURL_opt).*to_r)(&result)) { |
There was a problem hiding this comment.
I don't think this is readable at all.
There was a problem hiding this comment.
It uses relatively uncommon but still pretty normal C++ operators – but other than that, is there a specific issue here?
There was a problem hiding this comment.
IMHO, the benefit is not worth the cognitive effort this new addition requires for just understanding it. We should make the code more readable.
| template <typename R, | ||
| typename M, | ||
| bool (M::*to_r)(R*) const, | ||
| M (URLPattern::*func)(Environment*, |
There was a problem hiding this comment.
I don't think this is a good solution. Yes you're reducing code duplication but at what cost? reducing readability.
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
Don't really see the issue here
4 participants
Refactor these
URLPatternmethods and extract common logic into a helper function, reducing code duplication.