Sqlite.swift -- Selecting all rows and returning them in an array of tuples

Question

Link to SQLite wrapper

func queryAll() -> [(String, String, String)]  {
    do {
        let sql = try DB?.prepare("SELECT city, zip , temp FROM weather")
        var arr : [(city : String, zip : String, temp : String)] = []
        for row in sql! {
            arr += [(city: row[0] as! String, zip: row[1] as! String, temp: row[2] as! String)]               
        }
        return arr;            
    }
    catch {
        print("\(error)")
    }
    return [("","","")]
}

Grabbing all the rows, and returning them as a tuple for later use. I've read the docs but I cannot think of a more efficient way to return all the rows to a TableView

override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
    let cell : CustomTableViewController = self.tableView.dequeueReusableCell(withIdentifier: "Cell")! as! CustomTableViewController
    let DB : WeatherDataStorage = WeatherDataStorage()
    var arr = [(city : String, zip : String, temp : String)]()
    arr = DB.queryAll()
    cell.cityLabel.text = arr[i].city
    cell.zipLabel.text = arr[i].zip
    cell.temperatureLabel.text = arr[i].temp
    i += 1
    return cell
}

EDIT: GIST

Answer 1

1. Don't use tuples like this. The purpose of tuples is to make simple, short lived collections of values. For a use case like this, it is preferable to use a struct or a class to contain your results. There are many disadvantages.

2. Don't return non-sensical default values as a way of error handling. "" isn't a city, and "" isn't a valid zip code, so why try to claim them as such? This is what optionals are for, use them.

3. Narrow your do block to contain only the function calls that throw.

4. Avoid force casting if you can. Handle your errors sensibly.

5. Currently, you manually create an empty array, and repeatedly add to it in a for loop. This is discouraged for several reasons:

6. arr must be mutable.

7. It's a lot of boiler-plate code.

8. You can incur a lot of reallocation overhead, because you didn't call reserveCapacity on your array.

Using map is preferable instead.

6. Improve your naming. sql is a result set, it's not an sql code snippet, nor is it an sql connection. Name it so.

7. Don't use the pattern of: array += [newElement]. This technique causes an unnecessary array allocation. Instead, just use array.append(newElement).

8. Change your DB variable to not be an Optional. If there is something like a connection issue that causes DB to be nil, this should be handled at the time that you attempt to open the connection. Leaving it as an Optional, and using optional chaining DB?.doSomething() is a terrible idea. It makes the nil case fail silently, leading to very annoying to catch bugs.

9. "weather" is an awkward name for a table. Try something like "WeatherReading"

Here's how I would write this code:

struct WeatherReading {
    let city: String
    let zipCode: String
    let temperature: String
}

func queryAllWeatherReadings() -> [WeatherReading]?  {
    guard let queryResults = try? DB.prepare("SELECT city, zip, temperature FROM WeatherReading") else {
        debugPrint(error)
        return nil
    }

    let weatherReadings = queryResults.map { row in
        guard let city = row[0] as? String,
              let zipCode = row[1] as? String,
              let temperature = row[2] as? String else {
            fatalError("There was a type error in the received result set: \(row)")
        }

        return WeatherReading(city: city, zipCode: zipCode, temperature: temperature)
    }

    return weatherReadings
}