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

Unmaintainable code - IOS Developer Perspective...

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.

Tomasz Gebarowski

October 21, 2017
Tweet

More Decks by Tomasz Gebarowski

Other Decks in Programming

Transcript

  1. 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
  2. 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?
  3. 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?
  4. 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
  5. 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
  6. 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
  7. 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
  8. 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
  9. Mobilization | 21 OCTOBER 2017 @tgebarowski 18 func parse(input: String)

    -> [Result] { } OPTIONALS 1 struct Result { let subject: String? let weight: Int? let mark: Double? }
  10. Mobilization | 21 OCTOBER 2017 @tgebarowski 19 struct Result {

    let subject: String? let weight: Int? let mark: Double? } OPTIONALS 1
  11. 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
  12. 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" } }
  13. 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
  14. 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
  15. 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
  16. 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
  17. 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
  18. 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
  19. Mobilization | 21 OCTOBER 2017 @tgebarowski 28 QUESTION: Have you

    ever used class named BaseViewController? 2
  20. 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 { ... } } }
  21. Mobilization | 21 OCTOBER 2017 @tgebarowski 31 extension BaseViewController {

    func showActivityIndicator() { /* ... */ } func hideActivityIndicator() { /* ... */ } }
  22. Mobilization | 21 OCTOBER 2017 @tgebarowski 32 extension BaseViewController {

    func showActivityIndicator() { /* ... */ } func hideActivityIndicator() { /* ... */ } } extension BaseViewController { func handleLeftBarButtonItem() { /* ... */} var leftBarButtonItem: UIBarButtonItem? { return nil /* TODO */ } }
  23. 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
  24. 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 } }
  25. 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) } }
  26. 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 {}
  27. 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
  28. Mobilization | 21 OCTOBER 2017 @tgebarowski 42 We all ❤

    enums enum ParagraphStyle { case header case body case title } 3
  29. 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
  30. 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
  31. 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
  32. 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
  33. 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
  34. 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
  35. 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
  36. Mobilization | 21 OCTOBER 2017 @tgebarowski 59 speculative generality ->

    wrong abstraction prone to regressions contrived complexity WHAT WENT WRONG? 4
  37. Mobilization | 21 OCTOBER 2017 @tgebarowski 60 Placeholder.xib AvatarPlaceholder.xib Custom

    Class PREFER DUPLICATION OVER WRONG ABSTRACTION ButtonPlaceholder.xib + 4
  38. 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))") } }
  39. 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 } }
  40. 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) } }
  41. 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
  42. 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
  43. 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 } }
  44. 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)" } }
  45. 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 } }
  46. 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))") } }
  47. 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:
  48. 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