22

Tutorial: Spot the deliberate bug

 5 years ago
source link: https://www.tuicool.com/articles/hit/QJZRzaV
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.

[Difficulty: 1/5]

Don't worry if you're not an Android developer for this one, anyone can spot this bug (but surprisingly few do until it's too late - even with this warning!).

We are about to illustrate how complicated UI code can become (even for simple views), and then what to do about it.

Here's an ugly looking UI for a queue busting app that lets roaming staff sell $1 bottles of water at a festival (spoiler alert: this example is taken straight out of the fore docs )

URjyumj.png!web

ugly water seller app

As usual with MVO , we started by writing a model first (see the first tutorial for a refresher if you need it). We won't discuss the model here any further, other than to say: we called it Basket ; it is Observable ; it's quick and easy to unit test; the full kotlin source is in the github repo linked to below; and it has these public functions:

fun addBottle()
fun removeBottle()
fun setIsDiscounted(isDiscounted: Boolean)

fun getTotalItems(): Int
fun getTotalPrice(): Int
fun getIsDiscounted(): Boolean
fun getTotalSaving(): Int

The View Code

The rule of thumb for applying syncView() in MVO is: "If a model being observed changes in any way, then the entire view is refreshed." .

Let's first see what happens when we don't do that (i.e. how most view layer code is written):

Step 1 of 3

Let's start super simple:

FfeUfaY.png!web

ugly water seller app, first version

All we need to do is hook up the add and remove buttons in the UI and make sure we remember to update the total price . Something like this would be typical:

addButton.setOnClickListener {
    basket.addBottle()
    updateTotalPriceView()
}

removeButton.setOnClickListener {
    basket.removeBottle()
    updateTotalPriceView()
}

Don't worry about how we get the references for addButton and removeButton - this works fine as pseudo code (you can check the github source below if you're interested).

Also inside the updateTotalPriceView() call, we will just be setting the text in the UI to what the model tells us, essentially:

totalPriceView.text = basket.getTotalPrice()

Step 2 of 3

The designers have some inspiration and decide they want to add a basket icon:

EN7nymM.png!web

ugly water seller app, second version

This change is pretty easy as our model already supports it, so we just add an updateTotalNumberOfItemsView() method, which does exactly what you think it does. We'll also change the button click listeners so that they call both updateTotalPriceView() and updateTotalNumberOfItemsView()

addButton.setOnClickListener {
    basket.addBottle()
    updateTotalPriceView()
    updateTotalNumberOfItemsView()
}

removeButton.setOnClickListener {
    basket.removeBottle()
    updateTotalPriceView()
    updateTotalNumberOfItemsView()
}

Step 3 of 3

The business asks us to support a 10% discount for students. No problem, because we read ahead (...ahem), our model already has that capability.

rqueU3u.png!web

ugly water seller app, final version

So for the discount checkbox, we call basket.setIsDiscounted(isChecked) and then refresh the UI: updateTotalSavingsView() which shows the discount amount that has been applied, and also updateTotalPriceView() as that will have changed.

We don't call updateTotalNumberOfItemsView() (because of course, discounts have no effect there).

apply10PercOff.setOnCheckedChangeListener { isChecked ->
    basket.setIsDiscounted(isChecked)
    updateTotalPriceView()
    updateTotalSavingsView()
}

We end up with something like this to implement the view layer (this doesn't compile as we're only showing the important bits here - see the github repo for the full code):

lateinit var addItemButton: Button
lateinit var removeItemButton: Button
lateinit var apply10PercOff: CheckBox

lateinit var totalItemsView: TextView
lateinit var totalPriceView: TextView
lateinit var totalSavingView: TextView

private fun setupButtonListeners() {

  addButton.setOnClickListener {
      basket.addBottle()
      updateTotalPriceView()
      updateTotalNumberOfItemsView()
  }

  removeButton.setOnClickListener {
      basket.removeBottle()
      updateTotalPriceView()
      updateTotalNumberOfItemsView()
  }

  apply10PercOff.setOnCheckedChangeListener { isChecked ->
      basket.setIsDiscounted(isChecked)
      updateTotalPriceView()
      updateTotalSavingsView()
  }
}

