Refactor match finding (remove MatchFinder) #32
Conversation
| def initialize(@dynamic = false) | ||
| end |
There was a problem hiding this comment.
This makes me nervous but I think you plan to extract a DynamicFragment right? If so then I think this is good for now and can be cleaned up in a later PR
| # params are a key value pair of a path variable name matched to its value | ||
| # so a path like /users/:id will have a path variable name of id and | ||
| # a matching url of /users/456 will have a value of 456 |
src/lucky_router/fragment.cr
Outdated
| params = {} of String => String | ||
| result = parts.reduce(self) do |fragment, part| | ||
| match = fragment.static_parts[part]? || fragment.dynamic_part.try(&.fragment) | ||
| return NoMatch.new if match.nil? |
There was a problem hiding this comment.
Could you refactor this to use implicit returns instead? I find them a bit tricky to work with and refactor since the behavior can change when extacted. For example if this this block of code inside the do/end were moved to a private method the early return would no longer work because it'd return from the new method and not this one.
src/lucky_router/fragment.cr
Outdated
| end | ||
|
|
||
| payload = result.method_to_payload[method]? | ||
| return payload.nil? ? NoMatch.new : Match(T).new(payload, params) |
There was a problem hiding this comment.
| return payload.nil? ? NoMatch.new : Match(T).new(payload, params) | |
| payload.nil? ? NoMatch.new : Match(T).new(payload, params) |
src/lucky_router/fragment.cr
Outdated
| def dynamic? | ||
| @dynamic | ||
| end |
There was a problem hiding this comment.
You can do getter? :dynamic to define a getter :D It's a handy little macro!
|
@paulcsmith Your changes have been implemented |
Summary
This change removes the
MatchFinderclass completely and moves the logic into theFragmentclass. I believe this brings clarity to the codebase as the logic using the finder required passing in the fragment and all of the passed in information anyways and it was calling methods on the fragment. It also brought a fairly hefty speed boost.By putting all this code together where it was previously separated, I'm hoping to find patterns in the codebase that can be extracted to provide further clarity. The ones that I am currently considering are:
Fragment#findupdates a hash of params as it reduces over the path parts. I'm thinking of an alternative API where this find delegates to a find on its current fragment that handles a single path part and returns back a tuple of the matching fragment and the path part and after the reduce check if any of the matching fragments were dynamic to be up the paramsBenchmark (with
--release)