Refactoring Risky Code

Refactoring Risky Code

How do you refactor Bluetooth code on Android with zero test coverage? When touching such code you have to test it with real phones and Bluetooth devices to ensure you are not breaking anything. Thus changes come with a high risk of introducing new bugs. At mySugr we spent quite some time refactoring our Bluetooth code, so that we can write plain Kotlin unit tests for it and easily add new functionality.

You are going to see how we achieved the refactoring with almost zero bugs and a minimal manual testing effort. This talk doesn’t deal with the details of Bluetooth on Android. Instead, we will discuss the strategies and methods we used and how it can be applied to any refactoring of dangerous code. Finally, I’m going to show you that you can even write tests for Bluetooth code - there is no excuse not to write tests.

D8796cfbfd245458dd193fbc4313d777?s=128

Philipp Hofmann

September 09, 2019
Tweet

Transcript

  1. Refactoring Risky Code Philipp Hofmann

  2. Risky Code • Has a low test coverage • Has

    a high test effort • Has many side effects National Geographic
  3. Legacy Code

  4. Implementing Bluetooth features on Android can be risky.

  5. Bluetooth features on Android are difficult for test automation.

  6. You need to test with lots of different devices.

  7. We had a stable Bluetooth stack.

  8. Bluetooth Stack • It had almost no tests • It

    was not testable • Could break a lot • Could make users angry National Geographic
  9. Make it extendable and changeable.

  10. None
  11. None
  12. None
  13. Things You Should Never Do Part 1

  14. Refactoring != Rewriting

  15. Refactoring

  16. Code refactoring is the process of restructuring existing code without

    changing its external behaviour. Wikipedia
  17. Code refactoring is the process of restructuring existing code without

    changing its external behaviour. Refactoring is intended to improve non functional attributes of the software. Wikipedia
  18. Code refactoring is the process of restructuring existing code without

    changing its external behaviour. Refactoring is intended to improve non functional attributes of the software. Advantages include improved code readability and reduced complexity; these can improve maintainability and create a more expressive internal architecture or object model to improve extensibility. Wikipedia
  19. Code refactoring is the process of restructuring existing code without

    changing its external behaviour. Refactoring is intended to improve non functional attributes of the software. Advantages include improved code readability and reduced complexity; these can improve maintainability and create a more expressive internal architecture or object model to improve extensibility. Wikipedia
  20. restructuring existing code without changing its external behaviour improve non

    functional attributes improved code readability and reduced complexity improve maintainability and extensibility Wikipedia
  21. Why should we refactor?

  22. “Any fool can write code that a computer can understand.

    Martin Fowler Good programmers write code that humans can understand.”
  23. 70 to 90% of our time we spend on understanding

    code. Robert C. Martin, Carlola Lilienthal, and The Mythical Man Month
  24. fun c( e: List<String> ): HashMap<String, Int> { var i

    = 0 val r = hashMapOf<String, Int>() while (i < e.size) { val ee = r[e[i]] if (ee != null) { r[e[i]] = ee + 1 } else { r[e[i]] = 1 } i++ } return r }
  25. fun countOccurences( elements: List<String> ): HashMap<String, Int> { var i

    = 0 val results = hashMapOf<String, Int>() while (i < elements.size) { val element = results[elements[i]] if (element != null) { results[elements[i]] = element + 1 } else { results[elements[i]] = 1 } i++ } return results }
  26. fun countOccurences( elements: List<String> ): List<Pair<String, Int>> = elements.groupBy {

    it }.map { Pair(it.key, it.value.count()) }
  27. https://cropper.watch.aetnd.com/cdn.watch.aetnd.com/sites/2/2018/06/Einstein-78970955.jpg

  28. None
  29. This time we do it right.

  30. Even the best get it wrong sometimes.

  31. Loose trust.

  32. You have do to the dishes.

  33. How to refactor code without breaking it?

  34. Tests.

  35. Loads of Tests.

  36. It’s safer to refactor when your code is covered by

    tests.
  37. Code without tests is a bad situation.

  38. It doesn’t matter how pretty or abstracted it is.

  39. Code without tests can also be a good situation.

  40. If you have testable code add tests then refactor.

  41. Bluetooth Stack • It had almost no tests • It

    was not testable • Could break a lot • Could make users angry National Geographic
  42. Refactoring Risky Code ?

  43. There are no tests. 
 Neither is the code testable.

  44. None
  45. ?

  46. make it testable risky code add tests refactor

  47. make it testable risky code

  48. Deep coding

  49. Get rid of not testable dependencies.

  50. Adding abstractions.

  51. class Greeting( private val context: Context, private val connectivityManager: ConnectivityManager

    ) { val greeting = if (isConnected) { context.getString(R.string.greeting_online) } else { context.getString(R.string.greeting_offline) } private val isConnected: Boolean get() { val activeNetworkInfo = connectivityManager.activeNetworkInfo return activeNetworkInfo != null && activeNetworkInfo.isConnected } }
  52. class Greeting( private val context: Context, private val connectivityManager: ConnectivityManager

    ) { val greeting = if (isConnected) { context.getString(R.string.greeting_online) } else { context.getString(R.string.greeting_offline) } private val isConnected: Boolean get() { val activeNetworkInfo = connectivityManager.activeNetworkInfo return activeNetworkInfo != null && activeNetworkInfo.isConnected } }
  53. class Connectivity( private val connectivityManager: ConnectivityManager ) { val isConnected:

    Boolean get() { val activeNetworkInfo = connectivityManager.activeNetworkInfo return activeNetworkInfo != null && activeNetworkInfo.isConnected } }
  54. class Greeting( private val context: Context, private val connectivity: Connectivity

    ) { val greeting = if (connectivity.isConnected) { context.getString(R.string.greeting_online) } else { context.getString(R.string.greeting_offline) } }
  55. class Greeting( private val context: Context, private val connectivity: Connectivity

    ) { val greeting = if (connectivity.isConnected) { context.getString(R.string.greeting_online) } else { context.getString(R.string.greeting_offline) } }
  56. interface GreetingResources { val greetingOnline: String val greetingOffline: String }

    class AndroidGreetingResources( private val context: Context ) : GreetingResources { override val greetingOnline: String get() = context.getString(R.string.greeting_online) override val greetingOffline: String get() = context.getString(R.string.greeting_offline) }
  57. class Greeting( private val resources: GreetingResources, private val connectivity: Connectivity

    ) { val greeting = if (connectivity.isConnected) { resources.greetingOnline } else { resources.greetingOffline } }
  58. None
  59. interface ClassicScanner { suspend fun scan(): ReceiveChannel<ClassicScanResult> }

  60. class AndroidClassicScanner( private val bluetoothProvider: BluetoothProvider, private val context: Context

    ) : ClassicScanner { override suspend fun scan(): ReceiveChannel<ClassicScanResult> { // All the other stuff } }
  61. is stupid. contains logic.

  62. Refactoring Survival Guide Those tips can save your career.

  63. 1. Refactoring. What else?

  64. When refactoring we don’t change the behaviour of the code.

  65. We improve the structure.

  66. I know what I am doing.

  67. No you don’t.

  68. 2. Keep your code green.

  69. None
  70. Miller Jackson

  71. Justin Miller Chris Jackson

  72. Justin Miller Frank Chris Jackson Tina

  73. Justin Miller Frank Chris Jackson Tina

  74. Justin Miller Frank Chris Jackson Tina

  75. Justin Miller Frank Chris Jackson Tina

  76. Justin Miller Frank Chris Jackson Tina

  77. Justin Miller Frank Chris Jackson Tina

  78. Justin Miller Frank Chris Jackson Tina

  79. Justin Miller Frank Chris Jackson Tina

  80. Justin Miller Frank Chris Jackson Tina

  81. 3. Small Tasks

  82. None
  83. Don’t go crazy.

  84. Clearly define a goal.

  85. Small Tasks • Merge often. • Avoid huge PRs.

  86. 4. Commit a lot

  87. When you reach a clear state, make a commit.

  88. git reset HEAD --hard

  89. Tiny steps

  90. None
  91. 5. Use the tools.

  92. Don’t do stuff manually.

  93. Android Studio is your friend.

  94. Find & Replace can help.

  95. 6. Keep your tests green.

  96. None
  97. None
  98. Don’t continue with failing tests.

  99. 7. Tests. Tests. Tests.

  100. None
  101. None
  102. enum class Level { Junior, Senior, Ancient } fun bonus(workingMonths:

    Int, salary: Float, level: Level) = if (workingMonths >= 12) { val bonusMultiplier = when (level) { Level.Junior -> 1.1f Level.Senior -> 1.15f Level.Ancient -> 1.30f } round(salary * bonusMultiplier) } else { salary }
  103. @Test fun `junior worked 11 months`() = assertEquals(40000f, bonus(11, 40000f,

    Level.Junior), 0.01f) @Test fun `junior worked 12 months`() = assertEquals(44000f, bonus(12, 40000f, Level.Junior), 0.01f) @Test fun `senior worked 12 months`() = assertEquals(57500f, bonus(12, 50000f, Level.Senior), 0.01f) @Test fun `ancient worked 12 months`() = assertEquals(65000f, bonus(12, 50000f, Level.Ancient), 0.01f)
  104. @Test fun `junior worked 23 months`() = assertEquals(40000f, bonus(23, 40000f,

    Level.Junior), 0.01f) @Test fun `junior worked 24 months`() = assertEquals(44000f, bonus(24, 40000f, Level.Junior), 0.01f) @Test fun `senior worked 12 months`() = assertEquals(57500f, bonus(12, 50000f, Level.Senior), 0.01f) @Test fun `ancient worked 12 months`() = assertEquals(65000f, bonus(12, 50000f, Level.Ancient), 0.01f)
  105. None
  106. fun bonus(workingMonths: Int, salary: Float, level: Level) = if (workingMonths

    >= 12) { val bonusMultiplier = when (level) { Level.Junior -> if (workingMonths >= 24) 1.1f else 1f Level.Senior -> 1.15f Level.Ancient -> 1.30f } round(salary * bonusMultiplier) } else { salary }
  107. None
  108. @Test fun `junior worked 23 months`() = assertEquals(40000f, bonus(23, 40000f,

    Level.Junior), 0.01f) @Test fun `junior worked 24 months`() = assertEquals(44000f, bonus(24, 40000f, Level.Junior), 0.01f) @Test fun `senior worked 11 months`() = assertEquals(50000f, bonus(11, 50000f, Level.Senior), 0.01f) @Test fun `senior worked 12 months`() = assertEquals(57500f, bonus(12, 50000f, Level.Senior), 0.01f) @Test fun `ancient worked 11 months`() = assertEquals(50000f, bonus(11, 50000f, Level.Ancient), 0.01f) @Test fun `ancient worked 12 months`() = assertEquals(65000f, bonus(12, 50000f, Level.Ancient), 0.01f)
  109. None
  110. @Test fun `junior worked 23 months`() = assertEquals(40000f, bonus(23, 40000f,

    Level.Junior), 0.01f) @Test fun `junior worked 24 months`() = assertEquals(44000f, bonus(24, 40000f, Level.Junior), 0.01f) @Test fun `senior worked 15 months`() = assertEquals(50000f, bonus(11, 50000f, Level.Senior), 0.01f) @Test fun `senior worked 16 months`() = assertEquals(57500f, bonus(16, 50000f, Level.Senior), 0.01f) @Test fun `ancient worked 11 months`() = assertEquals(50000f, bonus(11, 50000f, Level.Ancient), 0.01f) @Test fun `ancient worked 12 months`() = assertEquals(65000f, bonus(12, 50000f, Level.Ancient), 0.01f)
  111. None
  112. fun bonus(workingMonths: Int, salary: Float, level: Level) = if (workingMonths

    >= 12) { val bonusMultiplier = when (level) { Level.Junior -> if (workingMonths >= 24) 1.1f else 1f Level.Senior -> 1.15f Level.Ancient -> 1.30f } round(salary * bonusMultiplier) } else { salary }
  113. fun bonus(workingMonths: Int, salary: Float, level: Level) = salary *

    when (level) { Level.Junior -> if (workingMonths >= 24) 1.1f else 1f Level.Senior -> if (workingMonths > 12) 1.15f else 1f Level.Ancient -> if (workingMonths >= 12) 1.3f else 1f }
  114. fun bonus(workingMonths: Int, salary: Float, level: Level) = salary *

    when (level) { Level.Junior -> if (workingMonths >= 24) 1.1f else 1f Level.Senior -> if (workingMonths > 16) 1.15f else 1f Level.Ancient -> if (workingMonths >= 12) 1.3f else 1f }
  115. None
  116. @Test fun `junior worked 23 months`() = assertEquals(40000f, bonus(23, 40000f,

    Level.Junior), 0.01f) @Test fun `junior worked 24 months`() = assertEquals(44000f, bonus(24, 40000f, Level.Junior), 0.01f) @Test fun `senior worked 15 months`() = assertEquals(50000f, bonus(11, 50000f, Level.Senior), 0.01f) @Test fun `senior worked 16 months`() = assertEquals(57500f, bonus(16, 50000f, Level.Senior), 0.01f) @Test fun `ancient worked 11 months`() = assertEquals(50000f, bonus(11, 50000f, Level.Ancient), 0.01f) @Test fun `ancient worked 12 months`() = assertEquals(65000f, bonus(12, 50000f, Level.Ancient), 0.01f)
  117. @Test fun `senior worked 15 months`() = assertEquals(50000f, bonus(11, 50000f,

    Level.Senior), 0.01f)
  118. @Test fun `senior worked 15 months`() = assertEquals(50000f, bonus(15, 50000f,

    Level.Senior), 0.01f)
  119. None
  120. fun bonus(workingMonths: Int, salary: Float, level: Level) = salary *

    when (level) { Level.Junior -> if (workingMonths >= 24) 1.1f else 1f Level.Senior -> if (workingMonths > 16) 1.15f else 1f Level.Ancient -> if (workingMonths >= 12) 1.3f else 1f }
  121. fun bonus(workingMonths: Int, salary: Float, level: Level) = salary *

    when (level) { Level.Junior -> if (workingMonths >= 24) 1.1f else 1f Level.Senior -> if (workingMonths >= 16) 1.15f else 1f Level.Ancient -> if (workingMonths >= 12) 1.3f else 1f }
  122. None
  123. 8. Minimise The Damage

  124. Staged rollout.

  125. Use alpha testing.

  126. Put your refactoring behind a feature switch.

  127. You can roll back within a minute.

  128. We duplicated our code and made small changes to it.

  129. Secret status view for QA

  130. 9. Ship.

  131. It only counts when it’s shipped.

  132. Conclusion

  133. Refactoring is improving the structure of the code without changing

    its external behaviour.
  134. Refactoring Survival Guide 1. Refactoring. What else? 2. Keep your

    code green. 3. Keep your tests green. 4. Tests. Tests. Tests. 5. Small tasks. 6. Commit a lot. 7. Use the tools. 8. Minimise the damage. 9. Ship.
  135. make it testable risky code

  136. Deep coding.

  137. make it testable risky code

  138. make it testable risky code add tests refactor

  139. make it testable risky code add tests refactor @muffls Philipp

    Hofmann