private fun updateTotalNumberOfItemsView() {
    totalItemsView.text = basket.getTotalItems()
}

private fun updateTotalPriceView() {
    totalPriceView.text = basket.getTotalPrice()
}

private fun updateTotalSavingsView() {
    totalSavingView.text = basket.getTotalSaving()
}

private fun updatePostRotation(){
    updateTotalNumberOfItemsView()
    updateTotalPriceView()
    updateTotalSavingsView()
}

The updatePostRotation() method is there to handle android rotations. We probably also want some code to disable the remove button when the basket is empty etc, but this will do for our purposes - it's already complicated enough.

The Bug

The reason I say it's complicated enough, is that we already managed to introduce the bug. Did you spot it? How about if we just concentrate on the click listeners? (it's not a syntax bug, the bug is still present below if we treat the click listeners as psuedo code)

addButton.setOnClickListener {
      basket.addBottle()
      updateTotalPriceView()
      updateTotalNumberOfItemsView()
  }

  removeButton.setOnClickListener {
      basket.removeBottle()
      updateTotalPriceView()
      updateTotalNumberOfItemsView()
  }

  apply10PercOff.setOnCheckedChangeListener { isChecked ->
      basket.setIsDiscounted(isChecked)
      updateTotalPriceView()
      updateTotalSavingsView()
  }

In case you haven’t worked out the bug yet, follow the code in the click listeners when these actions are performed in sequence (focus on the amount in the savings field):

  • start with an empty basket
  • select the discount checkbox first
  • then add an item

BrARj2B.png!web

recreating the bug

We forgot to call updateTotalSavingsView() from the add and remove click listeners, so the savings value will be incorrect in the view unless the discount checkbox is toggled again.

UI consistency bugs like this happen all the time, even in simple views.

Unfortunately the view layer is slow and annoying to test (at least on Android), and even with automated tests, these types of bugs are very hard to spot. In this case, an automated test or human tester would have had to have performed specific actions in the correct sequence just to see it.

And this is why syncView() does what it does. "If a model being observed changes in any way, then the entire view is refreshed." .

Applying that principle, our code can be re-written like this:

lateinit var addItemButton: Button
lateinit var removeItemButton: Button
lateinit var apply10PercOff: CheckBox

lateinit var totalItemsView: TextView
lateinit var totalPriceView: TextView
lateinit var totalSavingView: TextView

private fun setupButtonListeners() {

  addButton.setOnClickListener {
      basket.addBottle()
  }

  removeButton.setOnClickListener {
      basket.removeBottle()
  }

  apply10PercOff.setOnCheckedChangeListener { isChecked ->
      basket.setIsDiscounted(isChecked)
  }
}

fun syncView() {
    totalItemsView.text = basket.getTotalItems()
    totalPriceView.text = basket.getTotalPrice()
    totalSavingView.text = basket.getTotalSaving()
}

This skips some details, if you want to know how it's all hooked up that's discussed here . For our purposes it's good enough to know that whenever our basket model changes, syncView() gets called.

What's surprising about this, is that it's not only more robust , it’s also less code . (And any android view can be written like this, including those using adapters).

Here are some tips for writing great syncView() functions.

rA7JzeF.gif

full app, rotation support as standard

This very simple app has no animation code, but it is: clear; robust; has no memory leaks; it's testable; and it supports rotation - which is a great place to be with android before you start adding beautiful animations and finishing touches. Check out the basic animation sample in the fore docs if you want explore further.

Thanks for reading! I hope you got something out of that, even if you're not an Android developer. If you're thinking of using fore in your team for an android project, the fore docs have most of the basics covered in easy to digest sample apps, e.g. adapters , networking or databases .

here’s the complete code for the tutorial


About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK