Spriter / SwiftyHue

Philips Hue SDK
MIT License
83 stars 24 forks source link

BridgeFinder: scanner classes are reversed then popLast()'d #8

Closed theladyjaye closed 8 years ago

theladyjaye commented 8 years ago

Trying to understand why this is vs just setting the array to the desired order.

AKA why not:

NUPNPScanner first

self.init(validator: BridgeValidator(), scannerClasses: [NUPNPScanner.self, SSDPScanner.self])
... later ....

allScannerClasses.pop()

OR SSDPScanner first

self.init(validator: BridgeValidator(), scannerClasses: [SSDPScanner.self, NUPNPScanner.self])
... later ....

allScannerClasses.pop()

Why the need for the reversed

self.allScannerClasses = scannerClasses.reversed()
NilsLattek commented 8 years ago

The recommended search order from Philips is SSDPScanner, NUPNPScanner, IPScanner (not implemented currently) and I wanted to make sure that this order is visible in code. So we should use the order scannerClasses: [SSDPScanner.self, NUPNPScanner.self]. We could remove the reverse() call and replace the popLast() call with something like removeAtIndex(0). This would have the same effect and might be more readable. I think at the time I was mainly using pop to remove elements from an array 😄 and did not think of removeAtIndex.

Does this make sense?

theladyjaye commented 8 years ago

actually I think all it would need is the contents of this comment and it'd be gold. Once I read your rationale it made sense.

theladyjaye commented 8 years ago

I like the removeAtIndex as well to be honest.. PR incoming

theladyjaye commented 8 years ago

ehhhh.. actually there is not way to avoid that IndexError there, without counting, that I see. popLast however is -> T?... so I think popLast it is =)

NilsLattek commented 8 years ago

Ah right exception handling with popLast() returning an optional is more elegant. Thanks for all your contributions!

theladyjaye commented 8 years ago

https://github.com/Spriter/SwiftyHue/pull/9/files

Splain'd it =)

NilsLattek commented 8 years ago

Awesome 👍 👍