Code advice - What would you change?

[JamesW]

Hey guys,

Thought I'd post a little something I've been working on. I'd like to know what things people would do differently if they could change anything.

Don't laugh when you see it, I'm still very delicate from last night with Jon Von and Mike...

Always up for constructive criticism, and I am always looking to learn new things.

Option Explicit
Dim lRow, NPILRow, i, j, n As Integer
Dim FileToOpen As String
Dim MyBook, ThisWB As Workbook
Dim cell As Variant
Dim fileNCheck As VbMsgBoxResult

Sub Main()
    With Application
        .Calculation = xlCalculationManual
        .ScreenUpdating = False
    End With
    
    Set ThisWB = ThisWorkbook
    
    NPILRow = Range("C" & Rows.Count).End(xlUp).Row
    Range("S8:S" & NPILRow).Copy Destination:=Range("R8:R" & NPILRow)
    
    FileToOpen = Application.GetOpenFilename _
    (Title:="Please choose the latest xxx file", _
    FileFilter:="Excel Files *.xls (*.xls),")
    
    If FileToOpen = "False" Then
        MsgBox "No file specified."
        Exit Sub
    Else
        If Not FileToOpen Like "*xxx*" Then
            fileNCheck = MsgBox(FileToOpen & " is not a recognised file/filename.  Please ensure that you are using an APO file. " & vbNewLine & vbNewLine & "Do you wish to continue regardless? Doing so may produce incorrect results", vbYesNo)
            If fileNCheck = vbNo Then
                Exit Sub
            End If
        End If
        Set MyBook = Workbooks.Open(Filename:=FileToOpen)
    End If

    With MyBook.Sheets("Sheet1")
        lRow = .Range("AL" & Rows.Count).End(xlUp).Row
        For i = 2 To lRow
            For j = 38 To 193
                If .Cells(i, j).Value <> 0 Then
                    If j = 38 Then
                        .Range("AK" & i).Value = "From Now"
                        Exit For
                    Else
                        .Range("AK" & i).Value = Cells(1, j).Value
                        Exit For
                    End If
                Else
                    .Range("AK" & i).Value = "No FC"
                End If
            Next j
        Next i
    End With
    
    With ThisWB.Sheets("SKU Completed")
        .Range("S8").Value = "=VLOOKUP(RC[-16],'" & FileToOpen & "'!C1:C37,37,FALSE)"
        .Range("S8").AutoFill (.Range("S8:S" & NPILRow))
        .Range("T8").Value = "=IF(OR(RC[-1]=""No FC"",RC[-1]=""From Now""),""Unknown"",IF(and(MID(RC[-1],FIND(""."",RC[-1],1)+1,(FIND(""."",RC[-1],3)-FIND(""."",RC[-1],1)-1))+0-RC[-4]<=1,MID(RC[-1],FIND(""."",RC[-1],1)+1,(FIND(""."",RC[-1],3)-FIND(""."",RC[-1],1)-1))+0-RC[-4]>=-1),""OK"",""Issue""))"
        .Range("T8").AutoFill (.Range("T8:T" & NPILRow))
    End With
    
    ThisWB.Activate
    MyBook.Close SaveChanges:=True
    
    Range("S8:T" & NPILRow).Copy
    Range("S8:T" & NPILRow).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone
    
    With Application
        .Calculation = xlCalculationAutomatic
        .ScreenUpdating = True
    End With
End Sub

Cheers,

James

 

[Jon von der Heyden]

An immeditae observation:

    Dim lRow, NPILRow, i, j, n As Integer
    Dim FileToOpen As String
    Dim MyBook, ThisWB As Workbook
    Dim cell As Variant
    Dim fileNCheck As VbMsgBoxResult

lRow, NPILRow, i, j, MyBook are all variant data types. Only the last variable in each line is defined as a particular data type. It should be:

    Dim lRow [B][COLOR=Red]As Integer[/COLOR][/B], NPILRow [B][COLOR=Red]As Integer[/COLOR][/B], i[COLOR=Red][B] As Integer[/B][/COLOR], j [COLOR=Red][B]As Integer[/B][/COLOR], n As Integer
    Dim FileToOpen As String
    Dim MyBook [COLOR=Red][B]As Workbook[/B][/COLOR], ThisWB As Workbook
    Dim cell As Variant
    Dim fileNCheck As VbMsgBoxResult

Further, why are these defined outside of the main procedure?

Finally, are you sure you want to be using Integer data type?

 

[Jon von der Heyden]

The bit that worries me most is probably the bits in the code where the sub might be exited. What I mean is that using Exit Sub doesn't allow you to tidy up. You have changed application settings (screenupdating and calculation) at the start of the code. Ok, so in this instance screenupdating will change to True even if you use Exit Sub, but calculation will remain on manual. You should probably be telling it to go to a label, where everything beneath the label is about tidying up... E.g:

Public Sub Demo()
    Dim lngCalc As XlCalculation
    
    With Application
        .ScreenUpdating = False
        lngCalc = .Calculation
        .Calculation = xlManual
    End With
    
    'do stuff here
    
    If blnExit Then GoTo Finally
    
    'more stuff
    
    If blnExit Then GoTo Finally
    
    'more stuff
    
Finally:
    With Application
        .ScreenUpdating = True
        .Calculation = lngCalc
    End With
End Sub

 

[Jon von der Heyden]

I'll let you deal with my first two replies before I hit you with another 5 or 6.

