Unmaintainable code - IOS Developer Perspective - Mobilization 2017

Is code duplication the root of all evil? Should I always write a generic code? How to make others life harder? A subjective journey through common iOS anti-patterns, not so best practices and code smells from the perspective of an experienced Software Engineer. Based on +10 years of commercial experienced, thousands of code reviews, made with a dose of pragmatism.

Transcript

1. 1 UNMAINTAINABLE CODE iOS Developer perspective
2. Mobilization | 21 OCTOBER 2017 2 Tomasz Gębarowski @tgebarowski www.codica.pl gebarowski@gmail.com
3. Mobilization | 21 OCTOBER 2017 @tgebarowski 3
4. Mobilization | 21 OCTOBER 2017 @tgebarowski 4
5. Mobilization | 21 OCTOBER 2017 @tgebarowski 5 EXPERIENCE
6. Mobilization | 21 OCTOBER 2017 @tgebarowski 6 IS IT EVERYTHING?
7. Mobilization | 21 OCTOBER 2017 @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. Mobilization | 21 OCTOBER 2017 @tgebarowski 8 we start from trivial things learn from our errors and past decisions take your time use checklists HOW TO MASTER CODE REVIEWS?
9. Mobilization | 21 OCTOBER 2017 @tgebarowski 9 we start from trivial things learn from our errors and past decisions take your time use checklists HOW TO MASTER CODE REVIEWS?
10. Mobilization | 21 OCTOBER 2017 @tgebarowski 10 CODE REVIEW what is it all about?
11. Mobilization | 21 OCTOBER 2017 @tgebarowski 11 code smells
12. Mobilization | 21 OCTOBER 2017 @tgebarowski 12 a code smell is a surface indication that usually corresponds to a deeper problem in the system MARTIN FOWLER
13. Mobilization | 21 OCTOBER 2017 @tgebarowski 13 code smells Method Level Class Level Application Level
14. Mobilization | 21 OCTOBER 2017 @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
15. Mobilization | 21 OCTOBER 2017 @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
16. Mobilization | 21 OCTOBER 2017 @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
17. Mobilization | 21 OCTOBER 2017 @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
18. Mobilization | 21 OCTOBER 2017 @tgebarowski 18 func parse(input: String) -> [Result] { } OPTIONALS 1 struct Result { let subject: String? let weight: Int? let mark: Double? }
19. Mobilization | 21 OCTOBER 2017 @tgebarowski 19 struct Result { let subject: String? let weight: Int? let mark: Double? } OPTIONALS 1
20. Mobilization | 21 OCTOBER 2017 @tgebarowski 20 struct Result { let subject: String? let weight: Int? var mark: Double? { didSet { isGood = (mark ?? 0) >= 4 } } var isGood: Bool = false } OPTIONALS 1
21. Mobilization | 21 OCTOBER 2017 @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" } }
22. Mobilization | 21 OCTOBER 2017 @tgebarowski 22 OPTIONALS func average(results: [Result]) -> Double { return Double(results.reduce(0, { return $0 + ($1.mark ?? 0) })) / Double(results.count) } 1
23. Mobilization | 21 OCTOBER 2017 @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
24. Mobilization | 21 OCTOBER 2017 @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
25. Mobilization | 21 OCTOBER 2017 @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
26. Mobilization | 21 OCTOBER 2017 @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
27. Mobilization | 21 OCTOBER 2017 @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
28. Mobilization | 21 OCTOBER 2017 @tgebarowski 28 QUESTION: Have you ever used class named BaseViewController? 2
29. Mobilization | 21 OCTOBER 2017 @tgebarowski 29 open class BaseViewController: UIViewController { }
30. Mobilization | 21 OCTOBER 2017 @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 { ... } } }
31. Mobilization | 21 OCTOBER 2017 @tgebarowski 31 extension BaseViewController { func showActivityIndicator() { /* ... */ } func hideActivityIndicator() { /* ... */ } }
32. Mobilization | 21 OCTOBER 2017 @tgebarowski 32 extension BaseViewController { func showActivityIndicator() { /* ... */ } func hideActivityIndicator() { /* ... */ } } extension BaseViewController { func handleLeftBarButtonItem() { /* ... */} var leftBarButtonItem: UIBarButtonItem? { return nil /* TODO */ } }
33. Mobilization | 21 OCTOBER 2017 @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
34. Mobilization | 21 OCTOBER 2017 @tgebarowski 34 HOW TO MAKE IT BETTER? 2
35. Mobilization | 21 OCTOBER 2017 @tgebarowski 35 BEWARE OF BASE CLASS SOLUTION 1: 2
36. Mobilization | 21 OCTOBER 2017 @tgebarowski 36 PROTOCOLS + CHILD VIEW CONTROLLERS SOLUTION 2: 2
37. Mobilization | 21 OCTOBER 2017 @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 } }
38. Mobilization | 21 OCTOBER 2017 @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) } }
39. Mobilization | 21 OCTOBER 2017 @tgebarowski 39 class ViewController: UIViewController { override func viewDidLoad() { super.viewDidLoad() (self as? SwipeBackBlockerType)?. disableSwipeBack() } } class UserViewController: MyApp.ViewController {} extension UserViewController: SwipeBackBlockerType {}
40. Mobilization | 21 OCTOBER 2017 @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
41. Mobilization | 21 OCTOBER 2017 @tgebarowski 41 We all ❤ enums 3
42. Mobilization | 21 OCTOBER 2017 @tgebarowski 42 We all ❤ enums enum ParagraphStyle { case header case body case title } 3
43. Mobilization | 21 OCTOBER 2017 @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
44. Mobilization | 21 OCTOBER 2017 @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
45. Mobilization | 21 OCTOBER 2017 @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
46. Mobilization | 21 OCTOBER 2017 @tgebarowski 46 Violates Open/Closed principle Switch statement smell Copy and paste PITFALLS 3
47. Mobilization | 21 OCTOBER 2017 @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
48. Mobilization | 21 OCTOBER 2017 @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
49. Mobilization | 21 OCTOBER 2017 @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
50. Mobilization | 21 OCTOBER 2017 @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
51. Mobilization | 21 OCTOBER 2017 @tgebarowski 51 Placeholder.xib 4 Let’s do some UI…
52. Mobilization | 21 OCTOBER 2017 @tgebarowski 52 Placeholder.xib custom class 4
53. Mobilization | 21 OCTOBER 2017 @tgebarowski 53 let’s make it generic 4
54. Mobilization | 21 OCTOBER 2017 @tgebarowski 54 4
55. Mobilization | 21 OCTOBER 2017 @tgebarowski 55 ENUM 4
56. Mobilization | 21 OCTOBER 2017 @tgebarowski 56 4
57. Mobilization | 21 OCTOBER 2017 @tgebarowski 57 contrived complexity statically specified constraint values 4 switch smell
58. Mobilization | 21 OCTOBER 2017 @tgebarowski 58 giphgip 4
59. Mobilization | 21 OCTOBER 2017 @tgebarowski 59 speculative generality -> wrong abstraction prone to regressions contrived complexity WHAT WENT WRONG? 4
60. Mobilization | 21 OCTOBER 2017 @tgebarowski 60 Placeholder.xib AvatarPlaceholder.xib Custom Class PREFER DUPLICATION OVER WRONG ABSTRACTION ButtonPlaceholder.xib + 4
61. Mobilization | 21 OCTOBER 2017 @tgebarowski 61 5 class EventReporter { func send(domain: Events.Domain, scope: Events.Scope, action: Events.Action) { print("domain: \(domain), scope: \(scope) action: \(action))") } }
62. Mobilization | 21 OCTOBER 2017 @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 } }
63. Mobilization | 21 OCTOBER 2017 @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) } }
64. Mobilization | 21 OCTOBER 2017 @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
65. Mobilization | 21 OCTOBER 2017 @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
66. Mobilization | 21 OCTOBER 2017 @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 } }
67. Mobilization | 21 OCTOBER 2017 @tgebarowski 67 5 SOLUTION protocol EventType { static var domain: String { get } associatedtype S associatedtype A } extension EventType { static var domain: String { return "\(self)" } }
68. Mobilization | 21 OCTOBER 2017 @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 } }
69. Mobilization | 21 OCTOBER 2017 @tgebarowski 69 5 SOLUTION class EventReporter<T: EventType> { func send(scope: T.S, action: T.A) { print("domain: \(T.domain), scope: \(scope) action: \(action))") } }
70. Mobilization | 21 OCTOBER 2017 @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:
71. Mobilization | 21 OCTOBER 2017 @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
72. & Q&A questions answers 72 @tgebarowski
73. THANK YOU! 73 @tgebarowski