klingtnet / rosc

An OSC library for Rust.
Apache License 2.0
173 stars 25 forks source link

Refactored `Matcher.match_address` to use `PeekableIterator` instead of manual indexing. #42

Closed maxnoel closed 1 year ago

maxnoel commented 1 year ago

Fixes https://github.com/klingtnet/rosc/issues/28

That being said, the removal of mut remainder in favor of a proper fold operation means the loss of one interesting property: the shortcut return when the match_* function returns an error. This changeset means self.pattern_parts is always fully iterated over (although the performance loss is probably negligible, unless the matcher's own pattern is huge).

In addition, if you look through the history of this pull request, my personal opinion is that the second-last revision (the one right before I get rid of the for loop and mut remainder) reads better.

klingtnet commented 1 year ago

my personal opinion is that the second-last revision (...) reads better.

Is this the commit you're referring to c45b3be991f420864bdcabefcfb3292b74d4b840 (or ea788c309e4df7b2e69dd768b24f1fe87cde83de if we want to see the diff of the mut iter removal)? Note that you can just paste commit hashes into the text and GitHub will automatically link to them.

klingtnet commented 1 year ago

I think I agree with you, that the version which uses the peekable iterator is most readable. So, I'd be fine with keeping the peekable iterator version and discard the following two commits. This will make the code more readable even though we still have the mut variable.

What do you think @maxnoel ?

maxnoel commented 1 year ago

That's fair. I've updated the PR to re-introduce the peekable iterator (which I initially was reluctant to -- you want to remove a mut variable and my solution is to not only not remove it but add a second one... :D )

klingtnet commented 1 year ago

That's fair. I've updated the PR to re-introduce the peekable iterator (which I initially was reluctant to -- you want to remove a mut variable and my solution is to not only not remove it but add a second one... :D )

Hehe, but it looks better now. Sometimes the unorthodox solutions are to be preferred :)

klingtnet commented 1 year ago

Thank you for the contribution, I'm going to merge this now.