Common mistakes discovered through code review

Common mistakes discovered through code review Dima Vorona, iOS Dev

Agenda

Agenda • Style and semantics • Language • Code/SDK behavior
understanding • No-tests-ever design • Database misuse • Files management • One target - one love • Model versioning

Agenda • Style and semantics • Language • Code/SDK behavior
understanding • No-tests-ever design • Database misuse • Files management • One target • Model versioning

Style and semantic

Style inconsistency Does it really matter? Style and semantics

Style inconsistency class SomeClass{ let my_property = 0 private(set)var anotherIntProperty:
Int = 1 init(){} func apply(value: Int){ if (value<=self.anotherIntProperty && value >= my_property) { print("Applied") }else{ print("Not applied") } } func printHello() { if my_property > 0{ print("Hello") } } } Does it really matter? Style and semantics

Style inconsistency Style and semantics Impact

Style inconsistency • Distracts your attention to understand Style and
semantics Impact

Style inconsistency • Distracts your attention to understand • Encourages
«I don’t give a shit» attitude (Broken windows theory) Style and semantics Impact

Style inconsistency • Distracts your attention to understand • Encourages
«I don’t give a shit» attitude (Broken windows theory) • Unprofessional look and feel Style and semantics Impact

Style inconsistency Style and semantics Solution

Style inconsistency • Understand importance of consistency (must!) Style and
semantics Solution

Style inconsistency • Understand importance of consistency (must!) • Define
or use coding guidelines consistently per (at least) File / Project Style and semantics Solution

or use coding guidelines consistently per (at least) File / Project • Discuss with team - PR rejection is not enough Style and semantics Solution

or use coding guidelines consistently per (at least) File / Project • Discuss with team - PR rejection is not enough • Use automation tools: SwiftLint, SwiftFormat Style and semantics Solution

Single process vs multiple terms Style and semantics

Single process vs multiple terms protocol UserModelDelegate: class { func
modelDidUpdate(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFailWithError: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } func loadData() { /* call API */ } } Style and semantics

