Remove recursion from MatchFinder

[matthewmcgarvey]

Summary

This replaces a somewhat complicated recursive method with an almost equally complicated loop. The advantage is that loops are faster than recursion. More refactoring can be done in the future to clean up the code.

The benchmark code changes take an average of running the code many, many times so that the measurements can be more accurate.

Benchmark

• Before: 12.3 ms
• After: 10.4 ms
• Difference: 1.9 ms
• Improvement: 15%

Side Note

I tried to remove the NoMatch class and replace it with simply returning nil. It actually was slower than keeping the NoMatch class and I have no idea why!

[matthewmcgarvey]

@jwoertink Would you be able to review this?

[jwoertink]

I always forget about this repo 😂

[jwoertink]

This is pretty cool. I did some testing on this PR and actually pulled it down to run the benchmark and compare VS master. I used this exact same benchmark code between master branch and this PR, on average I got 2.9ms from master, and 2.1ms from this branch. I also took this branch and put it in to the main Lucky repo to ensure all the specs on that still pass. Then lastly, I put that version of Lucky in to one of my apps, and booted it. Everything booted, all pages loaded....

So all in all, this looks great to me! I did leave a few comments, like the benchmark can get a small cleanup, and this branch does work fine on Crystal 0.35...

I do have a few questions just to be super extra sure...

• When I ran specs on master, I got 15 examples passing. Running specs on this branch, I got 17 examples passing... I don't see any specs changed here.. How many examples do you get locally?
• In the master branch, we do a check for static_fragment, and dynamic_fragment for the recursive bit... What was static_fragment and dynamic_fragment that we no longer need them?
• I also left a comment about possibly using loop instead of until. Let me know your thoughts on that.

Great work on this!

[matthewmcgarvey]

@jwoertink I've implemented the requested changes.

Regarding the differing number of specs, I'm not seeing the same thing. Running spec on master results in 17 examples for me.

Regarding the static_fragment/dynamic_fragment comment, I did not remove that code. The recursion was using those methods to pass along the matched fragment to the next iteration. In my code, I am assigning matched_fragment to a variable and that fragment is either static or dynamic and continuing on through the loop.

[jwoertink]

Sweet! This is all looking solid to me. Thanks so much for taking care of this! 🚀 Can't wait to get this in my apps.

[matthewmcgarvey]

@jwoertink any chance you meant to merge this?

[jwoertink]

Not quite. I'm going to wait for 1 more approval on this. @paulcsmith will take a look after he has some free time, but it might be a bit. We just want to make sure we have all the edge cases covered. We might need to add a few specs in just to be sure, but we're hoping that there's no weird side affects from this 😄

[paulcsmith]

I'd love to take a look at this before merging to make sure it handles some of the trickier edge cases that most routers don't get. Especially since routing is so core to Lucky. I've been very busy, but will try to take a look soon!

[paulcsmith]

Looks great! Thank you :) And I'm glad we can stick with NoMatch. In general I like to make wrapper structs/classes that clarify what the result means since nil isn't as clear and can sometimes sneak bugs in where there was a nil that wasn't noticed. Using NoMatch makes it clear if an accidental case wasn't handled since it needs to return a Match or NoMatch and not nil.

Anyway, this looks good! Just thought it may be helpful to share why NoMatch and Match were introduced