Add up & sort a spreadsheet column

I have the following code. It's purpose is to add up and sort another columns Data. I tweaked it to fulfil my purpose, but I'm sure it can be cleaned up.

The only problem I have with its function is it leaves a trailing zero if the last item in the referenced list is text.

Can this be changed without altering how it currently reacts to text? Occasionally, I want the zero's if they fall within the middle of the referenced cells, but never if the last cell is text.

My VBA skills are very basic. I can generally alter a piece of code to do what I want, but have no idea on how to fix it.

```
Public Sub SumCages()
Dim current_row, summary_row, item_total As Integer

current_row = 45
summary_row = 44
While Sheet8.Cells(current_row, 7) <> ""
If IsNumeric(Sheet8.Cells(current_row, 7)) Then
item_total = item_total + Val(Sheet8.Cells(current_row, 7))
Else
summary_row = summary_row + 1 ' Advance summary_row
If item_total > 0 Then
Sheet8.Cells(summary_row, 8) = item_total ' Display total
current_row = current_row - 1 ' Correct advancement
Else
Sheet8.Cells(summary_row, 8) = Sheet8.Cells(current_row, 7) ' Copy label
End If
item_total = 0 ' Reset item_total
End If
current_row = current_row + 1 ' Advance current_row
Wend
Sheet8.Cells(summary_row + 1, 8) = item_total

current_row = 45
summary_row = 44
While Sheet8.Cells(current_row, 11) <> ""
If IsNumeric(Sheet8.Cells(current_row, 11)) Then
item_total = item_total + Val(Sheet8.Cells(current_row, 11))
Else
summary_row = summary_row + 1 ' Advance summary_row
If item_total > 0 Then
Sheet8.Cells(summary_row, 12) = item_total ' Display total
current_row = current_row - 1 ' Correct advancement
Else
Sheet8.Cells(summary_row, 12) = Sheet8.Cells(current_row, 11) ' Copy label
End If
item_total = 0 ' Reset item_total
End If
current_row = current_row + 1 ' Advance current_row
Wend
Sheet8.Cells(summary_row + 1, 12) = item_total

current_row =

There are many iterative structures in VBA, but I think the one that would be most useful here is For...Next...Step; in your case, looks like Step should be 4, looping from 7 to 87.

All of your code blocks look very, very similar. I would only keep a single block and replace the hard-coded "magic numbers" with variables.

Something like this:

Private Const SUMMARY_ROW As Integer = 44
Private Const START_ROW As Integer = 45 'these two never change.
'...and if they ever do, that's the only place you need to change them.

Public Sub SumCages()
    Dim c As Integer
    For c = 7 To 87 Step 4 'start at 7 and +4 per iteration, up to 87
        CalculateTotal c
    Next
End Sub

The CalculateTotal procedure is nothing more than one of your code blocks, with the hard-coded row numbers replaced by the SumColumn parameter, which comes from the loop above.

Notice variables' names; variables are your friends, whenever you find yourself repeating a given operation (like, Sheet8.Cells(current_row + 1, SumColumn)), you can improve readability by adding a variable - this way at a glance you know two different places are talking about the same thing.

Private Sub CalculateTotal(sumColumn As Integer)

    Dim currentRow  As Integer
    Dim targetRow   As Integer
    Dim columnTotal As Integer
    Dim thisValue   As String

    currentRow = START_ROW
    targetRow = SUMMARY_ROW
    targetColumn = sumColumn + 1

    With Sheet8 ' this avoids having to specify it every time

        thisValue = .Cells(currentRow, sumColumn)

        While thisValue <> vbNullString

            If IsNumeric(thisValue) Then

                'this is where the sum actually takes place:
                columnTotal = columnTotal + Val(thisValue)

            Else
                If columnTotal > 0 Then

                    .Cells(targetRow + 1, targetColumn) = columnTotal

                Else

                    targetRow = targetRow + 1 'not sure I get why this is done...
                    currentRow = currentRow - 1 '...and this is even more confusing...
                    .Cells(targetRow, targetColumn) = thisValue

                End If

                'this is likely throwing off your total!
                columnTotal = 0
            End If

            currentRow = currentRow + 1
            thisValue = .Cells(currentRow, SumColumn)
        Wend

        .Cells(targetRow, targetColumn) = columnTotal

    End With
End Sub

It would be nice if you could post a screenshot of what the result looks like, because it's quite confusing (to me, at least) why you're incrementing your write-to row and then decrementing your loop counter, but only when you hit a non-numeric value.

To me it looks like what you have as current_row = current_row - 1 ' Correct advancement should really read summary_row = summary_row - 1 ' Correct advancement, because I would think it's an infinite loop written like this.

StackExchange Code Review Q#31075, answer score: 4