Return to Revisions

The indentation in TelekomRechnungEinlesen makes it hard to tell where it starts and where it ends. Sub and End Sub define a scope - anything in between should have a Tab:

Sub TelekomRechnungEinlesen()
    
    Dim lastSetTelekomRow As Long
    Dim lastSetNovatecRow As Long
    
    Dim iterator As Long
    Dim novatecSumColumn As String
    
    Dim mobilenumberSpalte As String
    Dim subSumFlag As String
    Dim bereichsSpalte As String
    Dim bereichsWert As String
    Dim telekomSumColumn As String
    
    Dim mobilenumber As String
    Dim NTmobilenumber As String
    
    ' used worksheet shortcuts
    Dim TKSheet As Worksheet
    Dim NTSheet As Worksheet
    
    ' final comparison required stuff
    Dim novatecSum As Double
    Dim telekomSum As Double
    Dim closeflag As Boolean

I used to do that too in VB6/VBA: declare all my variables at the top. I stopped doing that after seeing the benefits of declaring variables as close as possible to their usage. Sticking declarations at the top of a procedure is ok if your procedure is a 5-liner or similar. Clearly, that's not the case here. Let's see...

Dim mobilenumberSpalte As String
mobilenumberSpalte = "D"

Dim bereichsSpalte As String
bereichsSpalte = "F"

Dim novatecSumColumn As String
novatecSumColumn = "E"

Dim telekomSumColumn As String
telekomSumColumn = "O"

bereichsWert = "Telekom Deutschland GmbH"

These aren't really variables, they're more like constants. Declaring them as such clears a bunch of declarations and assignments from the function:

Private Const MobileNumberSpalte As String = "D"
Private Const BereichsSpalte As String = "F"
Private Const NovatecSumColumn As String = "E"
Private Const TelekomSumColumn As String = "O"

What the procedure actually does:

• Activate the first sheet of the first opened workbook, keep a reference to the active sheet.
• Get the last row with data on that sheet.
• Clear values of [E2:Ex], where x is the last row with data.

Break here. Scratch LastRowInGivenSheet and take a look at this fabulous Stack Overflow answer:

###Find Last Row in a Column

To find the last Row in Col E use this:

With Sheets("Sheet1")
    LastRow = .Range("E" & .Rows.Count).End(xlUp).Row
End With

_{http://stackoverflow.com/questions/11169445}

How about extracting functions out of that monster?

Private Function FindAndActivateFirstWorksheet() As Worksheet

    If Workbooks.Count = 0 Then Exit Function 'return Nothing

    Dim result As Worksheet
    Set result = Workbooks(1).Worksheets(1)

    result.Activate
    Set FindAndActivateFirstWorksheet = result

End Function

Private Function GetLastDataRow(xlSheet As Worksheet, Optional column As String = "A") As Long
    GetLastDataRow = xlSheet.Range(column & xlSheet.Rows.Count).End(xlUp).Row
End Function

Clearing the contents of a Range object is as easy as calling Range.ClearContents().

So, moving on:

• Bring up a FileDialog, open any Excel file that the user selects.
• Activate the 3rd sheet in the 2nd workbook (the one we just opened?) and reuse the NTSheet we don't need anymore, to keep a reference to the active sheet.

This should also be extracted in its own function that returns a Worksheet object, or Nothing if the user, say, cancelled out of the FileDialog. I believe your code... blows up if that happens, right? And what if the user opens a workbook that doesn't have the layout you're expecting, or if a user has inserted a worksheet and the one you're expecting 3rd is now 4th?

If possible, give the 3rd worksheet a specific name, and refer to that sheet by its name in code, and then gracefully handle the case where the expected sheet name isn't found in the selected workbook file; it's also likely that the workbooks that work with this macro are all called a certain specific way - you can validate that before opening the workbook file.

Avoid reusing variables. Doing that gives one identifier two different meanings! "Yeah, but I'm not reusing it anyway"? That makes it an even better reason to remove it from the procedure and put it in its own function.

Activating the 1st sheet in the 1st book and returning it, is pretty much like activating the 3rd sheet in the 2nd book and returning it, no?

Private Function FindAndActivateWorksheet(xlWorkbook As Workbook, Optional ByVal sheetIndex As Long, Optional ByVal sheetName As String) As Worksheet

    Dim isIndex As Boolean
    isIndex = Not IsMissing(sheetIndex)
    If Not isIndex And IsMissing(sheetName) Then
        isIndex = True
        sheetIndex = 1
    End If 

    Dim result As Worksheet
    If isIndex Then
        Set result = xlWorkbook.Worksheets(sheetIndex)
    Else
        Set result = xlWorkbook.Worksheets(sheetName)
    End If

    result.Activate
    Set FindAndActivateWorksheet = result

End Function

Now you can use that to find and activate the Nth sheet of any workbook, or to find and activate a sheet with a specific name, in any workbook - just pass in the workbook instance and what you're looking for. If there's an error (most likely something like an index out of range error), it's up to the caller to handle it.

The procedure is doing a lot more. Isolate each thing it does, extract it into a procedure or function; when you're done, it will read like plain English... or German. And you'll have a bunch of tiny little functions that do pretty much one single thing, with a "master" procedure at a high abstraction level, that anyone that can read could grasp and understand what it's doing, at a glance.