Upgrade to Pro — share decks privately, control downloads, hide ads and more …

"Common mistakes discovered through code-review" by Dmitriy Vorona

"Common mistakes discovered through code-review" by Dmitriy Vorona

During last few years Dmitriy has made numerous code-reviews and project audits and discovered lots of mistakes in iOS projects. He've gathered the most common ones and want to share them with you. They vary a lot, from stylistics to design, architecture and concept issues. In his talk Dmitry will show alternative ways to solve same problems along and avoid most common mistakes. This should improve quality of your projects, make them tidier and neater.

This talk was made for CocoaHeads Kyiv #11 which took place March 04 2017.

CocoaHeads Ukraine

March 09, 2017
Tweet

More Decks by CocoaHeads Ukraine

Other Decks in Programming

Transcript

  1. Agenda • Style and semantics • Language • Code/SDK behavior

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

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

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

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

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

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

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

    understanding • No-tests-ever design • Database misuse • Files management • One target • Model versioning
  9. 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
  10. Style inconsistency • Distracts your attention to understand • Encourages

    «I don’t give a shit» attitude (Broken windows theory) Style and semantics Impact
  11. 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
  12. Style inconsistency • Understand importance of consistency (must!) • Define

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

    or use coding guidelines consistently per (at least) File / Project • Discuss with team - PR rejection is not enough Style and semantics Solution
  14. Style inconsistency • Understand importance of consistency (must!) • Define

    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
  15. 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
  16. 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
  17. 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 */ } } // Later in UserViewController.swift @IBAction func refreshControlValueChanged(_ sender: UIRefreshControl) { model.loadData() } Style and semantics
  18. Single process vs multiple terms protocol UserModelDelegate: class { func

    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
  19. Single process vs multiple terms protocol UserModelDelegate: class { func

    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
  20. Single process vs multiple terms protocol UserModelDelegate: class { func

    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
  21. Single process vs multiple terms protocol UserModelDelegate: class { func

    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
  22. 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
  23. 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
  24. 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
  25. Ignoring SDK's API design guidelines 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
  26. 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
  27. 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
  28. Ignoring SDK's API design guidelines extension Array where Element: Equatable

    { 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
  29. Ignoring SDK's API design guidelines extension Array where Element: Equatable

    { 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
  30. Ignoring SDK's API design guidelines // Factory method func createPhotosViewControllerForUser(user:

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

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

    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
  33. Ignoring SDK's API design guidelines // NSDateFormatter-like class class PositionFormatter

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

    { 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
  35. Ignoring SDK's API design guidelines // NSDateFormatter-like class class PositionFormatter

    { 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
  36. 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
  37. 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
  38. 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
  39. 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
  40. 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
  41. 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
  42. Optional.map .flatMap // Unwrapped value required for the result titleLabel.text

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

    = user?.title.map { "Username: \($0)" } // Ternary force unwrap func toString(_ value: Int?) -> String { return value != nil ? String(value!) : "empty string" } Still ignored Language
  44. Optional.map .flatMap // Unwrapped value required for the result titleLabel.text

    = 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
  45. Optional.map .flatMap // Unwrapped value required for the result titleLabel.text

    = 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
  46. Optional.map .flatMap // Unwrapped value required for the result titleLabel.text

    = 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
  47. 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
  48. No access control • @IBAction / @IBOutlet declared as internal

    • Properties with private setter by intent declared as readwrite • Utility methods declared as internal Language Discovered as
  49. No access control • @IBAction / @IBOutlet declared as internal

    • Properties with private setter by intent declared as readwrite • Most of the methods declared as internal Language Discovered as
  50. 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
  51. 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
  52. 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
  53. 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
  54. 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
  55. 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
  56. Wrong assumptions about layout cycle override func viewDidLoad() { super.viewDidLoad()

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

    // Wrong. Size might be incorrect titleLabel.preferredMaxLayoutWidth = imageView.bounds.width } Code/SDK behavior understanding
  58. 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
  59. Wrong assumptions about view lifecycle class UserCell: UITableViewCell { @IBOutlet

    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
  60. Wrong assumptions about view lifecycle class UserCell: UITableViewCell { @IBOutlet

    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
  61. 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
  62. Closures: unowned, weak or strong? @IBAction func doSomething() { DispatchQueue.main.async

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

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

    { [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
  65. Closures: unowned, weak or strong? @IBAction func doSomething() { DispatchQueue.main.async

    { [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
  66. But why? • Breaking retain cycle - OK • Unawareness

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

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

    of how particular code works • Ignorance of (non)deallocation impact Code/SDK behavior understanding
  69. 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
  70. 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
  71. 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
  72. Temporary retain cycles are OK class AbuseDetector { let networkAPI:

    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
  73. Temporary retain cycles are OK class AbuseDetector { let networkAPI:

    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
  74. Temporary retain cycles are OK class AbuseDetector { let networkAPI:

    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
  75. Temporary retain cycles are OK class AbuseDetector { let networkAPI:

    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
  76. 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 // Better to not rely on external env. and to guarantee response handling self.blockFurtherExecutionIfNeeded() } } } // usage AbuseDetector().reportAppState() data consistency Code/SDK behavior understanding
  77. 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
  78. 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
  79. 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
  80. 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
  81. 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
  82. 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
  83. 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 } } } // How to evaluate whether request requires authorization or not? No-tests-ever design
  84. 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 } } } // How to evaluate whether request requires authorization or not? // How to evaluate credential key? No-tests-ever design
  85. 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 } } } // 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
  86. 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 } } } // 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
  87. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  88. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  89. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  90. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  91. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  92. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  93. 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
  94. 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? { // No way to stub/mock/inject keychain easily return Keychain.getValue(for: "key") } } No-tests-ever design
  95. 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? { // No way to stub/mock/inject keychain easily // No way to isolate tests environment return Keychain.getValue(for: "key") } } No-tests-ever design
  96. 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
  97. No dependencies management class Keychain { func storeValue(_ value: Any,

    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
  98. No dependencies management class Keychain { func storeValue(_ value: Any,

    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
  99. No dependencies management class Keychain { func storeValue(_ value: Any,

    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
  100. No dependencies management class Keychain { func storeValue(_ value: Any,

    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
  101. 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
  102. Callee is responsible for async class ResponseParser { func parseResponse<T>(_

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

    response: [String: Any]) -> T { // trivial to test sync code } } No-tests-ever design Let the caller be responsible for it (IoC)
  104. 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
  105. 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
  106. 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
  107. No-tests-ever design No-tests-ever design • Designing with tests in mind:

    • Really improves your API and code • Makes your code "tests ready" for free Summary
  108. No-tests-ever design No-tests-ever design • Designing with tests in mind:

    • Really improves your API and code • Makes your code "tests ready" for free • Doesn't require to write tests immediately Summary
  109. No-tests-ever design No-tests-ever design • Designing with tests in mind:

    • 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
  110. One db to rule them all • Faster to setup

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

    - zero cost • Simpler to maintain in short-term Database usage
  112. One db to rule them all • Faster to setup

    - zero cost • Simpler to maintain in short-term • Bundled with MR_defaultContext() / Realm() Database usage
  113. One db to rule them all • Faster to setup

    - zero cost • Simpler to maintain in short-term • Bundled with MR_defaultContext() / Realm() • Leads to no-tests-ever design Database usage
  114. One db to rule them all • Faster to setup

    - 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
  115. One db to rule them all • Faster to setup

    - 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
  116. Logout cleanup // UserA logged out // We need to

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

    reset database for the UserB let path = "Documents/Database.sqlite" FileManager.default.removeItem(atPath: path) // Are we good to go? Database misuse
  118. Logout cleanup // UserA logged out // We need to

    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
  119. Logout cleanup // UserA logged out // We need to

    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
  120. Logout cleanup // UserA logged out // We need to

    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
  121. Logout cleanup // UserA logged out // We need to

    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!
  122. Logout cleanup // UserA logged out // We need to

    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!
  123. Logout/Login in concurrent world // Statistics: started UserA's events upload

    // UserA logged out // UserB logged in Database misuse
  124. Logout/Login in concurrent world // Statistics: started UserA's events upload

    // UserA logged out // UserB logged in // Statistics: received response Database misuse
  125. Logout/Login in concurrent world // Statistics: started UserA's events upload

    // UserA logged out // UserB logged in // Statistics: received response // Statistics: writing UserA's results to the Database Database misuse
  126. Logout/Login in concurrent world // Statistics: started UserA's events upload

    // 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
  127. Logout/Login in concurrent world • Divide and conquer: • Keep

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

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

    database(s) per user • Keep service(s) per user • Keep folders on the disk per user Database misuse A better approach
  130. Logout/Login in concurrent world • Divide and conquer: • Keep

    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
  131. Logout/Login in concurrent world // UserA.Statistics: started events upload //

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

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

    UserA logged out // UserB logged in // UserA.Statistics: received response // UserA.Statistics: writing results to the UserA.Database Database and disk usage Ideally
  134. Logout/Login in concurrent world // UserA.Statistics: started events upload //

    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
  135. Logout/Login in concurrent world // UserA.Statistics: started events upload //

    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
  136. Documents/ Documents !"" Database.sqlite !"" Database.sqlite-shm !"" Database.sqlite-wal #"" images

    !"" 0e76292794888d4f1fa75fb3aff4ca27c58f56a6 !"" 7a85f4764bbd6daf1c3545efbbf0f279a6dc0beb #"" e5f26f9227e3ff4ece1431c2fc497c054d0e8269 Common case Files management
  137. Documents/ Documents !"" user_a $ !"" Database.sqlite $ !"" ...

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

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

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

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

    $ !"" a $ $ #"" ... $ #"" b $ #"" ... #"" com.thirdPartySDK With separated app data Files management
  142. Separated app data • Use chdir-like approach • Don't let

    services to choose location on their own Files management How-to
  143. 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
  144. Separated app data struct Filepath { private(set) var url: URL

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

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

    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
  147. Files management • Simplifies any resources cleanup • Simplifies migration

    process • Isolates resources from different services Files management When performed
  148. 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
  149. 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
  150. 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
  151. 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
  152. 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
  153. 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
  154. 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
  155. 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
  156. Logic and transformations done in Void methods class RequestAuthorizer {

    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
  157. 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
  158. Version number • Identifier of the model scheme • Required

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

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

    during migration • Often is missing before first app update
  161. Migration example func application(..., didFinishLaunchingWithOptions: ...) -> Bool { if

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

    let storedModelVersion = ..., storedModelVersion < desiredModelVersion { // perform migration } return true } // What if storedModelVersion == nil, but we actually do have a data with old version? Model versioning
  163. Migration example func application(..., didFinishLaunchingWithOptions: ...) -> Bool { if

    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
  164. Migration example func application(..., didFinishLaunchingWithOptions: ...) -> Bool { if

    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
  165. 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
  166. 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
  167. 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
  168. Q&A