1
\$\begingroup\$

I have a RadioStation class file and a view controller where I create two station objects based on this class, one for FM and one for AM.

I created both of these station objects from within the same required init method. I'm not sure this was this the best way to do this, for example can you imagine if you wanted 8 such objects, this would just get very clunky. Is the issue rather that I am creating them in the view controller, and should I have in fact created separate subclass files for each and init them in those? Or should I have created two separate init methods, one for each new object?

RadioStation Class File: RadioStation.swift

import UIKit

class RadioStation: NSObject {

    var name: String
    var frequency: Double

    override init() {  //init class method to set default values.
        name = "Default"
        frequency = 100
    }

    static var minAMFFrequency: Double = 520.0
    static var maxAMFFrequency: Double = 1610.0
    static var minFMFFrequency: Double = 88.3
    static var maxFMFFrequency: Double = 107.9

    func isBandFM() -> Int {
        if frequency >= RadioStation.minFMFFrequency && frequency <= RadioStation.maxAMFFrequency {
            return 1 //FM
        } else {
            return 0 //AM
        }
    }

}

View Controller Class

import UIKit

class ViewController: UIViewController {

    @IBOutlet weak var stationName: UILabel!
    @IBOutlet weak var stationFrequency: UILabel!
    @IBOutlet weak var stationBand: UILabel!

    var myStation: RadioStation

    var myStationAM: RadioStation //second object variable

    required init?(coder aDecoder: NSCoder) {
        myStation = RadioStation()
        myStationAM = RadioStation()
        myStation.frequency = 104.7
        myStationAM.frequency = 600.2
        myStation.name = "FM1"
        myStationAM.name = "AM1"
        super.init(coder: aDecoder)
    }

    override func viewDidLoad() {
        super.viewDidLoad()
        // Do any additional setup after loading the view.
    }

    @IBAction func buttonClick(_ sender: Any) {
        stationName.text = myStation.name //set top left label text to name property of myStation object instance
        stationFrequency.text = "\(myStation.frequency)"
        if myStation.isBandFM() == 1 {
            stationBand.text = "FM'"
        } else {
            stationBand.text = "AM"
        }
    }

    @IBAction func buttonClickAM(_ sender: Any) {
        stationName.text = myStationAM.name
        stationFrequency.text = "\(myStationAM.frequency)"
        if myStationAM.isBandFM() == 1 {
            stationBand.text = "FM"
        } else {
            stationBand.text = "AM"
        }
    }

}
\$\endgroup\$
3
  • \$\begingroup\$ Can someone tell me why this was downvoted? The code works, and I would like to learn about init methods for more than one object at a time. What did I do wrong? \$\endgroup\$ – timman Mar 27 '20 at 21:16
  • 2
    \$\begingroup\$ One problem is that (on this site) the title should state what your code does in your title, not your main concerns about it, compare How to Ask. I would suggest to revise the title accordingly. Make it clear in the question the code works correctly. \$\endgroup\$ – Martin R Mar 28 '20 at 16:29
  • 2
    \$\begingroup\$ (Cont.) There is one “Needs details or clarity” vote, so make sure that the code is self-contained, or at least contains sufficient context to understand it. Then you can (in the question, not in the title) describe your concerns about the code and what you would like to be reviewed. Make sure that it is not a “best practices” question as described in help center. \$\endgroup\$ – Martin R Mar 28 '20 at 16:29
5
\$\begingroup\$

