InternalDevelopment

Fixing a Hole - A Refactoring Story

Refactoring, en, scala-lang, xuc

Introduction

I’m fixing a hole where the rain gets in, And stops my mind from wandering Where it will go, no it is not about The Beatles even if I am a fan but a story about a bug we discovered some days ago in Asterisk and the fix needed to be done.
We had a great discussion around this fix about the code to write and refactoring I wanted to share.

The Story

When a call is queued we receive an event from the Asterisk AMI (QueueCallerJoinEvent), so we are able to maintain a list of calls queued inside the Xuc memory. This list is used to calculate and send to the applications - as the CCManager - the number of calls queued and the longest waiting time. When a call is leaving the queue, another event is sent by Asterisk (QueueCallerLeaveEvent or QueueCallerAbandonEvent) and we can remove the call from the queue. The problem arises in some scenarios when the call is transferred to another queue: in this case Asterisk sends a specific event (AttendedTransferEvent) which enables us to track the call to the other queue. But this event is sometimes incorrect. So, first, we choose to implement a workaround inside the Xuc before having a patch available in Asterisk.
The idea is to remove the oldest call from the queue when we get a leave event with an unknown call id. This is probably the best guess !!

Test first

As usual when a problem is found and analysis is done we write a test to prove that the problem will be fixed. This is what we call Test Driven Development.

So let’s first write the test :

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
"remove oldest call from a queue when uniqueid is not found in list" in {
  val queueName = "foo"
  val oldestUniqueId = "000000.000"
  val newestUniqueId = "999999.999"
  val notFoundUniqueId = "123456.789"
  val oldestCall = QueueCall(4, "foo", "3345678",
             new DateTime(2017,9,10,11,Fixing a hole24))
  val newestCall = QueueCall(2, "bar", "44445678",
             new DateTime(2017,9,10,12,12))

  configRepository.queueCalls = Map(queueName ->
                                   Map(oldestUniqueId -> oldestCall,
                                       newestUniqueId -> newestCall))

  configRepository.onQueueCallFinished(queueName, notFoundUniqueId)

  configRepository.queueCalls shouldEqual
                           Map(queueName -> Map(newestUniqueId -> newestCall))
}

Now that we have a failing test, we can confidently starts to implement the solution in the code. After some thoughts we ended by having a first implementation.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
def onQueueCallFinished(queue: String, uniqueId: String): Unit = {
  def removeOldestCall(queueName: String): Unit = {
    def findOldest(callsList: List[(String, QueueCall)], oldestCall: (String, QueueCall)): String = {
      if (callsList.isEmpty) oldestCall._1
      else {
        val currentCall = callsList.head
        if (currentCall._2.queueTime.isBefore(oldestCall._2.queueTime))
          findOldest(callsList.tail, currentCall)
        else
          findOldest(callsList.tail, oldestCall)
      }
    }

    val listOfQueuesCalls = queueCalls(queueName).toList
    val oldestUniqueId = findOldest(listOfQueuesCalls, listOfQueuesCalls.head)

    queueCalls = queueCalls + (queueName -> (queueCalls(queueName) - oldestUniqueId))
  }

  queueCalls.get(queue) match {
    case None => log.warn(s"Received a call finished notification on the queue $queue which is not registered")
    case Some(calls) if calls.isEmpty =>
    case Some(calls) if ! calls.contains(uniqueId) => removeOldestCall(queue)
    case Some(calls) => queueCalls = queueCalls + (queue -> (calls - uniqueId))
  }
  Logger(LoggerName).trace(s"QUEUECALLS DEL queueName=$queue callId=$uniqueId queueAfter=${queueCalls.get(queue)}")
}

A few explanations of this extract:

  • we enter in the function onQueueCallFinished when we receive a QueueCallerLeaveEvent (cf. explanations above),
  • at line 20 we get the list of calls in the queue (note that queueCalls is a Map of Map, so at this step we get a Map),
  • and at line 23 we have the condition in case we don’t find the uniqueid of the call in the list: here we call the new function removeOldestCall.

Refining the first implementation

