RedMadRobot / input-mask-ios

User input masking library repo.
MIT License
578 stars 96 forks source link

Less ! #73

Closed RomanPodymov closed 5 years ago

RomanPodymov commented 5 years ago

Hello. In this pull request I optimized code to use less ! and more Swift standard library functions.

taflanidi commented 5 years ago

Hey @RomanPodymov! Thanks for your effort throwing this pull request together.

My question is: why do you think we need less force unwrapping in this code?

RomanPodymov commented 5 years ago

My question is: why do you think we need less force unwrapping in this code?

As for me a code snippet like optionalVariable != nil ? optionalVariable! : anotherValue doesn't look like a Swift 4.2 code. I suggested to use map in such case, but you can propose something different.

taflanidi commented 5 years ago

TL;DR: I won't accept a pull request with reasoning like this.

doesn't look like a Swift 4.2 code

No, it looks exactly like a Swift code.

Instead of a reasoned and straightforward nullable unpacking your changes introduce ambiguity. Now at a glance it looks like a Sequence is mapped and added to the original string. It is exactly one of the reasons Kotlin introduced its own .let { … } call for the cases like these, to counter ambiguity.

Also also, a nitpicking low-level developer in me says that in the original solution there were four ternary operators plus four force unwrap operators. Latter were guaranteed to be effectively optimised by the compiler into direct memory access just because of the if nil != optional statements on the same line.

Instead, you've put four implicit stack return statements inside four additional closures, plus you've introduced four additional implicit variable declarations inside those closures and yet the same four nil checks with ?? operators. And believe it or not, Swift inliner likely won't optimise your closures.

Thus, my four "ifs" against your four "ifs" + four closures + four additional variables + four stack pops. Doesn't look good, does it?

Just because it is a language feature doesn't mean you've got to mindlessly abuse it. Apple puts a lot of effort into this nice PR targeted at junior and web developers to paint Swift modern and progressive. But this doesn't mean you've got to put your own mind & thinking on a shelf, and refuse what's your own experience says.

You may argue, but remember: in a polite manner, Apple had to explicitly say (at 13:00) developers tend to abuse protocol-oriented programming, yet an another modern & progressive feature.

RomanPodymov commented 5 years ago

Hello @taflanidi Sorry, I did not know about the code style used in this library. I will close the pull request then.