Sqlite select and delete operation using C API in Swift

Question

I've following sqlite code using C api within Swift having a read and a delete operation for review.

For those not familiar with Swift, defer will execute after the end of the function.

Since I'm not closing the database connection and keeping it alive, I'm calling sqlite3_db_cacheflush(database) to flush out the changes to the disk. Link

class Database {
 
 internal var database: OpaquePointer! = nil
 
 init(_ database: OpaquePointer) {
 self.database = database
 }
 func deleteById(_ id: Int64) throws -> Int32? {
 let sql = "DELETE FROM user WHERE id = \(id)"
 
 defer {
 sqlite3_db_cacheflush(database)
 }
 
 guard sqlite3_exec(database, sql, nil, nil, nil) == SQLITE_OK else {
 throw NSError(domain: String(cString: sqlite3_errmsg(database)), code: 1, userInfo: nil)
 }
 
 return sqlite3_changes(database)
 }
 
 func findAllDepartments() throws -> [Department]? {
 var statement: OpaquePointer? = nil
 let sql = "SELECT id, name FROM department"
 
 guard sqlite3_prepare_v2(database, sql, -1, &statement, nil) == SQLITE_OK else {
 throw NSError(domain: String(cString: sqlite3_errmsg(database)), code: 1, userInfo: nil)
 }
 
 defer {
 sqlite3_finalize(statement)
 sqlite3_db_cacheflush(database)
 }
 
 var departments: [Department] = []
 while sqlite3_step(statement) == SQLITE_ROW {
 departments.append(Department(id: sqlite3_column_int64(statement, 0), name: String(cString: sqlite3_column_text(statement, 1))))
 }
 
 return departments
 }
}

Usage of Database Class

do {
 if sqlite3_open_v2(url.path, &database, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_FULLMUTEX, nil) == SQLITE_OK { // Existing database (Serialized mode)
 let db = Database(database)
 try! db.findAllDepartments()
 try! db.deleteById(4)
 } else { // Error opening database
 print(String(cString: sqlite3_errmsg(database)))
 }
catch { let error as NSError {
 print(error.description)
}

• MultiColourPixel: Have you considered making more of a wrapper around SQLite? So that you wouldn't really get the spilling over of opening a database where you're using the Database class?

Answer 1

You did not specify what precisely you were looking for, so a few general observations:

1. You do not need sqlite3_db_cacheflush. When you perform SQL updates, they are written to disk immediately (unless you are using transactions). You can do your inserts, updates, and deletes, leave the database open, and you’ll be fine.

2. Use Error, not NSError. I would advise against NSError (unless you needed Objective-C backward compatibility). In Swift, we would define our own Error type:

   enum DatabaseError: Error {
    case sqlError(Int32, String)
   }
   

   And then

   guard sqlite3_prepare_v2(database, sql, -1, &statement, nil) == SQLITE_OK else {
    throw DatabaseError.sqlError(sqlite3_errcode(database), String(cString: sqlite3_errmsg(database)))
   }
   

   Then, when catching errors, the codes and the error messages are more easily retrieved.

3. Failure of sqlite3_open_v2 will leak. When handling sqlite3_open_v2 failure, make sure to close the database or else resources allocated to it will never get freed. I know that this seems unintuitive, but the documentation is clear (emphasis added):

   Whether or not an error occurs when it is opened, resources associated with the database connection handle should be released by passing it to sqlite3_close() when it is no longer required.

4. Method names. You have a method called deleteById(_:), which does not make it clear what you are deleting. Besides the use of a preposition and parameter name in the method name is common in Objective-C, but not Swift. I would suggest deleteUser(id:) or something like that. That makes the domain and functional intent of the method clear, and moves the parameter label where it belongs.

5. Be careful with string interpolation of SQL. In your user deletion routine, you are building the SQL statement through string interpolation of the identifier. That happens to work because the parameters are numeric, but can be problematic with string parameters. Usually we would use a prepared statement and then bind values to ? placeholders in the SQL.

6. Methods that throw should not return optionals. Optionals are useful when writing functions that must return either nil or some value. But in this case, you always either return a value or throw errors, so use of optionals is not needed any only complicates the calling point. Remove the ? in the return values of E.g.,

   enum DatabaseError: Error {
    case sqlError(Int32, String)
   }
   func deleteUser(by identifier: Int64) throws -> Int32 {
    let sql = "DELETE FROM user WHERE id = \(identifier)"
    guard sqlite3_exec(database, sql, nil, nil, nil) == SQLITE_OK else {
    throw DatabaseError.sqlError(sqlite3_errcode(database), String(cString: sqlite3_errmsg(database)))
    }
    return sqlite3_changes(database)
   }
   func findAllDepartments() throws -> [Department] {
    var statement: OpaquePointer? = nil
    let sql = "SELECT id, name FROM department"
    guard sqlite3_prepare_v2(database, sql, -1, &statement, nil) == SQLITE_OK else {
    throw DatabaseError.sqlError(sqlite3_errcode(database), String(cString: sqlite3_errmsg(database)))
    }
    defer {
    sqlite3_finalize(statement)
    }
    var departments: [Department] = []
    while sqlite3_step(statement) == SQLITE_ROW {
    departments.append(Department(id: sqlite3_column_int64(statement, 0), name: String(cString: sqlite3_column_text(statement, 1))))
    }
    return departments
   }
   

   
   

7. Strive for highly cohesive, loosely coupled types. In this case, you have some other code opening the SQLite database and passing the sqlite3 pointer to the Database class. This is bad because it tightly couples the caller with the Database class, littering SQLite calls in multiple classes. It also is problematic because the Database type is less cohesive, not encompassing all the SQLite calls.

   If, for example, you replace SQLite with some other storage mechanism at some later date, you are going to have to not only replace your storage class, Database, but go through all the caller code, too.

8. Database is tightly coupled with model types. In a similar vein as my previous point, we generally want to keep model objects and database objects a little less tightly coupled than this. For example, rather than having Department specific methods in our base Database class, you would want to define a protocol for fetching and storing of data. Or, at the very least, I would move Department and other model-specific methods in their own extensions, to avoid cluttering the main Database class with model-specific methods.

9. Consider SQLite wrapper class. Before you write your own SQLite wrapper class, you might want to consider one of the existing ones out there. They have already wrestled with many of the basic considerations and might have interfaces to handle this.

Just a few observations.