This is a nice implementation with a recursive function which finds the oldest call to be removed from the list. But as you may know, usually everything you may write on scala collection is already implemented in the language itself, the only thing you have to do is to dig into the documentation.
So we started to try to find a more elegant solution, and that’s the result :

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
def onQueueCallFinished(queueName: String, uniqueId: String): Unit = {

  def oldestCallId(calls: ConfigRepository.QueueCalls): String =
      calls.reduceLeft{
        (callA,callB) => if (callB._2.queueTime.isBefore(callA._2.queueTime)) callB else callA
      }._1

  queueCalls.get(queueName) match {
    case None =>
    log.warn(s"Received a call finished notification on the queue $queueName which is not registered")
    case Some(calls) if calls.isEmpty =>
    case Some(calls) if ! calls.contains(uniqueId) =>
      queueCalls = queueCalls  + (queueName -> (calls - oldestCallId(calls)))
    case Some(calls) =>
      queueCalls = queueCalls + (queueName -> (calls - uniqueId))
  }
  Logger(LoggerName).trace(s"QUEUECALLS DEL queueName=$queueName callId=$uniqueId queueAfter=${queueCalls.get(queueName)}")
}

So here we replaced our removeOldestCall function by another one named oldestCallId which returns the uniqueid of the oldest calls in the list. The main change versus the previous solution is that instead of browsing ourself the Map etc. we use a native method of a scala Map.
At first glance it seems to be better, fewer lines to read. But in fact it is really hard to understand without spending time what reduceLeft is doing. So, instead of using reduceLeft, why not sorting the list of calls by date and time and get the head of the result ?
Another problem is that the function oldestCallId we wrote is not respecting Totality: A function must yield a value for every possible input (See John A De Goes). Try to pass an empty list of calls and this function will crash with an exception. Ok it is protected by the previous pattern matching, but let’s imagine a developer sees this method and says That’s really cool, I can reuse this to my needs !! and reuse it without taking care of sending a non empty list of calls…

Reimplementing oldestCallId:

1
2
3
4
5
6
def oldestCallId(calls: QueueCalls): Option[String] =
      calls
        .toList
        .sortBy(_._2.queueTime.getMillis)
        .headOption
        .map(_._1)

Here we solved our two problems:

  • readibility: now it is far more easier to understand that we sort the calls list by their queueTime (i.e. their entry time) and that we get the first one,
  • totality: this is solved by using the .headOption method to retrieve the result of the sortBy: now if we pass an empty list of calls to this function, it will return a None

So, that’s it! We now have implemented our workaround to remove the oldest calls from the list. And doing this we improved our knowledge on Scala collections and functional programming.
Hmmm… have we really finished ?

Tidying

After having dived in this part of the code for some time we now see some other part to tidy :-)
Let’s look at the pattern matching block queueCalls.get(queueName) we can see that the call removal is duplicated, and this group of instructions mix two different purposes, getting the id of the call to be removed and removing the call.

So let’s split that a little:

1
2
3
4
5
6
7
8
9

def getId = queueCalls.get(queueName) match {
  case None => None
  case Some(calls) if calls.isEmpty => None
  case Some(calls) if ! calls.contains(uniqueId) => oldestCallId(calls)
  case Some(calls) => Some(uniqueId)
}

getId().foreach(id => queueCalls = queueCalls + (queueName -> (queueCalls.get(queueName) - uniqueId)))

Still have a not so readable pattern matching, and duplication of code, so let’ iterate a bit again…

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20

def onQueueCallFinished(queue: String, uniqueId: String): Unit = {

   def oldestCallId(calls: QueueCalls): Option[String] =
     calls
       .toList
       .sortBy(_._2.queueTime.getMillis)
       .headOption
       .map(_._1)

   def getUniqueId(calls:QueueCalls) =
     if (calls.contains(uniqueId)) Some(uniqueId)
     else oldestCallId(calls)

   for {
     calls <- queueCalls.get(queue)
     uid <- getUniqueId(calls)
   } queueCalls = queueCalls + (queue -> (calls - uid))

 }

And here it is:

  • we replaced the pattern matching by a for comprehension,
  • added the function getUniqueId which returns the uniqueid of the call to remove,
  • and we now update the queueCalls Map only once

Take some time to compare the first implementation, and the final one, don’t you think it is far more readable and understandable ? We had a very pleasant moment with the team working with this piece of code.
I hope you will enjoy reading this post and I would like to thank Etienne and Jean-Pierre to participate to this game.

And remember, even if it’s common to think a developer writes source code, he actually spends 90% of his time reading code !!!!

Do not hesitate to comment.

References

Share