modelDidUpdate(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFailWithError: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } func loadData() { /* call API */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { model.loadData() } Style and semantics

modelDidUpdate{What?}(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFailWithError: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } func loadData() { /* call API */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { model.loadData() } Style and semantics

modelDidUpdate{What?}(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFail{To do what?}WithError: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } func loadData() { /* call API */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { model.loadData() } Style and semantics

modelDidUpdate{What?}(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFail{To do what?}WithError: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } func loadData() { /* call API */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { // Recurring action invokes one-off method model.loadData() } Style and semantics

modelDidUpdate{What?}(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFail{To do what?}WithError: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } // False expectations: method can be used repeatedly func loadData() { /* call API */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { // Recurring action invokes one-off method model.loadData() } Style and semantics

Single process = single term protocol UserModelDelegate: class { func
modelDidReload(_ model: UserModelDelegate) func model(_ model: UserModelDelegate, didFailToReloadWithError error: Error) } class UserModel { weak var delegate: UserModelDelegate! var user: User { } var features: [Feature] { } func reload() { /* call API */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { model.reload() } Style and semantics

Ignoring SDK's API design guidelines Style and semantics

Ignoring SDK's API design guidelines extension Array where Element: Equatable
{ mutating func removeObject(_ object: Element) { if let index = self.index(of: object) { self.remove(at: index) } } mutating func removeObjectsInArray(_ array: [Element]) { for object in array { self.removeObject(object) } } } Style and semantics

{ // Two terms describe single entity mutating func removeObject(_ object: Element) { if let index = self.index(of: object) { self.remove(at: index) } } mutating func removeObjectsInArray(_ array: [Element]) { for object in array { self.removeObject(object) } } } Style and semantics

Ignoring SDK's API design guidelines // Array uses 'Element' term,
not Object extension Array where Element: Equatable { // Two terms describe single entity mutating func removeObject(_ object: Element) { if let index = self.index(of: object) { self.remove(at: index) } } mutating func removeObjectsInArray(_ array: [Element]) { for object in array { self.removeObject(object) } } } Style and semantics

Ignoring SDK's API design guidelines // Array uses 'Element' term,
not Object extension Array where Element: Equatable { // Two terms describe single entity mutating func removeObject(_ object: Element) { if let index = self.index(of: object) { self.remove(at: index) } } // Array.append(contentsOf:) mutating func removeObjectsInArray(_ array: [Element]) { for object in array { self.removeObject(object) } } } Style and semantics

{ mutating func remove(_ element: Element) { if let index = self.index(of: element) { self.remove(at: index) } } mutating func removeObjectsInArray(_ array: [Element]) { for object in array { self.removeObject(object) } } } Style and semantics

{ mutating func remove(_ element: Element) { if let index = self.index(of: element) { self.remove(at: index) } } mutating func remove<C: Collection>(contentsOf elements: C) where C.Iterator.Element == Element { for element in elements { self.remove(element) } } } Style and semantics

Ignoring SDK's API design guidelines // Factory method func createPhotosViewControllerForUser(user:
User) -> PhotosViewController { return /* ... */ } Style and semantics

User) -> PhotosViewController { return /* ... */ } // But public protocol Collection : ... { ... func makeIterator() -> Iterator } Style and semantics

User) -> PhotosViewController { return /* ... */ } // But public protocol Collection : ... { ... func makeIterator() -> Iterator } // Begin names of factory methods with “make”, e.g. x.makeIterator() // Factory method func makePhotosViewController(user: User) -> PhotosViewController { return /* ... */ } Style and semantics

Ignoring SDK's API design guidelines // NSDateFormatter-like class class PositionFormatter
{ func formattedPosition(_ position: Int) -> String } Style and semantics

{ func formattedPosition(_ position: Int) -> String } // Comparing to class DateFormatter : Formatter { ... func string(from date: Date) -> String func date(from string: String) -> Date? ... } Style and semantics

{ func string(from position: Int) -> String func position(from string: String) -> Int? } // Comparing to class DateFormatter : Formatter { ... func string(from date: Date) -> String func date(from string: String) -> Date? ... } Style and semantics

Style and semantics • Every detail matters • Name should
describe actual intent - don't give false expectations • Reuse existing API terms - don't pollute with new ones without a need • Follow vendor/recommended/any API design guidelines Links: • Swift API Design Guidelines: https://goo.gl/YT3LA5 • SE-0006: https://goo.gl/nX78X0 • How to name things: the hardest problem in programming: https://goo.gl/ZGEy81 • Hunting for great names in programming: https://goo.gl/H6zxcT • Erica Sadun: Swift protocol names: https://goo.gl/iM2re7 • Swift Protocol Naming Convention: https://goo.gl/2xnIxT Summary

Language

Optional.map .flatMap Still ignored Language

Optional.map .flatMap // Unwrapped value required for the result if
let title = user?.title { titleLabel.text = "Username: \(title)" } else { titleLabel.text = nil } Still ignored Language

Optional.map .flatMap // Unwrapped value required for the result titleLabel.text
= user?.title.map { "Username: \($0)" } Still ignored Language

= user?.title.map { "Username: \($0)" } // Ternary force unwrap func toString(_ value: Int?) -> String { return value != nil ? String(value!) : "empty string" } Still ignored Language

= user?.title.map { "Username: \($0)" } // Ternary force unwrap func toString(_ value: Int?) -> String { return value.map(String.init) ?? "" // or return value.map { String($0) } ?? "" } Still ignored Language

= user?.title.map { "Username: \($0)" } // Ternary force unwrap func toString(_ value: Int?) -> String { return value.map(String.init) ?? "" // or return value.map { String($0) } ?? "" } // Unwrapped value may produce optional value func imageData(at maybePath: String?) -> Data? { if let path = maybePath, let data = try? Data(contentsOf: URL(fileURLWithPath: path)) { return data } else { return nil } } Still ignored Language

= user?.title.map { "Username: \($0)" } // Ternary force unwrap func toString(_ value: Int?) -> String { return value.map(String.init) ?? "" // or return value.map { String($0) } ?? "" } // Unwrapped value may produce optional value func imageData(at maybePath: String?) -> Data? { return maybePath.flatMap { try? Data(contentsOf: URL(fileURLWithPath:$0)) } } Language Still ignored

No access control Language Discovered as

No access control • @IBAction / @IBOutlet declared as internal
• Properties that should be readonly for the client declared as readwrite • Utility methods declared as internal Language Discovered as

• Properties with private setter by intent declared as readwrite • Utility methods declared as internal Language Discovered as

• Properties with private setter by intent declared as readwrite • Most of the methods declared as internal Language Discovered as

No access control Language Impact

No access control • Gives false expectation to your teammates:
no difference between public and private anymore • Breaks "black box" by exposing implementation • Encourages to use fast and dirty workarounds Language Impact

No access control Language Solution

No access control • @IBAction private func login(_ sender:) -
still available to Interface Builder • use private(set) to hide mutability • Declare private first! Expose to public/ internal only when needed Language Solution

Language Summary

Language • Learn tools you're using everyday via: • Books
• Blogs • Practice! Summary

Code/SDK behavior understanding

Wrong assumptions about layout cycle Code/SDK behavior understanding

Wrong assumptions about layout cycle override func viewDidLoad() { super.viewDidLoad()
titleLabel.preferredMaxLayoutWidth = imageView.bounds.width } Code/SDK behavior understanding

Wrong assumptions about layout cycle override func viewDidLoad() { super.viewDidLoad()
// Wrong. Size might be incorrect titleLabel.preferredMaxLayoutWidth = imageView.bounds.width } Code/SDK behavior understanding

Awareness about layout cycle override func viewDidLayoutSubviews() { super.viewDidLayoutSubviews() titleLabel.preferredMaxLayoutWidth
= imageView.bounds.width view.layoutIfNeeded() } Code/SDK behavior understanding

Wrong assumptions about view lifecycle Code/SDK behavior understanding

Wrong assumptions about view lifecycle class UserCell: UITableViewCell { @IBOutlet
private var titleLabel: UILabel! // invoked on every tableView:cellForRowAtIndexPath: func apply(_ user: User) { titleLabel.textColor = UIColor.yellow titleLabel.font = UIFont.boldSystemFont(ofSize: 12) } } Code/SDK behavior understanding

private var titleLabel: UILabel! // invoked on every tableView:cellForRowAtIndexPath: func apply(_ user: User) { // UI configuration performed on every reuse titleLabel.textColor = UIColor.yellow titleLabel.font = UIFont.boldSystemFont(ofSize: 12) } } Code/SDK behavior understanding

private var titleLabel: UILabel! // invoked on every tableView:cellForRowAtIndexPath: func apply(_ user: User) { // UI configuration performed on every reuse titleLabel.textColor = UIColor.yellow titleLabel.font = UIFont.boldSystemFont(ofSize: 12) } // also found in layoutSubviews override func layoutSubviews() { super.layoutSubviews() // UI configuration performed during layout! titleLabel.textColor = UIColor.yellow titleLabel.font = UIFont.boldSystemFont(ofSize: 12) } } Code/SDK behavior understanding

Awareness about view lifecycle class UserCell: UITableViewCell { @IBOutlet private
var titleLabel: UILabel! override func awakeFromNib() { super.awakeFromNib() titleLabel.textColor = UIColor.yellow titleLabel.font = UIFont.boldSystemFont(ofSize: 12) } ... } Code/SDK behavior understanding

Closures: unowned, weak or strong? Code/SDK behavior understanding

Closures: unowned, weak or strong? @IBAction func doSomething() { DispatchQueue.main.async
{ [weak self] /* or [unowned self] */ in ... } } Code/SDK behavior understanding

{ [weak self] /* or [unowned self] */ in ... } } func dismissSomething() { self.dismiss(animated: true) { [unowned self] in ... } } Code/SDK behavior understanding

{ [weak self] /* or [unowned self] */ in ... } } func dismissSomething() { self.dismiss(animated: true) { [unowned self] in ... } } func animateChanges() { UIView.animate(withDuration: 0.2) { [weak self] in ... } } Code/SDK behavior understanding

{ [weak self] /* or [unowned self] */ in ... } } func dismissSomething() { self.dismiss(animated: true) { [unowned self] in ... } } func animateChanges() { UIView.animate(withDuration: 0.2) { [weak self] in ... } } func saveCoreDataContext() { context.performBlockAndWait { [unowned self] in ... } // or context.performBlock { [weak self] in ... } } Code/SDK behavior understanding

But why? Code/SDK behavior understanding

But why? • Breaking retain cycle - OK • Unawareness
of how particular code works • Ignorance of (non)deallocation impact Code/SDK behavior understanding

Retain cycles are OK. Use `strong` Code/SDK behavior understanding

Retain cycles are OK. Use `strong` • Temporary retain cycles
are OK when: • it is clear that a cycle will be broken • guarantees correct app execution • Not all API leads to the permanent retain cycles: • dispatch_(a)sync on the main/global/any unowned queue • UIView.animate • MOC.performBlock(AndWait) Code/SDK behavior understanding

Temporary retain cycles are OK data consistency Code/SDK behavior understanding

Temporary retain cycles are OK class AbuseDetector { let networkAPI:
API func reportAppState() { // collect usage data and send it to the server self.networkAPI.makeRequest { response in self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding

API func reportAppState() { // collect usage data and send it to the server self.networkAPI.makeRequest { [unowned self] response in self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding

API func reportAppState() { // collect usage data and send it to the server self.networkAPI.makeRequest { [unowned self] response in // crash! self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding

API func reportAppState() { // collect usage data and send it to the server self.networkAPI.makeRequest { [weak self] response in // no crash self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding

API func reportAppState() { // collect usage data and send it to the server self.networkAPI.makeRequest { [weak self] response in // no crash. But `self` was deallocated self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding

API func reportAppState() { // collect usage data and send it to the server self.networkAPI.makeRequest { response in // Better to not rely on external env. and to guarantee response handling self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding

Code/SDK behavior understanding • Seek to understand how and why
code does its job • Learn tools you're using everyday • Layout cycle isn't that straightforward • Fundamentals understanding is a must • Apple Guides is worth reading Links • objc.io: Advanced autolayout toolbox: https://goo.gl/ebc9jd • Apple: Changing constrains: https://goo.gl/UbWlhI • Krakendev: weak and unowned refs in swift: https://goo.gl/6eqYco Summary

Code/SDK behavior understanding • Seek to understand how and why
code does its job • Learn tools you're using everyday • Layout cycle isn't that straightforward • Apple Guides is worth reading Links • objc.io: Advanced autolayout toolbox: https://goo.gl/ebc9jd • Apple: Changing constrains: https://goo.gl/UbWlhI • Krakendev: weak and unowned refs in swift: https://goo.gl/6eqYco Summary

No-tests-ever design

Logic and transformations done in Void methods No-tests-ever design

Logic and transformations done in Void methods class RequestAuthorizer {
func authorize(request: URLRequest) { let authorizationRequired = // perform some non-trivial check let credentialKey = // key depends on the domain/protocol/api/etc if authorizationRequired, let credential = Keychain.getValue(for: credentialKey) { let headers = // form custom HTTP headers // append to the request } } } No-tests-ever design

func authorize(request: URLRequest) { let authorizationRequired = // perform some non-trivial check let credentialKey = // key depends on the domain/protocol/api/etc if authorizationRequired, let credential = Keychain.getValue(for: credentialKey) { let headers = // form custom HTTP headers // append to the request } } } // How to evaluate whether request requires authorization or not? No-tests-ever design

func authorize(request: URLRequest) { let authorizationRequired = // perform some non-trivial check let credentialKey = // key depends on the domain/protocol/api/etc if authorizationRequired, let credential = Keychain.getValue(for: credentialKey) { let headers = // form custom HTTP headers // append to the request } } } // How to evaluate whether request requires authorization or not? // How to evaluate credential key? No-tests-ever design

func authorize(request: URLRequest) { let authorizationRequired = // perform some non-trivial check let credentialKey = // key depends on the domain/protocol/api/etc if authorizationRequired, let credential = Keychain.getValue(for: credentialKey) { let headers = // form custom HTTP headers // append to the request } } } // How to evaluate whether request requires authorization or not? // How to evaluate credential key? // Hot to ensure that headers will be in the correct form? No-tests-ever design

func authorize(request: URLRequest) { let authorizationRequired = // perform some non-trivial check let credentialKey = // key depends on the domain/protocol/api/etc if authorizationRequired, let credential = Keychain.getValue(for: credentialKey) { let headers = // form custom HTTP headers // append to the request } } } // How to evaluate whether request requires authorization or not? // How to evaluate credential key? // Hot to ensure that headers will be in the correct form? // Side effects! No-tests-ever design

func authorize(request: URLRequest) { if let credential = credential(for: request) { for (key, value) in authorizationHeaders(for: credential) { // append to request headers } } } func shouldAuthorize(request: URLRequest) -> Bool { ... } func credential(for request: URLRequest) -> URLCredential? { if shouldAuthorize(request: request) { ... } else return nil } func headers(for credential: URLCredential) -> [String: String] { ... } } No-tests-ever design

func authorize(request: URLRequest) { if let credential = credential(for: request) { for (key, value) in authorizationHeaders(for: credential) { // append to request headers } } } func shouldAuthorize(request: URLRequest) -> Bool { ... } func credential(for request: URLRequest) -> URLCredential? { if shouldAuthorize(request: request) { ... } else return nil } func headers(for credential: URLCredential) -> [String: String] { ... } } // Testable! No-tests-ever design

No dependencies management No-tests-ever design

No dependencies management class Keychain { class func storeValue(_ value:
Any, for key: String) { ... } class func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { ... func credential(for request: URLRequest) -> URLCredential? { return Keychain.getValue(for: "key") } } No-tests-ever design

Any, for key: String) { ... } class func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { ... func credential(for request: URLRequest) -> URLCredential? { // No way to stub/mock/inject keychain easily return Keychain.getValue(for: "key") } } No-tests-ever design

Any, for key: String) { ... } class func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { ... func credential(for request: URLRequest) -> URLCredential? { // No way to stub/mock/inject keychain easily // No way to isolate tests environment return Keychain.getValue(for: "key") } } No-tests-ever design

No dependencies management class Keychain { func storeValue(_ value: Any,
for key: String) { ... } func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { ... func credential(for request: URLRequest) -> URLCredential? { return Keychain.getValue(for: "key") } } No-tests-ever design

for key: String) { ... } func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { init(keychain: Keychain) { ... } ... func credential(for request: URLRequest) -> URLCredential? { return keychain.getValue(for: "key") } } No-tests-ever design

for key: String) { ... } func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { init(keychain: Keychain) { ... } ... func credential(for request: URLRequest) -> URLCredential? { return keychain.getValue(for: "key") } } // Before let authorizer = RequestAuthorizer() // After. Hell for a legacy app let authorizer = RequestAuthorizer(keychain: Keychain()) No-tests-ever design

for key: String) { ... } func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { init(keychain: Keychain = Keychain.yourDefaultKeychain) { ... } ... func credential(for request: URLRequest) -> URLCredential? { return keychain.getValue(for: "key") } } // Before let authorizer = RequestAuthorizer() // After. Hell for a legacy app let authorizer = RequestAuthorizer(keychain: Keychain()) No-tests-ever design

for key: String) { ... } func getValue(for key: String) -> Any? { ... } } // later class RequestAuthorizer { init(keychain: Keychain = Keychain.yourDefaultKeychain) { ... } ... func credential(for request: URLRequest) -> URLCredential? { return keychain.getValue(for: "key") } } // Before let authorizer = RequestAuthorizer() // After let authorizer = RequestAuthorizer() No-tests-ever design

Callee is responsible for async No-tests-ever design

Callee is responsible for async class ResponseParser { func parseResponse<T>(_
response: [String: Any], completion: (T) -> Void) { DispatchQueue.global().async { // parse completion(result) } } } No-tests-ever design

response: [String: Any], completion: (T) -> Void) { // async is non-trivial for testing DispatchQueue.global().async { // parse completion(result) } } } No-tests-ever design

response: [String: Any]) -> T { // trivial to test sync code } } No-tests-ever design Let the caller be responsible for it (IoC)

Callee is responsible for async No-tests-ever design Use Executor pattern

Callee is responsible for async class ResponseParser { init(executor: Executor)
func parseResponse<T>(_ response: [String: Any], completion: (T) -> Void) { executor.execute { /* parse and completion */ } } } protocol Executor { func execute(_ work: (Void) -> Void) } No-tests-ever design Use Executor pattern

Callee is responsible for async class ResponseParser { init(executor: Executor
= DispatchQueueExecutor()) // default behavior func parseResponse<T>(_ response: [String: Any], completion: (T) -> Void) { executor.execute { /* parse and completion */ } } } protocol Executor { func execute(_ work: (Void) -> Void) } class DispatchQueueExecutor: Executor { func execute(_ work: (Void) -> Void) { ... } } No-tests-ever design Use Executor pattern

Callee is responsible for async class ResponseParser { init(executor: Executor
= DispatchQueueExecutor()) // default behavior func parseResponse<T>(_ response: [String: Any], completion: (T) -> Void) { executor.execute { /* parse and completion */ } } } protocol Executor { func execute(_ work: (Void) -> Void) } class DispatchQueueExecutor: Executor { func execute(_ work: (Void) -> Void) { ... } } // sync for testing class SyncExecutor: Executor { func execute(_ work: (Void) -> Void) { work() } } No-tests-ever design Use Executor pattern

No-tests-ever design No-tests-ever design Summary

No-tests-ever design No-tests-ever design • Designing with tests in mind:
Summary

• Really improves your API and code Summary

• Really improves your API and code • Makes your code "tests ready" for free Summary

• Really improves your API and code • Makes your code "tests ready" for free • Doesn't require to write tests immediately Summary

• Really improves your API and code • Makes your code "tests ready" for free • Doesn't require to write tests immediately • Saves you from some potential bugs Summary

Database misuse

One db to rule them all Database misuse

One db to rule them all • Faster to setup
- zero cost Database usage

- zero cost • Simpler to maintain in short-term Database usage

- zero cost • Simpler to maintain in short-term • Bundled with MR_defaultContext() / Realm() Database usage

- zero cost • Simpler to maintain in short-term • Bundled with MR_defaultContext() / Realm() • Leads to no-tests-ever design Database usage

- zero cost • Simpler to maintain in short-term • Bundled with MR_defaultContext() / Realm() • Leads to no-tests-ever design • Tricky to manage data from different users Database usage

- zero cost • Simpler to maintain in short-term • Bundled with MR_defaultContext() / Realm() • Leads to no-tests-ever design • Tricky to manage data from different users • Tricky to perform db cleanup Database usage

Logout cleanup Database misuse

Logout cleanup // UserA logged out Database misuse

Logout cleanup // UserA logged out // We need to
reset database for the UserB Database misuse

reset database for the UserB let path = "Documents/Database.sqlite" FileManager.default.removeItem(atPath: path) // Are we good to go? Database misuse

reset database for the UserB let path = "Documents/Database.sqlite" FileManager.default.removeItem(atPath: path) // Are we good to go? No! // How about: Database.sqlite-shm Database.sqlite-wal Database misuse

reset database for the UserB let path = "Documents/Database.sqlite" FileManager.default.removeItem(atPath: path) // Are we good to go? No! // How about: Database.sqlite-shm Database.sqlite-wal // Or maybe Database_SUPPORT/_EXTERNAL_DATA/ Database misuse

reset database for the UserB let path = "Documents/Database.sqlite" FileManager.default.removeItem(atPath: path) // Are we good to go? No! // How about: Database.sqlite-shm Database.sqlite-wal // Or maybe Database_SUPPORT/_EXTERNAL_DATA/ // Nobody cares! Database misuse

reset database for the UserB let coordinator: NSPersistentStoreCoordinator // Choose a store you'd like to reset let store = coordinator.persistentStores.first Database misuse Use framework API!

reset database for the UserB let coordinator: NSPersistentStoreCoordinator // Choose a store you'd like to reset let store = coordinator.persistentStores.first if let url = store.url { try coordinator.destroyPersistentStore(at: url, ofType: store.type) } // Add a new store using the same options Database misuse Use framework API!

Logout/Login in concurrent world Database misuse

Logout/Login in concurrent world // Statistics: started UserA's events upload
Database misuse

// UserA logged out Database misuse

// UserA logged out // UserB logged in Database misuse

// UserA logged out // UserB logged in // Statistics: received response Database misuse

// UserA logged out // UserB logged in // Statistics: received response // Statistics: writing UserA's results to the Database Database misuse

// UserA logged out // UserB logged in // Statistics: received response // Statistics: writing UserA's results to the Database // Results has been written to the UserB database or crashed due to the missing DB Database misuse

Logout/Login in concurrent world Database misuse A better approach

Logout/Login in concurrent world • Divide and conquer: Database misuse
A better approach

Logout/Login in concurrent world • Divide and conquer: • Keep
database(s) per user Database misuse A better approach

database(s) per user • Keep service(s) per user Database misuse A better approach

database(s) per user • Keep service(s) per user • Keep folders on the disk per user Database misuse A better approach

database(s) per user • Keep service(s) per user • Keep folders on the disk per user • Prefer DI over MR_defaultContext / Realm() to gain control over your resources Database misuse A better approach

Logout/Login in concurrent world Database and disk usage Ideally

Logout/Login in concurrent world // UserA.Statistics: started events upload Database
and disk usage Ideally

Logout/Login in concurrent world // UserA.Statistics: started events upload //
UserA logged out // UserB logged in Database and disk usage Ideally

UserA logged out // UserB logged in // UserA.Statistics: received response Database and disk usage Ideally

UserA logged out // UserB logged in // UserA.Statistics: received response // UserA.Statistics: writing results to the UserA.Database Database and disk usage Ideally

UserA logged out // UserB logged in // UserA.Statistics: received response // UserA.Statistics: writing results to the UserA.Database // UserA.Services are shutting down // UserA.Database removed or closed Database and disk usage Ideally

UserA logged out // UserB logged in // UserA.Statistics: received response // UserA.Statistics: writing results to the UserA.Database // UserA.Services are shutting down // UserA.Database removed or closed // Easy to achieve with DI Database and disk usage Ideally

Files management

Documents/ Common case Files management

Documents/ Documents !"" Database.sqlite !"" Database.sqlite-shm !"" Database.sqlite-wal #"" images
!"" 0e76292794888d4f1fa75fb3aff4ca27c58f56a6 !"" 7a85f4764bbd6daf1c3545efbbf0f279a6dc0beb #"" e5f26f9227e3ff4ece1431c2fc497c054d0e8269 Common case Files management

Documents/ Documents !"" user_a $ !"" Database.sqlite $ !"" ...
$ #"" ... #"" user_b !"" Database.sqlite !"" ... #"" ... With separated user data Files management

$ #"" ... #"" user_b !"" Database.sqlite !"" ... #"" ... // Are we done? With separated user data Files management

$ #"" ... !"" user_b $ !"" Database.sqlite $ !"" ... $ #"" ... #"" com.thirdPartySDK With separated user data Files management

$ #"" ... !"" user_b $ !"" Database.sqlite $ !"" ... $ #"" ... #"" com.thirdPartySDK FileManager.default.contentsOfDirectory(atPath: "Documents") // ["user_a", "user_b", "com.thirdPartySDK"] With separated user data Files management

Documents/ Documents !"" com.bundle.id $ !"" metadata $ #"" users
$ !"" a $ $ #"" ... $ #"" b $ #"" ... #"" com.thirdPartySDK With separated app data Files management

Separated app data Files management How-to

Separated app data • Use chdir-like approach Files management How-to

Separated app data • Use chdir-like approach • Don't let
services to choose location on their own Files management How-to

Separated app data • Use chdir-like approach • Don't let
services to choose location on their own • Pass "root" directory for every service Files management How-to

Separated app data How-to Files management

Separated app data struct Filepath { private(set) var url: URL
mutating func append(_ component: String) { ... } func appending(_ component: String) -> Filepath { ... } } How-to Files management

mutating func append(_ component: String) { ... } func appending(_ component: String) -> Filepath { ... } } let userFilepath = Filepath(user: "user") How-to Files management

mutating func append(_ component: String) { ... } func appending(_ component: String) -> Filepath { ... } } let userFilepath = Filepath(user: "user") let databasePath = userFilepath.appending("Database.sqlite").url // Documents/com.bundle.id/users/user/Database.sqlite How-to Files management

Files management When performed

Files management • Simplifies any resources cleanup Files management When
performed

Files management • Simplifies any resources cleanup • Simplifies migration
process Files management When performed

Files management • Simplifies any resources cleanup • Simplifies migration
process • Isolates resources from different services Files management When performed

One target - One love

One big, fat main target One target - one love
Issues

One big, fat main target • Easy to break layers
boundaries • Can not fully utilize `internal` access level • Long compilation time (Swift only) • Single namespace One target - one love Issues

One big, fat main target • Easy to break layers
boundaries • Can not fully utilize `internal` access level • Long compilation time (Swift only) One target - one love Issues

Several targets One target - one love Benefits

Several targets • Forces you to write cleaner API •
Better tests with @testable due to `internal` • Prevents you from leaking layers - no way to mix UI with Core.framework easily • Speeds up compilation (Swift only) One target - one love Benefits

func authorize(request: URLRequest) { ... } func shouldAuthorize(request: URLRequest) -> Bool { ... } func credential(for request: URLRequest) -> URLCredential? { ... } func headers(for credential: URLCredential) -> [String: String] { ... } } No-tests-ever design

Logic and transformations done in Void methods // RequestAuthorizer.swift in
your Core.framework to utilize @testable public class RequestAuthorizer { public func authorize(request: URLRequest) { ... } func shouldAuthorize(request: URLRequest) -> Bool { ... } func credential(for request: URLRequest) -> URLCredential? { ... } func headers(for credential: URLCredential) -> [String: String] { ... } } No-tests-ever design

Model versioning

Version number

Version number • Identifier of the model scheme • Required
during migration • Often is missing before first app update

Migration example Model versioning

Migration example func application(..., didFinishLaunchingWithOptions: ...) -> Bool { if
let storedModelVersion = ..., storedModelVersion < desiredModelVersion { // perform migration } return true } Model versioning

let storedModelVersion = ..., storedModelVersion < desiredModelVersion { // perform migration } return true } // What if storedModelVersion == nil, but we actually do have a data with old version? Model versioning

let storedModelVersion = ..., storedModelVersion < desiredModelVersion { // perform migration } return true } // What if storedModelVersion == nil, but we actually do have a data with old version? func application(..., didFinishLaunchingWithOptions: ...) -> Bool { let storedModelVersion = UserDefaults.standard.integer(forKey: "version") if storedModelVersion == Keychain.hasCredentialInOldFormat { // we're running untracked old model version } ... return true } Model versioning

let storedModelVersion = ..., storedModelVersion < desiredModelVersion { // perform migration } return true } // What if storedModelVersion == nil, but we actually do have a data with old version? func application(..., didFinishLaunchingWithOptions: ...) -> Bool { let storedModelVersion = UserDefaults.standard.integer(forKey: "version") if storedModelVersion == Keychain.hasCredentialInOldFormat { // we're running untracked old model version } ... return true } // Migration also can no be fully automated because of this special case Model versioning

Model versioning Summary

Model version • Define and store model version from the
first release • Use integers for simplified comparison • Model version != app version (810 vs 1.3) Summary

Thanks Dima Vorona, iOS Dev [email protected]

Common mistakes discovered through code review

Common mistakes discovered through code review

Transcript