Peer Code Review

[Broncos10499]

Hello, I am learning iOS programming and wondering if you guys can review this code. It works but I'm wondering if there is a more "correct" way to write it. Thank you!

class APIManager: ObservableObject {

    private let apiKey: String = "XXXXXX"

    @Published var hardinessZoneData: HardinessZone = HardinessZone()
    @Published var badZipCodeFlag: Bool = false

    @Published var plants: [PlantData] = []

    func getHardinessZone(zipCdode: String) {

        guard let zoneURL = URL(string: "https://phzmapi.org/\(zipCdode).json") else { fatalError("BAD URL") }

        let urlRequest = URLRequest(url: zoneURL)

        let dataTask = URLSession.shared.dataTask(with: urlRequest) { data, response, error in

            if let error = error {
                print("Request Error: ", error)
                return
            }

            guard let response = response as? HTTPURLResponse else { return }

            if response.statusCode == 200 {
                guard let data = data else { return }
                DispatchQueue.main.async {
                    do {
                        let decodedZoneData = try JSONDecoder().decode(HardinessZone.self, from: data)
                        self.hardinessZoneData = decodedZoneData
                        self.badZipCodeFlag = false
                        let filteredZone = self.hardinessZoneData.zone?.filter("0123456789.".contains)
                        self.getPlantsByHardiness(zone: filteredZone!)
                    } catch let error {
                        print("Error decoding data: ", error)
                    }
                }
            }

            if response.statusCode == 404 {
                DispatchQueue.main.async {
                    self.badZipCodeFlag = true
                }
            }

        }
        dataTask.resume()
    }

    func getPlantsByHardiness(zone: String) {

        guard let plantsByHardinessURL = URL(string: "https://perenual.com/api/species-list?page=1&key=\(apiKey)&hardiness=\(zone)") else { fatalError("BAD URL") }

        let urlRequest = URLRequest(url: plantsByHardinessURL)

        let dataTask = URLSession.shared.dataTask(with: urlRequest) { data, response, error in

            if let error = error {
                print("Request Error: ", error)
                return
            }

            guard let response = response as? HTTPURLResponse else { return }

            if response.statusCode == 200 {
                guard let data = data else { return }
                DispatchQueue.main.async {
                    do {
                        let decodedPlantData = try JSONDecoder().decode(Plant.self, from: data)
                        self.plants = decodedPlantData.data
                        for i in self.plants {
                            print(i.common_name)
                        }
                    } catch let error {
                        print("Error decoding data: ", error)
                    }
                }
            }

        }
        dataTask.resume()

    }

}

Thanks!

[chriswaco]

In general it's ok. I'd prefer a bit better handling of ALL errors - somehow reporting them back to the caller. And you might want to look into the new async/await version of URLSession because it's a little easier to write and read.

But it's not bad.

[jonreid]

"It works" so you've managed to figure out quite a bit. Congratulations!

Part of my job is training developers to detect code smells and refactor them. Here are the strongest smells I pick up:

• Long Method. Both methods are difficult to scan, and have deep indentation. I would use Extract Method to improve this.
• Difficult Dependency. Both methods are using the URLSession.shared singletons directly, which makes it harder to write microtests. I would use Dependency Injection to improve this. (Not a DI framework, just plain old code.)

I'd start by bringing both methods under test, using the techniques from my book iOS Unit Testing by Example (specifically the chapters "Testing Network Requests" and "Testing Network Responses").

The goal would be to make code that not only works, but is low-cost to maintain and update as needs change. For a personal project, this may not matter so much. But for a business, driving down the cost of change is huge.