Unmaintainable code - iOS developer perspective

Slides from presentation at CocoaHeads Łódź (iOS wiOSłuje)

Transcript

1. 1 UNMAINTAINABLE CODE iOS Developer perspective
2. CocoaHeads Łódź | 30 MARCH 2018 2 Tomasz Gębarowski @tgebarowski www.codica.pl gebarowski@gmail.com
3. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 3
4. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 4
5. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 5 EXPERIENCE
6. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 6 IS IT EVERYTHING?
7. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 7 people are lazy juniors are afraid of seniors they think that they could be wrong people tend to ✅ auto-accept already reviewed code POOR QUALITY OF REVIEW PROCESS
8. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 7 people are lazy juniors are afraid of seniors they think that they could be wrong people tend to ✅ auto-accept already reviewed code POOR QUALITY OF REVIEW PROCESS
9. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 7 people are lazy juniors are afraid of seniors they think that they could be wrong people tend to ✅ auto-accept already reviewed code POOR QUALITY OF REVIEW PROCESS
10. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 7 people are lazy juniors are afraid of seniors they think that they could be wrong people tend to ✅ auto-accept already reviewed code POOR QUALITY OF REVIEW PROCESS
11. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 8 we start from trivial things learn from our errors and past decisions take your time use checklists HOW TO MASTER CODE REVIEWS?
12. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 9 we start from trivial things learn from our errors and past decisions take your time use checklists HOW TO MASTER CODE REVIEWS?
13. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 10 CODE REVIEW what is it all about?
14. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 11 code smells
15. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 12 a code smell is a surface indication that usually corresponds to a deeper problem in the system MARTIN FOWLER
16. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 13 code smells Method Level Class Level Application Level
17. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 14 Method Level Class Level Application Level Too many parameters Too short name Ambiguous names Excessive return of data SRP violation Cyclomatic complexity God line Excessively short identifiers Partial methods
18. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 15 Method Level Class Level Application Level Too many parameters Too short name Ambiguous names Excessive return of data SRP violation Cyclomatic complexity God line Excessively short identifiers lint SonarQube CodeBeat
19. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 16 Class Level Method Level Application Level Data clump Feature envy Excessive use of literals Downcasting Orphan variable Inappropriate intimacy Divergent change Strange side effects Refused Bequest Switch statement
20. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 17 Application Level Method Level Class Level Contrived complexity Duplicated code Speculative generality God object Inappropriate design patterns Shotgun surgery Middle man Inappropriate intimacy Parallel Inheritance Hierarchy
21. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 18 func parse(input: String) -> [Result] { } OPTIONALS 1 struct Result { let subject: String? let weight: Int? let mark: Double? }
22. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 18 func parse(input: String) -> [Result] { } OPTIONALS 1 struct Result { let subject: String? let weight: Int? let mark: Double? }
23. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 19 struct Result { let subject: String? let weight: Int? let mark: Double? } OPTIONALS 1
24. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 20 struct Result { let subject: String? let weight: Int? var mark: Double? { didSet { isGood = (mark ?? 0) >= 4 } } var isGood: Bool = false } OPTIONALS 1
25. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 21 OPTIONALS 1 class ResultPrinter { static func printDescription(result: Result) { print("\(result.subject ?? "No Name")") print("mark: \(result.mark ?? 0)") print("weight: \(result.weight ?? 1)") print(" -> \(isGood(result: result))") } private static func isGood(result: Result?) -> String { return result?.isGood ? "good" : "not good" } }
26. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 22 OPTIONALS func average(results: [Result]) -> Double { return Double(results.reduce(0, { return $0 + ($1.mark ?? 0) })) / Double(results.count) } 1
27. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 23 struct Result { let subject: String? let weight: Int? var mark: Double? { didSet { isGood = (mark ?? 0) >= 4 } } var isGood: Bool = false } CODE SMELLS (1) 1 Introducing side effects in setter
28. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 24 CODE SMELLS (2) func average(results: [Result]) -> Double { return Double(results.reduce(0, { return $0 + ($1.mark ?? 0) })) / Double(results.count) } 1 class ResultPrinter { static func printDescription(result: Result) { print("\(result.subject ?? "No Name")") print("mark: \(result.mark ?? 0)") print("weight: \(result.weight ?? 1)") print(" -> \(isGood(result: result))") } ... } Fallbacks + Divergent Change
29. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 25 CODE SMELLS (3) func average(results: [Result]) -> Double { return Double(results.reduce(0, { return $0 + ($1.mark ?? 0) })) / Double(results.count) } 1 class ResultPrinter { static func printDescription(result: Result) { print("\(result.subject ?? "No Name")") print("mark: \(result.mark ?? 0)") print("weight: \(result.weight ?? 1)") print(" -> \(isGood(result: result))") } ... } Feature Envy
30. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 26 func parse(input: String) -> [Result] { } struct Result { let subject: String let mark: Double let weight: Int } IMPROVEMENTS (1) Unwrap early Avoid fallbacks and not consistent default values Do not pass optional when not needed 1
31. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 27 IMPROVEMENTS (2) Eliminate side effects by introducing computed property 1 extension Result { var isGood: Bool { return mark > 4 } } extension Result: CustomStringConvertible { var description: String { return "Subject: \(subject ?? “Unknown”) ...” } } Get rid of Feature Envy class
32. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 27 IMPROVEMENTS (2) Eliminate side effects by introducing computed property 1 extension Result { var isGood: Bool { return mark > 4 } } extension Result: CustomStringConvertible { var description: String { return "Subject: \(subject ?? “Unknown”) ...” } } Get rid of Feature Envy class
33. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 28 QUESTION: Have you ever used class named BaseViewController? 2
34. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 29 open class BaseViewController: UIViewController { }
35. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 30 open class BaseViewController: UIViewController { var disableSwipeBack: Bool = true override open func viewWillAppear(_ animated: Bool) { super.viewWillAppear(animated) if disableSwipeBack { ... } } override open func viewWillDisappear(_ animated: Bool) { super.viewWillDisappear(animated) if disableSwipeBack { ... } } }
36. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 31 extension BaseViewController { func showActivityIndicator() { /* ... */ } func hideActivityIndicator() { /* ... */ } }
37. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 32 extension BaseViewController { func showActivityIndicator() { /* ... */ } func hideActivityIndicator() { /* ... */ } } extension BaseViewController { func handleLeftBarButtonItem() { /* ... */} var leftBarButtonItem: UIBarButtonItem? { return nil /* TODO */ } }
38. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 33 God object Refused Bequest - exposes functionality not needed by all subclasses Override to block certain behaviour BaseViewController is not a good name PITFALLS 2
39. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 33 God object Refused Bequest - exposes functionality not needed by all subclasses Override to block certain behaviour BaseViewController is not a good name PITFALLS 2
40. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 33 God object Refused Bequest - exposes functionality not needed by all subclasses Override to block certain behaviour BaseViewController is not a good name PITFALLS 2
41. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 33 God object Refused Bequest - exposes functionality not needed by all subclasses Override to block certain behaviour BaseViewController is not a good name PITFALLS 2
42. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 34 HOW TO MAKE IT BETTER? 2
43. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 35 BEWARE OF BASE CLASS SOLUTION 1: 2
44. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 36 PROTOCOLS + CHILD VIEW CONTROLLERS SOLUTION 2: 2
45. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 37 open class SwipeBackBlocker: UIViewController { override open func viewWillAppear(_ animated: Bool) { super.viewWillAppear(animated) popGestureRecognizer?.isEnabled = false popGestureRecognizer?.delegate = self } override open func viewWillDisappear(_ animated: Bool) { super.viewWillDisappear(animated) popGestureRecognizer?.isEnabled = true popGestureRecognizer?.delegate = nil } private var popGestureRecognizer: UIGestureRecognizer? { return navigationController?. interactivePopGestureRecognizer } }
46. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 38 public protocol SwipeBackBlockerType { func disableSwipeBack() } public extension SwipeBackBlockerType where Self: UIViewController { func disableSwipeBack() { let viewController = SwipeBackBlocker() addChildViewController(viewController) view.addSubview(viewController.view) viewController.view.alpha = 0 viewController.didMove(toParentViewController: self) } }
47. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 39 class ViewController: UIViewController { override func viewDidLoad() { super.viewDidLoad() (self as? SwipeBackBlockerType)?. disableSwipeBack() } } class UserViewController: MyApp.ViewController {} extension UserViewController: SwipeBackBlockerType {}
48. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 40 extension UserViewController: LeftNavigationItemBehaviorType { func handleLeftNavigationItemAction() { print("Execute action") } var leftNavigationItemName: String { return "Name" } } Activity Indicator Search Controller Empty States Right Navigation Bar Item
49. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 41 We all ❤ enums 3
50. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 42 We all ❤ enums enum ParagraphStyle { case header case body case title } 3
51. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 43 enum ParagraphStyle { case header case body case title var fontSize: CGFloat { switch self { case .header: return 16 case .body: return 14 case .title: return 12 } } } Computed properties 3
52. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 44 enum ParagraphStyle { case header case body case title case footer var fontSize: CGFloat { switch self { case .header: return 16 case .body: return 14 case .title: return 12 case .footer: return 12 } } } 3
53. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 45 enum ParagraphStyle { ... var fontSize: CGFloat { switch self { case .header: return 16 case .body: return 14 case .title: return 12 case .footer: return 12 } } var color: UIColor { switch self { case .header: return .blue case .body: return .black case .title: return .grey case .footer: return .black } } 3
54. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 45 enum ParagraphStyle { ... var fontSize: CGFloat { switch self { case .header: return 16 case .body: return 14 case .title: return 12 case .footer: return 12 } } var color: UIColor { switch self { case .header: return .blue case .body: return .black case .title: return .grey case .footer: return .black } } 3
55. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 46 Violates Open/Closed principle Switch statement smell Copy and paste PITFALLS 3
56. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 46 Violates Open/Closed principle Switch statement smell Copy and paste PITFALLS 3
57. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 46 Violates Open/Closed principle Switch statement smell Copy and paste PITFALLS 3
58. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 47 enum ParagraphStyle { struct Appearance { let color: UIColor let fontSize: Int } ... var appearance: Appearance { switch self { case .header: return Appearance(color: .blue, fontSize: 16) ... case .footer: return Appearance(color: .black, fontSize: 8) } } } aggregation SOLUTION 1 3
59. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 48 protocols SOLUTION 2 protocol ParagraphStyleType { var color: UIColor { get } var fontSize: CGFloat { get } } struct ParagraphStyle { struct Header: ParagraphStyleType { var color: UIColor = .blue var fontSize: CGFloat = 16 } struct Body: ParagraphStyleType { var color: UIColor = .black var fontSize: CGFloat = 12 } } 3
60. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 49 SOLUTION 2 extension ParagraphStyle { struct Footer: ParagraphStyleType { var color: UIColor = .black var fontSize: CGFloat = 8 } } struct ParagraphStyle { struct Header: ParagraphStyleType { var color: UIColor = .blue var fontSize: CGFloat = 16 } struct Body: ParagraphStyleType { var color: UIColor = .black var fontSize: CGFloat = 12 } } 3 protocols
61. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 50 SOLUTION 2 struct ParagraphStyle { struct Header: ParagraphStyleType { var color: UIColor = .blue var fontSize: CGFloat = 16 } struct Body: ParagraphStyleType { var color: UIColor = .black var fontSize: CGFloat = 12 } } 3 func load(style: ParagraphStyleType) { print("\(style.color) \(style.fontSize)") } load(style: ParagraphStyle.Footer()) protocols
62. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 51 Placeholder.xib 4 Let’s do some UI…
63. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 52 Placeholder.xib custom class 4
64. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 53 let’s make it generic 4
65. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 53 let’s make it generic 4
66. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 54 4
67. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 55 ENUM 4
68. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 55 ENUM 4
69. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 56 4
70. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 57 contrived complexity statically specified constraint values 4 switch smell
71. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 58 giphgip 4
72. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 58 giphgip 4
73. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 59 speculative generality -> wrong abstraction prone to regressions contrived complexity WHAT WENT WRONG? 4
74. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 59 speculative generality -> wrong abstraction prone to regressions contrived complexity WHAT WENT WRONG? 4
75. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 59 speculative generality -> wrong abstraction prone to regressions contrived complexity WHAT WENT WRONG? 4
76. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 59 speculative generality -> wrong abstraction prone to regressions contrived complexity WHAT WENT WRONG? 4
77. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 60 Placeholder.xib AvatarPlaceholder.xib Custom Class PREFER DUPLICATION OVER WRONG ABSTRACTION ButtonPlaceholder.xib + 4
78. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 61 5 class EventReporter { func send(domain: Events.Domain, scope: Events.Scope, action: Events.Action) { print("domain: \(domain), scope: \(scope) action: \(action))") } }
79. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 62 5 struct Events { enum Domain { case auth case featureA } enum Scope { case menu case button case view } enum Action { case login case logout } }
80. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 63 5 class AuthReporter { private let reporter = EventReporter() func reportUserLoginAction() { reporter.send(domain: .auth, scope: .button, action: .login) } func reportUserLogoutAction() { reporter.send(domain: .auth, scope: .button, action: .logout) } }
81. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 64 5 class AuthReporter { private let reporter = EventReporter() func reportUserLoginAction() { reporter.send(domain: .auth, scope: .button, action: .login) } func reportUserLogoutAction() { reporter.send(domain: .auth, scope: .button, action: .logout) } } Middle Man
82. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 65 5 class AuthReporter { private let reporter = EventReporter() func reportUserLoginAction() { reporter.send(domain: .auth, scope: .button, action: .login) } func reportUserLogoutAction() { reporter.send(domain: .auth, scope: .button, action: .logout) } } Middle Man Shotgun surgery
83. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 66 5 class AuthReporter { private let reporter = EventReporter() func reportUserLoginAction() { reporter.send(domain: .auth, scope: .button, action: .login) } func reportUserLogoutAction() { reporter.send(domain: .auth, scope: .button, action: .logout) } } Open Closed Principle violation struct Events { enum Domain { case auth case featureA } enum Scope { case menu case button case view } enum Action { case login case logout } }
84. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 67 5 SOLUTION protocol EventType { static var domain: String { get } associatedtype S associatedtype A } extension EventType { static var domain: String { return "\(self)" } }
85. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 68 5 SOLUTION protocol EventType { static var domain: String { get } associatedtype S associatedtype A } extension EventType { static var domain: String { return "\(self)" } } struct Auth: EventType { typealias S = Scope typealias A = Action enum Scope: String { case menu case button case view } enum Action: String { case login case logout } }
86. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 69 5 SOLUTION class EventReporter<T: EventType> { func send(scope: T.S, action: T.A) { print("domain: \(T.domain), scope: \(scope) action: \(action))") } }
87. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 70 5 SOLUTION class EventReporter<T: EventType> { func send(scope: T.S, action: T.A) { print("domain: \(T.domain), scope: \(scope) action: \(action))") } } let authReporter = EventReporter<Auth>() authReporter.send(scope: Auth.Scope.menu, action: Auth.Action.login) usage example:
88. CocoaHeads Łódź | 30 MARCH 2018 @tgebarowski 71 Bloaters Change Preventers Dispensables Couplers OO Abusers code smells Comments | Duplicated Code | 
 Dead code | Speculative generality Long names | Long classes | 
 Data clumps | Excessive return of data Divergent change | Shotgun surgery | Parallel Inheritance Hierarchy Middle man | Feature Envy | Inappropriate intimacy | Message chain Switch statement | Refused bequest | Temporary field
89. & Q&A questions answers 72 @tgebarowski
90. THANK YOU! 73 @tgebarowski