I had some time to kill today, and I found the Karate Chop Kata. Specification: Write a binary chop method that takes an integer search target and a sorted array of integers. It should return the  integer index of the target in the array, or -1 if the target is not  in the array.I've never implemented a binary search before. So, even though all of my tests pass, I'm not sure that I've covered all of the corner cases. It also doesn't look very elegant. How can I improve on this?I'm also still not sure that I'm unit testing in a proper way. How can I improve them? (Please keep in mind that Rubberduck automatically inserts boilerplate for new test methods.)ChopOption Explicit' Returns index of the target number in a given array.'   If not found returns -1Public Function Chop(target As Long, Arr() As Long, Optional midpoint As Long = -1) As Long    Dim result As Long    Dim currentTest As Long    If Not IsArrayAllocated(Arr) Then        Chop = -1        Exit Function    End If    If midpoint < 0 Then        midpoint = UBound(Arr) \ 2  'integer division    End If    currentTest = Arr(midpoint)    If target = currentTest Then        result = midpoint    ElseIf midpoint = 0 Or (Arr(UBound(Arr)) < target) Then        result = -1 'not found    Else        If target > currentTest Then            midpoint = midpoint + (midpoint \ 2) ' go up by half            If midpoint > UBound(Arr) Then midpoint = UBound(Arr)        Else            midpoint = midpoint \ 2        End If        result = Chop(target, Arr, midpoint)    End If    Chop = resultEnd Function'Borrorwed from Chip Pearson' http://www.cpearson.com/excel/isarrayallocated.aspxFunction IsArrayAllocated(Arr As Variant) As Boolean    On Error Resume Next    IsArrayAllocated = IsArray(Arr) And _                       Not IsError(LBound(Arr, 1)) And _                       LBound(Arr, 1) <= UBound(Arr, 1)End FunctionRubberduck Unit TestsOption ExplicitOption Private Module'@TestModulePrivate Assert As New Rubberduck.AssertClass'@TestMethodPublic Sub EmptyArrayReturnsNegativeOne()    On Error GoTo TestFailArrange:    Const expected As Long = -1    Dim integers() As LongAct:Assert:    Assert.AreEqual expected, Chop(2, integers)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub TargetNotInArrayReturnsNegativeOne()    On Error GoTo TestFailArrange:    Const expected As Long = -1    Const target As Long = 2    Dim integers(0) As Long    integers(0) = 3Act:Assert:    Assert.AreEqual expected, Chop(target, integers)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub TargetNotInArrayReturnsNegativeOne_LargerArray()    On Error GoTo TestFailArrange:    Const expected As Long = -1Act:Assert:    Assert.AreEqual expected, Chop(10, EvenSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub OddSizeArray_FirstReturnsZero()    On Error GoTo TestFailArrange:    Const expected As Long = 0Act:Assert:    Assert.AreEqual expected, Chop(1, OddSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub OddSizeArray_MiddleReturnsTwo()    On Error GoTo TestFailArrange:    Const expected As Long = 2Act:Assert:    Assert.AreEqual expected, Chop(3, OddSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub OddSizeArray_LastReturnsFour()    On Error GoTo TestFailArrange:    Const expected As Long = 4Act:Assert:    Assert.AreEqual expected, Chop(5, OddSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub EvenSizeArray_FirstReturnsZero()    On Error GoTo TestFailArrange:    Const expected As Long = 0Act:Assert:    Assert.AreEqual expected, Chop(1, EvenSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub EvenSizeArray_LastReturnsFive()    On Error GoTo TestFailArrange:    Const expected As Long = 5Act:Assert:    Assert.AreEqual expected, Chop(6, EvenSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd Sub'@TestMethodPublic Sub EvenSizeArray_NextToLastReturnsFour()    On Error GoTo TestFailArrange:    Const expected As Long = 4Act:Assert:    Assert.AreEqual expected, Chop(5, EvenSizeArray)TestExit:    Exit SubTestFail:    If Err.Number <> 0 Then        Assert.Fail Test raised an error: # & Err.Number &  -  & Err.Description    Else        Resume TestExit    End IfEnd SubPrivate Function OddSizeArray() As Long()    Dim result(0 To 4) As Long    Dim i As Long    For i = LBound(result) To UBound(result)        result(i) = i + 1    Next    OddSizeArray = resultEnd FunctionPrivate Function EvenSizeArray() As Long()    Dim result(0 To 5) As Long    Dim i As Long    For i = LBound(result) To UBound(result)        result(i) = i + 1    Next    EvenSizeArray = resultEnd Function
Karate Chop Kata
algorithm;unit testing;vba;binary search;rubberduck
Even though it matches the spec, Chop isn't a very good name forwhat this function does. IndexOf would be a better name.Ubound(Arr) gets called in this code a lot. It happens morethan enough to declare a variable for it.All of the parameters are being implicitly passed ByRef which isn't nice. (And you have no excuse. The add-in you're using warns you about this.)The optional parameter for midpoint is okay, but it exposes implementation details to the client. It would be better to create a private function that does all of the heavy lifting. It can require that the midpoint be specified.Public Function IndexOf(ByVal target As Long, ByRef Arr() As Long) As Long    IndexOf = RecursiveIndexOf(target, Arr, -1)End FunctionThis is a useless comment. If the maintainer doesn't know the difference between / and \ they can look it up.midpoint = upperBoundry \ 2  'integer division-1 is a magic number with two separate meanings. Constants should be defined.Private Const DefaultMidpoint As Integer = -1Public Const IndexOfResultNotFound As Long = -1It's not semantically correct to initialize the midpoint if midpoint < 0. It's correct to initialize it if midpoint = DefaultMidpoint.If midpoint = DefaultMidpoint Then    midpoint = upperBoundry \ 2End IfIt's not quite as DRY, but an ElseIf here instead of an Or improves readability. Considering it's merely assignment, I think it's a good change.ElseIf midpoint = 0 Or (Arr(upperBoundry) < target) Then    result = IndexOfResultNotFoundElseBecomesElseIf midpoint = 0 Then    result = IndexOfResultNotFoundElseIf Arr(upperBoundry) < target Then    result = IndexOfResultNotFoundElseThis logic can and should be extracted into a GetNextMidpoint function.    Else        If target > currentTest Then            midpoint = midpoint + (midpoint \ 2) ' go up by half            If midpoint > upperBoundry Then                midpoint = upperBoundry            End If        Else            midpoint = midpoint \ 2        End If        result = RecursiveIndexOf(target, Arr, midpoint)    End IfWhich gives us the refactored code below.Option ExplicitPrivate Const DefaultMidpoint As Integer = -1Public Const IndexOfResultNotFound As Long = -1' Returns index of the target number in a given array.'   If not found returns -1Public Function IndexOf(ByVal target As Long, ByRef Arr() As Long) As Long    IndexOf = RecursiveIndexOf(target, Arr, DefaultMidpoint)End FunctionPrivate Function RecursiveIndexOf(ByVal target As Long, ByRef Arr() As Long, ByVal midpoint As Long)    Dim result As Long    Dim currentTest As Long    If Not IsArrayAllocated(Arr) Then        RecursiveIndexOf = IndexOfResultNotFound        Exit Function    End If    Dim upperBoundry As Long    upperBoundry = UBound(Arr)    If midpoint = DefaultMidpoint Then        midpoint = upperBoundry \ 2    End If    currentTest = Arr(midpoint)    If target = currentTest Then        result = midpoint    ElseIf midpoint = 0 Then        result = IndexOfResultNotFound    ElseIf Arr(upperBoundry) < target Then        result = IndexOfResultNotFound    Else        midpoint = GetNextMidpoint(currentTest, target, upperBoundry, midpoint)        result = RecursiveIndexOf(target, Arr, midpoint)    End If    RecursiveIndexOf = resultEnd FunctionPrivate Function GetNextMidpoint(ByVal current As Long, ByVal target As Long, ByVal upperBoundry As Long, ByVal midpoint As Long) As Long    Dim result As Long    If target > current Then        result = midpoint + (midpoint \ 2) ' go up by half        If result > upperBoundry Then            result = upperBoundry        End If    Else        result = midpoint \ 2    End If    GetNextMidpoint = resultEnd Function'Borrorwed from Chip Pearson' http://www.cpearson.com/excel/isarrayallocated.aspxFunction IsArrayAllocated(Arr As Variant) As Boolean    On Error Resume Next    IsArrayAllocated = IsArray(Arr) And _                       Not IsError(LBound(Arr, 1)) And _                       LBound(Arr, 1) <= UBound(Arr, 1)End Function