Since this is a good chunk of my day job, I've got a bit of feedback for you 🙂
Just keep in mind that Swift is a very flexible language! Your translation works, so you can use as much or as little of that as you want, depending on your or your team's preferences.
Embrace Immutability
C was designed to be sugar atop Assembly, which emulates a Turing machine, so it's very good at mutation. Swift was designed to be algorithmic, declarative, and functional, so it's very good at copying. Because of this, Swift code will work and optimize better if you embrace immutability.
For example, you already did a little bit of this in increaseIndex(_:)
by making it take an input and return an output, rather than mutating some global variable:
func increaseIndex(_ x: Int) -> Int {
var x = x + 1
if x >= queueSize {
x = 0
}
return x
}
In C, this is good because it works with C's talents of mutating memory. However, this comes at the cost of clarity: If x
is a var
, where is it mutated? What is its value when it's returned and when might that change? If I want to write more code in this function, how, where, and why should I mutate x
? I have to think like a machine, stepping through the code in my head and keeping track of state, just to understand what will be returned.
Compare that to this version:
func increaseIndex(_ x: Int) -> Int {
let x = x + 1
if x >= queueSize {
return 0
}
return x
}
Here, we know that x
has the same value throughout the function, and will never change, since it's declared as a let
constant. We can see at the declaration site that it's always the input plus one. We can also see that there's a branch with a special case, and the only code in there is return 0
. So we know that this function can only ever return its input plus one, or zero.
We can do better, though, if we...
Embrace language constructs
Taking the same example above, there are a few more approaches that Swift allows. Let's look at this one:
func increaseIndex(_ x: Int) -> Int {
let x = x + 1
guard x < queueSize else {
return 0
}
return x
}
The guard
statement! It works kind of like other languages' unless
, except it must always leave scope (in this case, return
. In loops, continue
and break
can also work, etc.). This gives us the peace-of-mind that, since we see a guard
, absolutely nothing after it will be executed if its condition is false. We also know that maintainers can't mess this up solely by changing the body of the guard
, since the compiler enforces that the body must return from this function no matter what.
This is another approach that's available to us:
func increaseIndex(_ x: Int) -> Int {
let x = x + 1
return x < queueSize
? x
: 0
}
Ternary operators aren't everyone's cup-of-tea, so I understand if you don't like this approach. Personally, I find it much better because you can see this function has two lines: it declares a constant and then returns. No possibility of it doing anything else, and it's clear that it either returns that constant or zero, depending on the condition we see.
Here's another place where Swift can help things be clearer:
private func getDistance(rssi: Double, txPower: Int) -> Double {
if rssi == 0 {
return -1.0 // if we cannot determine accuracy, return -1.
}
return pow(10, (Double(txPower) - rssi) / (10 * 2))
}
Your comment there is necessary because you're using a magic number. This is unnecessary in Swift, since enum
s are very cheap and lightweight, and have associated values. The Swift compiler's optimizer will take care of erasing the enum's cases where possible, too, so you don't have to worry about (un)boxing. So, this function can be made clearer with an Optional
return!
private func getDistance(rssi: Double, txPower: Int) -> Double? {
if rssi == 0 {
return nil
}
return pow(10, (Double(txPower) - rssi) / (10 * 2))
}
And, if you're OK using ternary operators, it can further be simplified to this:
private func getDistance(rssi: Double, txPower: Int) -> Double? {
return rssi == 0
? nil
: pow(10, (Double(txPower) - rssi) / 20)
}
Swift separates literals from types
In C and its descendents, each primitive has its own literal, and strings exist too. In Swift, literals can represent anything, and default to primitives if no type is specified. In fact, you take advantage of this in a few places already!
This means that you don't have to write 1.0
or similar where it's clear a Double
is expected; you can simply write 1
. This makes the language more human. For me, it makes it easier to read too.
Lots to learn in calculateAverage()
Last up, let's look at calculateAverage()
. At first glance, I'm not sure what's intended here, nor how it works. There's a lot of internal state and low-level operations, and a recreation of a C-style for-loop. These are big code smells for me, telling me there's stuff to be done here:
var rssiArray = [0]
var sortRssi = [0]
private func calculateAverage() -> Double {
var average: Double = 0
var i = 0
let drop = 3
memcpy(sortRssi, rssiArray, queueSize * MemoryLayout<Int>.size)
qsort(sortRssi, queueSize, MemoryLayout<Int>.size, cmpfunc)
for i in 0..<queueSize - drop {
if sortRssi[i + drop] == 0 {
break
}
average += Double(sortRssi[i])
}
return average / Double(i)
}
First, as rid said, the sorting can be simplified greatly by taking advantage of Swift's built-in memory management and standard library functions:
memcpy(sortRssi, rssiArray, queueSize * MemoryLayout<Int>.size)
qsort(sortRssi, queueSize, MemoryLayout<Int>.size, cmpfunc)
// becomes:
sortRssi = rssiArray.sorted(by: >)
This also means that compareFunction
is unnecessary!
Next, I see a constant drop = 3
. It appears to me you're using this to drop the first 3 values from the array, which can be replaced with the standard library function dropFirst(_:)
. This returns a lazily-evaluated sequence, so we'll also have to create an array from that:
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
for i in 0..<sortRssi.count {
if sortRssi[i] == 0 {
break
}
average += Double(sortRssi[i])
}
I also notice that you're keeping track of an i
variable here, and returning it at the end. This is also unnecessary since now its value will be the same as sortRssi.count
, and Swift only allows for-each loops anyway:
private func calculateAverage() -> Double {
var average: Double = 0
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
for item in sortRssi {
if item == 0 {
break
}
average += Double(item)
}
return average / Double(sortRssi.count)
}
Looking better already! We can further simplify this by taking advantage of the Swift higher-order function reduce
. As the name implies, this reduces a collection of values down to a single value, which is exactly what you want in an averaging function: to reduce an array of numbers to a single number representing the average:
private func calculateAverage() -> Double {
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
let total: Double = sortRssi.reduce(into: 0) { average, item in
if item == 0 {
break // !! Doesn't compile!
}
average += Double(item)
}
return total / Double(sortRssi.count)
}
Oh my! It seems that reduce
can't work here because of this special condition of your program! Not to worry, we can take care of that with another standard library function, prefix(while:)
. This lets us take only the first however-many items in the array, so long as they all match a certain condition, just like you're doing here:
let total: Double = sortRssi
.prefix(while: { $0 != 0 })
.reduce(into: 0) { average, item in
average += Double(item)
}
As a C dev, I'm sure this is setting off your big-O alarm, since it loops twice! Not to worry, there's this magical little gem: .lazy
. It allows any collection to be lazily-evaluated, so the loop only runs once:
let total: Double = sortRssi
.lazy
.prefix(while: { $0 != 0 })
.reduce(into: 0) { average, item in
average += Double(item)
}
And now since total
is created on one line and used once on the next, we don't have to dedicate a line to declaring it, so we can remove it altogether and get this:
private func calculateAverage() -> Double {
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
return sortRssi
.lazy
.prefix(while: { $0 != 0 })
.reduce(into: Double(0)) { average, item in
average += Double(item)
}
/ Double(sortRssi.count)
}
And since we're already in lazy-land, we can restructure this a little more if we want with another standard library function, map
:
private func calculateAverage() -> Double {
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
return sortRssi
.lazy
.prefix(while: { $0 != 0 })
.map(Double.init)
.reduce(into: 0) { average, item in
average += item
}
/ Double(sortRssi.count)
}
And now since the body of the reduce
only has one function call in it (+=
), we can remove the body and replace it with a reference to that function:
private func calculateAverage() -> Double {
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
return sortRssi
.lazy
.prefix(while: { $0 != 0 })
.map(Double.init)
.reduce(into: 0, +=)
/ Double(sortRssi.count)
}
This looks much better to me. It is very Swifty! There's no internal state, it's all standard Swift functions,
If you want, though, it can be made to fit the philosophies of Swift even better by removing it from state and placing it in a type-extension, to make it more reusable in other places:
private func calculateAverage() -> Double {
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
return sortRssi.averaged(while: { $0 != 0 })
}
extension Collection where Element: BinaryInteger {
func averaged(while allowedPrefixPredicate: (Element) -> Bool) -> Double {
lazy
.prefix(while: allowedPrefixPredicate)
.map(Double.init)
.reduce(into: 0, +=)
/ Double(count)
}
}
There it is. Now, the work we put into creating this averaging function can be reused for any collection containing integers. We use it here on an array of Int
s, but later if you want similar behavior, you can use it on a set of UInt8
s, etc!
Putting it all together
Embracing these patterns makes this more reliable. Let's see how it looks after we put all this together in your program:
let queueSize = 16
var rssiArray = [0]
var sortRssi = [0]
var rssiIndex = 0
func increaseIndex(_ x: Int) -> Int {
let x = x + 1
return x < queueSize
? x
: 0
}
private func getDistance(rssi: Double, txPower: Int) -> Double? {
return rssi == 0
? nil
: pow(10, (Double(txPower) - rssi) / 20)
}
private func calculateAccuracy(rssi: Double, txPower: Int) -> Double? {
guard rssi != 0 else {
return nil
}
let ratio = rssi * (1 / Double(txPower))
return ratio < 1
? pow(ratio, 10)
: (0.89976) * pow(ratio, 7.7095) + 0.111
}
private func calculateAverage() -> Double {
sortRssi = Array(rssiArray.sorted(by: >).dropFirst(3))
return sortRssi.averaged(while: { $0 != 0 })
}
extension Collection where Element: BinaryInteger {
func averaged(while allowedPrefixPredicate: (Element) -> Bool) -> Double {
lazy
.prefix(while: allowedPrefixPredicate)
.map(Double.init)
.reduce(into: 0, +=)
/ Double(count)
}
}
This looks good to me. It maintains the original functionality, and embraces Swift as much as it can. I hope it helps!
Compare before and after
After adjusting for whitespace and comment lines, and tweaking names a bit to be more comparable, here's a diff comparing the original C code to my Swift translation:
https://www.diffchecker.com/muVaLwtG
Double
can be represented with1
, omitting the.0
when it's clear that aDouble
goes there (comparing, passing to a function, etc.). \$\endgroup\$ – Ben Leggiero Dec 9 '20 at 22:48