The nice thing about your question is that your code includes various bits that can be improved, so if we stick with this then there are quite a number of good lessons you can learn of this one simple thread.

 

[chuckles1066]

An immeditae observation:
lRow, NPILRow, i, j, MyBook are all variant data types. Only the last variable in each line is defined as a particular data type. It should be:

    Dim lRow [B][COLOR=Red]As Integer[/COLOR][/B], NPILRow [B][COLOR=Red]As Integer[/COLOR][/B], i[COLOR=Red][B] As Integer[/B][/COLOR], j [COLOR=Red][B]As Integer[/B][/COLOR], n As Integer
    Dim FileToOpen As String
    Dim MyBook [COLOR=Red][B]As Workbook[/B][/COLOR], ThisWB As Workbook
    Dim cell As Variant
    Dim fileNCheck As VbMsgBoxResult

Really?

I've learned something new from this thread.

I assumed

 Dim a, b, c, d, e, f as Integer

meant all the variables were set to type Integer?

 

[JamesW]

I thought the same chuckles!

Cheers for the response Jon.

In answer to your questions:

I used Integer because, well, they are all numerical values. What is the reason for using Variant?

I defined them outside of the main procedure as it's a habit of mine that I picked up when I was first learning VBA (something to do with scope if I remember correctly - I suppose it doesn't matter in this scenario as the Sub is not private?)

Take 2:

Option Explicit


Sub Main()
    Dim lRow As Variant, NPILRow As Variant, i As Variant, j As Variant, n As Variant
    Dim FileToOpen As String
    Dim As Workbook, ThisWB As Workbook
    Dim cell As Range
    Dim fileNCheck As VbMsgBoxResult

    With Application
        .Calculation = xlCalculationManual
        .ScreenUpdating = False
    End With
    
    Set ThisWB = ThisWorkbook
    
    NPILRow = Range("C" & Rows.Count).End(xlUp).Row
    Range("S8:S" & NPILRow).Copy Destination:=Range("R8:R" & NPILRow)
    
    FileToOpen = Application.GetOpenFilename _
    (Title:="Please choose the latest APO file", _
    FileFilter:="Excel Files *.xls (*.xls),")
    
    If FileToOpen = "False" Then
        MsgBox "No file specified."
        Exit Sub
    Else
        If Not FileToOpen Like "*APO*" Then
            fileNCheck = MsgBox(FileToOpen & " is not a recognised file/filename.  Please ensure that you are using an APO file. " & vbNewLine & vbNewLine & "Do you wish to continue regardless? Doing so may produce incorrect results", vbYesNo)
            If fileNCheck = vbNo Then
                GoTo exitLabel
            End If
        End If
        Set MyBook = Workbooks.Open(Filename:=FileToOpen)
    End If

    With MyBook.Sheets("Sheet1")
        lRow = .Range("AL" & Rows.Count).End(xlUp).Row
        For i = 2 To lRow
            For j = 38 To 193
                If .Cells(i, j).Value <> 0 Then
                    If j = 38 Then
                        .Range("AK" & i).Value = "From Now"
                        Exit For
                    Else
                        .Range("AK" & i).Value = Cells(1, j).Value
                        Exit For
                    End If
                Else
                    .Range("AK" & i).Value = "No FC"
                End If
            Next j
        Next i
    End With
    
    With ThisWB.Sheets("SKU Completed")
        .Range("S8").Value = "=VLOOKUP(RC[-16],'" & FileToOpen & "'!C1:C37,37,FALSE)"
        .Range("S8").AutoFill (.Range("S8:S" & NPILRow))
        .Range("T8").Value = "=IF(OR(RC[-1]=""No FC"",RC[-1]=""From Now""),""Unknown"",IF(and(MID(RC[-1],FIND(""."",RC[-1],1)+1,(FIND(""."",RC[-1],3)-FIND(""."",RC[-1],1)-1))+0-RC[-4]<=1,MID(RC[-1],FIND(""."",RC[-1],1)+1,(FIND(""."",RC[-1],3)-FIND(""."",RC[-1],1)-1))+0-RC[-4]>=-1),""OK"",""Issue""))"
        .Range("T8").AutoFill (.Range("T8:T" & NPILRow))
    End With
    
    ThisWB.Activate
    MyBook.Close SaveChanges:=True
    
    Range("S8:T" & NPILRow).Copy
    Range("S8:T" & NPILRow).PasteSpecial Paste:=xlPasteValues, Operation:=xlNone
    
exitLabel:
    With Application
        .Calculation = xlCalculationAutomatic
        .ScreenUpdating = True
    End With
End Sub

 

[RoryA]

Never use an Integer as a row number variable as they only go up to 32767. Use a Long instead. Don't use Variants unless you have a reason for it.
Don't use Value if you are assigning a formula: use Formula or FormulaR1C1 depending on which syntax you are passing as the formula string (2003 is more forgiving than later versions about that)
As a general rule, variables should have the smallest scope possible.
Ignore people who tell you to shove Goto statements in your code (unless you are error handling and have no choice). This one is more personal preference than a 'rule' per se.

 

[JamesW]

Cheers Rory, I've made a few changes basd on what you said.

 

[RoryA]

One more:
Don't put parentheses around arguments unless you are returning a value from a function or using the Call statement.

 

[JamesW]

Could you explain a bit more Rory. Not entirely sure what you mean.