Skip to main content
Code Review

Return to Revisions

2 of 2
added 408 characters in body
Martin R
  • 24.2k
  • 2
  • 38
  • 96

Is there a way to tidy up the numerous functions into something like a dictionary?

Sure, there are various options. But first the functions should be made independent of maxIterations, i.e. the computation

Double(currentIteration) / Double(maxIterations)

should be done in the caller so that the functions simplify to

private func sine010(x: Double) -> Double {
 return (-cos(2 * .pi * x) + 1)/2
}

thus avoiding code duplication. Note also the use of the "implicit member expression" .pi – the type Double is automatically inferred from the context.

Now you could define a dictionary mapping a function types to functions. The functions can be embedded directly as closures instead of defining global functions:

let functionDict : [FunctionType: (Double) -> Double] = [
 .SINE_WAVE_FROM_0_TO_1_TO_0: { (1 - cos(2 * .pi * 0ドル)) / 2 },
 .SINE_WAVE_FROM_0_TO_1: { (1 - cos(.pi * 0ドル)) / 2 },
 .SINE_WAVE_FROM_1_TO_0: { (1 + cos(.pi * 0ドル)) / 2 },
]

The disadvantage is that you are now responsible to update the dictionary if new function types are added.

A perhaps better alternative is to make the function a computed property of the function type enumeration:

enum FunctionType {
 case SINE_WAVE_FROM_0_TO_1_TO_0
 case SINE_WAVE_FROM_0_TO_1
 case SINE_WAVE_FROM_1_TO_0
 
 var f: (Double) -> Double {
 switch self {
 case .SINE_WAVE_FROM_0_TO_1_TO_0: return { (1 - cos(2 * .pi * 0ドル)) / 2 }
 case .SINE_WAVE_FROM_0_TO_1: return { (1 - cos(.pi * 0ドル)) / 2 }
 case .SINE_WAVE_FROM_1_TO_0: return { (1 + cos(.pi * 0ドル)) / 2 }
 }
 }
}

Now everything is in "one place" and the compiler can check the exhaustiveness of the switch statement.

A disadvantage of all the above definitions is that they can not be extended: There is no way that a user can define its own interpolation function and pass it to the value animator.

So what I would really do is to define a struct as the function wrapper, with static properties for predefined functions. I am calling it Interpolator now (resembling the Android TimeInterpolator).

class ValueAnimator {
 
 struct Interpolator {
 // A function mapping [0, 1] to [0, 1].
 let f: (Double) -> Double
 
 // Predefined interpolation functions
 static let sineWaveFrom0To1To0 = Interpolator(f: { (1 - cos(2 * .pi * 0ドル)) / 2 } )
 static let sineWaveFrom0To1 = Interpolator(f: { (1 - cos(.pi * 0ドル)) / 2 } )
 static let sineWaveFrom1To0 = Interpolator(f: { (1 + cos(.pi * 0ドル)) / 2 } )
 }
 
 // ...
}

Note also that I switched to lower camel case for the property names, which is the standard for all Swift identifiers except for types.

Now a user can easily add more interpolation functions, e.g.

extension ValueAnimator.Interpolator {
 static let linear = ValueAnimator.Interpolator(f: { 0ドル } )
}

Is there a way of accomplishing the same functionality ... but without using a selector? ... I ask because of the nasty "MyNSDoubleObject" workaround.

First: The wrapper object is not needed even if you use selectors: Swift automatically wraps values in Objective-C compatible types if necessary. Which means that you can pass a floating point value through the selector:

let value: Double = ...
target.perform(selector, with: value)

and the receive can (conditionally) cast it back to a Double:

@objc func animate(obj: AnyObject) {
 guard let value = obj as? Double else { return }
 print(value)
}

So the MyNSDoubleObject workaround is not needed.

But it becomes much simpler if you replace the target/selector method by a simpler callback with a closure. Similarly for the local timer: With a block-based timer, the timer callback need not be Objective-C compatible, and the ValueAnimator class does not have to subclass NSObject anymore.


Some minor remarks: The

private let sampleRate: Int

property is not needed. Instead of initializing with an (inactive) timer

private var timer = Timer()

I would use an optional:

private var timer: Timer?

X_currentRepIndex and F_of_X to not follow the Swift naming conventions, the latter name is quite non-descriptive.


Putting it all together, the ValueAnimator class could look like this:

class ValueAnimator {
 
 struct Interpolator {
 // A function mapping [0, 1] to [0, 1].
 let f: (Double) -> Double
 
 // Predefined interpolation functions
 static let sineWaveFrom0To1To0 = Interpolator(f: { (1 - cos(2 * .pi * 0ドル)) / 2 } )
 static let sineWaveFrom0To1 = Interpolator(f: { (1 - cos(.pi * 0ドル)) / 2 } )
 static let sineWaveFrom1To0 = Interpolator(f: { (1 + cos(.pi * 0ドル)) / 2 } )
 }
 
 // args
 private let interpolation: Interpolator
 private let callback: (Double) -> Void
 
 // computed from args
 private let maxIterations: Int
 private let timeInterval: Double
 private var currentIteration: Int = 0
 
 // Other properties
 private var timer: Timer?
 
 init(durationInSeconds: Int, sampleRate: Int, interpolation: Interpolator,
 callback: @escaping (Double) -> Void) {
 self.maxIterations = durationInSeconds * sampleRate
 self.timeInterval = 1.0 / Double(sampleRate)
 self.interpolation = interpolation
 self.callback = callback
 }
 
 func start() {
 timer = Timer.scheduledTimer(withTimeInterval: timeInterval, repeats: true) { (timer) in
 let val = Double(self.currentIteration)/Double(self.maxIterations)
 self.callback(self.interpolation.f(val))
 self.currentIteration += 1
 if self.currentIteration > self.maxIterations {
 self.stop()
 }
 }
 }
 
 func stop() {
 timer?.invalidate()
 timer = nil
 }
}

and a sample usage could look like this:

class ViewController: ViewController {
 
 var valueAnimator: ValueAnimator?
 
 override func viewDidLoad() {
 valueAnimator = ValueAnimator(durationInSeconds: 2, sampleRate: 2,
 interpolation: .sineWaveFrom0To1To0)
 { [weak self] value in
 guard let self = self else { return }
 
 // Do something with value ...
 }
 valueAnimator?.start()
 }
 
}

Note the use of weak self in the closure to avoid a reference cycle.


Further thoughts:

  • Since ValueAnimator is a "pure Swift" class now it can be made generic to support other value types, such as integers.

  • Finally: If the goal is to animate visual elements: don't reinvent the wheel, use Core Animation.

Martin R
  • 24.2k
  • 2
  • 38
  • 96
default

AltStyle によって変換されたページ (->オリジナル) /