A couple of thoughts:

  1. You are conceiving the band as being something derived from the frequency. While this may be technically correct, from a user perspective, you generally choose a band and then choose a frequency within that band. So, I might have a model that reflects this:

    typealias RadioFrequency = Decimal
    
    enum RadioBand {
        case am
        case fm
    }
    
    extension RadioBand {
        var allowedRange: ClosedRange<RadioFrequency> {
            switch self {
            case .am: return 520.0 ... 1610.0
            case .fm: return 88.3 ... 107.9
            }
        }
    }
    
    struct RadioStation {
        let name: String
        let band: RadioBand
        let frequency: RadioFrequency
    
        init?(name: String, band: RadioBand, frequency: RadioFrequency) {
            guard band.allowedRange ~= frequency else { return nil }
    
            self.name = name
            self.band = band
            self.frequency = frequency
        }
    }
    

    and

    guard let station = RadioStation(name: "KPCC", band: .fm, frequency: 89.3) else {
        // handle error here
    
        return
    }
    
  2. If you really wanted to compute the band from the frequency, you could do:

    enum RadioBand: CaseIterable {
        case am
        case fm
    }
    
    extension RadioBand {
        var allowedRange: ClosedRange<RadioFrequency> {
            switch self {
            case .am: return 520.0 ... 1610.0
            case .fm: return 88.3 ... 107.9
            }
        }
    
        static func band(for frequency: RadioFrequency) -> RadioBand? {
            RadioBand.allCases.first { $0.allowedRange ~= frequency }
        }
    }
    
    struct RadioStation {
        let name: String
        let frequency: RadioFrequency
        var band: RadioBand? { RadioBand.band(for: frequency) }
    
        init?(name: String, frequency: RadioFrequency) {
            guard RadioBand.band(for: frequency) != nil else { return nil }
    
            self.name = name
            self.frequency = frequency
        }
    }
    
  3. Minor details in the above:

    • I wouldn’t generally subclass NSObject unless there was a compelling reason to do so.

    • Whether you make it a class or struct is up to you, but we often we would lean towards value types and immutable properties unless otherwise needed.

    • I’m abstracting the frequency ranges for bands out of the RadioStation type. This is behavior of radio bands, not of individual radio stations, themselves.

    • The radio bands might be better represented as an enumeration rather than an integer that has only two values, either 0 or 1.

      In general, if you find yourself inserting comments to explain cryptic values (e.g. where you note that 1 represents “FM”), that’s a good sign that you should adopt an enumeration, which makes its intent clear.

    • Note, I made RadioFrequency to be Decimal, rather than Double. I did that because you really don’t need floating point math, but rather want to accurately represent decimal numbers.

      FWIW, Double can introduce some strange behavior. Let’s say that you started at the lower bound of your AM frequencies and bumped it up by 0.1 ten times:

      var value: Double = 520.0
      for _ in 0 ..< 10 {
          value += 0.1
      }
      
      print(value, value == 521.0) // 521.0000000000002 false
      

      The result is not 521. But, if you use Decimal, you get the expected behavior:

      var value: Decimal = 520.0
      for _ in 0 ..< 10 {
          value += 0.1
      }
      
      print(value, value == 521.0) // 521 true
      

      See What Every Computer Scientist Should Know About Floating-Point Arithmetic.

    • I’d make the initializer failable in case the band didn’t match the frequency (or the band couldn’t be inferred from the supplied frequency).

    • I would not be inclined to have the RadioStation to have default values. That’s not a concept that makes sense. Sure, if you wanted your radio to default to a particular station, then fine, it would do that. But that’s not RadioStation behavior, but rather behavior of the radio.

  4. You said:

    ... imagine if you wanted 8 such objects, this would just get very clunky.

    If you had many, you might use collections. For example:

    let stations: [RadioStation]
    

    Or you might wrap it in a custom type that indicated the functional intent. For example, let’s imagine that your radio had programmable presets, then the model might be:

    struct RadioPresets {
        var stations: [RadioStation]
    }
    

    It just depends upon what these various stations are for. But don’t shy away from simpler fundamental object types just because you might need multiple ones. If the concept of a single radio station makes sense (which it does, IMHO), then stick with a simple radio station object, and compose it into more complex objects/models as needed later.

    But, as a generally programming principle, we generally prefer simple, cohesive types and compose complex objects from these simple objects (e.g. a collection of 8 radio stations).

\$\endgroup\$
1
  • \$\begingroup\$ Thank you again Rob, it's a lot to parse and will do so thoroughly, plus learn these new concepts. \$\endgroup\$ – timman Mar 28 '20 at 20:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.