class ViewController: UIViewController {
I noticed that your class is called ViewController
. There are two things to comment on based on this name.
ViewController
is basically the prefix that all of your view controllers should have. There should be more meat to the name. What sort of view are you controlling? Is this a BachPlayerViewController
(as Bach.mp3 is hardcoded)? Or is this the first iteration of MusicPlayerViewController
? Either way, ViewController
is not nearly self-documenting enough to be acceptable.
The name of the class is also not Everything
, and as such, it shouldn't be doing everything. You've dumped a lot of logic into this class that would have to be duplicated in other view controllers that also wanted to do things like update their UI on a tick with a new set of information from the audio player. A common mistake for iOS developers is to put all of their logic for everything inside the one set of classes that you can't make an app without: view controllers. We deserve a layer of abstraction.
I'm not going to critique much of your actually view controller code. There's not much in it, but a lot of what is there (and doesn't directly relate to interacting with the UI) doesn't belong in it. Instead, I'm going to present you with a class that would let any view controller more easily interact with AVAudioPlayer
and do things like update their UI on some sort of tick.
I'm going to start with a cleaner way of passing around the time stamp information:
class AudioTimeStamp: NSObject {
enum Format {
case WithoutHours, WithHours
}
private let timeInterval: Int
var seconds: Int { get { return timeInterval % 60 } }
var minutes: Int { get { return (timeInterval / 60) % 60 } }
var hours: Int { get { return timeInterval / 3600 } }
init(seconds: NSTimeInterval) {
timeInterval = Int(seconds)
}
func formattedString(format: Format = .WithHours) -> String {
switch format {
case .WithHours: return String(format: "%02i:%02i:%02i", hours, minutes, seconds)
case .WithoutHours: return String(format: "%02i:%02i", (hours * 60) + minutes, seconds)
}
}
override var description: String {
return formattedString()
}
override var debugDescription: String {
return formattedString()
}
}
Now all of the logic for dealing with the time stamp is abstracted into this struct. We can of course add more cases to the Format
enum and handle the different options in different ways.
Next, we need a way to update those that care about getting updates. For this, we'll go with the protocol-delegate pattern. For now, let's stick with something simple, but we'll be re-addressing this in the future:
protocol AudioPlayerDelegate: class {
func audioPlayer(audioPlayer: AudioPlayer, didUpdate timestamp: AudioTimeStamp)
}
Note that AudioPlayer
is the name of a class we're about to create.
Let's look at creating that AudioPlayer
class now. The interface of what we want out of our AudioPlayer
looks something like this:
class AudioPlayer {
weak var delegate: AudioPlayerDelegate?
init(contentsOfURL: NSURL) throws {}
func play() {}
func pause() {}
func stop() {}
func preload() {}
func unload() {}
}
- We want a way to assign our the
delegate
property so we can control who is updated of particular events happening on the player.
- We want an initializer that takes an
NSURL
so we can set up an audio player with a file like your code base already does. It is worth noting that AVAudioPlayer
has three other initializers, and we'll probably want to add an initializer for all of these, because this class is effectively just a wrapper for that.
- We want a way to begin playback of the audio. This method, however, will do more than just start playback. It will start up a timer to call the aforementioned
audioPlayer(_:didUpdate:)
method on our delegate.
- We want a way to pause the audio.
- We want a way to stop the audio. Importantly, here, unless
AVAudioPlayer
's stop
method, we will reset the current time to zero and we will not release the preloaded resources for the track.
- We want a way to preload and unload the resources required for the actual audio playback.
- As a note, we probably want to add some computed variables for values volume, whether or not the track is actively playing, current time, etc.
The answer is already growing quite long, and I don't want to implement the whole thing for you, but I'll give you some of the most important aspects...
First, let's update our protocol. We'll make it a little more informative:
@objc protocol AudioPlayerDelegate: class {
optional func audioPlayerShouldBeginPlaying(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp) -> Bool
optional func audioPlayerWillBeginPlaying(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp)
optional func audioPlayerDidBeginPlaying(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp)
optional func audioPlayerShouldPause(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp) -> Bool
optional func audioPlayerWillPause(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp)
optional func audioPlayerDidPause(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp)
optional func audioPlayerShouldStop(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp) -> Bool
optional func audioPlayerWillStop(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp)
optional func audioPlayerDidStop(audioPlayer: AudioPlayer, atTimestamp timestamp: AudioTimeStamp)
optional func audioPlayer(player: AudioPlayer, didUpdateAtTimestamp timestamp: AudioTimeStamp)
}
Now, a skeleton of our AudioPlayer
class looks something like this:
class AudioPlayer: NSObject, AVAudioPlayerDelegate {
private let player: AVAudioPlayer
private var updateTimer = NSTimer()
weak var delegate: AudioPlayerDelegate?
init(contentsOfURL url: NSURL) throws {
do {
try player = AVAudioPlayer(contentsOfURL: url)
super.init()
player.delegate = self
}
catch let error {
player = AVAudioPlayer()
super.init()
throw error
}
}
func play() {
let timestamp = AudioTimeStamp(seconds: player.currentTime)
guard let shouldPlay = delegate?.audioPlayerShouldBeginPlaying?(self, atTimestamp: timestamp) where !shouldPlay else {
return
}
delegate?.audioPlayerWillBeginPlaying?(self, atTimestamp: timestamp)
player.play()
delegate?.audioPlayerDidBeginPlaying?(self, atTimestamp: timestamp)
updateTimer = NSTimer.scheduledTimerWithTimeInterval(1, target: self, selector: "update", userInfo: nil, repeats: true)
updateTimer.fire()
}
private dynamic func update() {
delegate?.audioPlayer?(self, didUpdateAtTimestamp: AudioTimeStamp(seconds: player.currentTime))
}
}
The pause and stop methods are very similar to the play method. The difference is that where the play method start the timer, the pause/stop methods should invalidate the timer (and you might make a case for firing it just before invalidating it). The pause function should call pause
on the player. The stop function should call pause
and then set current time to zero.
The preload
function should simply call prepareToPlay
on the player.
The unload
function should call stop
on the player.
You should also implement at least this function audioPlayerDidFinishPlaying(player: AVAudioPlayer, successfully flag: Bool)
which is part of the AVAudioPlayerDelegate
call. I'd probably add something to the AudioPlayerDelegate
protocol to notify the delegate when this event happens, but most importantly, we should take this opportunity to invalidate the timer. We don't need to continue updating the delegate about the status of the player. We only need to do this when an event happens (like playing, pausing, stopping, track reached end, etc) or when the time stamp updates.