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

 

[RoryA]

No - if someone tells you to use GoSub, just chuckle, shake your head and walk away. They are beyond help.
Unless you absolutely need to save every femtosecond of processing time in your routine, GoSub just makes maintaining, or even reading, your code harder, in my opinion.

 

[Jon von der Heyden]

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.

But surely it's ok to use as an exit label instead of using Exit Sub? Or what alternative method do you use?

He had a pint and a half of Amstel last night! I was impressed...

I'd have had a couple more but I had to drive!

James, Rory nailed the comment I was leading onto. Avoid Integer in favour of Long Integer. I wasn't suggesting that you should use variants.

This:

Dim x, y As Long

x is a Variant because we haven't explicitly declared it of any data type.
y is a Long.

I'll try add my other points later today. I have to dash off now but should be around this evening...

 

[Colin Legg]

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?

Maybe you come from a VB .Net background, chuckles? In VB .Net, yes they would all be declared as Integer types but, as Jon pointed out, not in VB6 or VBA.

 

[chuckles1066]

Maybe you come from a VB .Net background, chuckles? In VB .Net, yes they would all be declared as Integer types but, as Jon pointed out, not in VB6 or VBA.

No, I'm entirely self-taught!

And now I'm a little older and a lot wiser

 

[RoryA]

But surely it's ok to use as an exit label instead of using Exit Sub? Or what alternative method do you use?

Never really had a need for it. Looking at the sample, you could basically move most of the last part of the code inside the If...End If and you'd have no need to jump around.

 

[JamesW]

Appreciate your help guys, and look forward to Jon's other 5991 billion pointers

 

[ZVI]

Rules without exception like beer without vodka, not sure about lemonade
Example below will not operate without parenthesis around Folder and Zip arguments:

Sub ExtractFromZipToFolder(Zip As String, Folder As String)
  With CreateObject("Shell.Application")
    ' Parenthesis around (Folder) and (Zip) are required!
    .Namespace((Folder)).CopyHere .Namespace((Zip)).Items
  End With
End Sub

Sub Test()
  ExtractFromZipToFolder "C:\Tmp\Archive.zip", "C:\Tmp"
End Sub

In this case parenthesis cause compiler to create required additional runtime variables.
The same can be achieved for example by Folder & "", Zip & "" as well.

 

[ZVI]

And what about the strict rules of declaring array variables?
If range values should be copied into array, what is better Dim arr() or Dim arr ?
The style of former looks preferable, but the latter is faster approx in 1.6 times:

Sub Test()
  
  Const N& = 10
  Dim t1!, t2!, i&, Rng As Range, varArray(), varVariable
  Set Rng = Range("A1:A60000")
  Rng.Value = "text"
  ReDim varArray(1 To Rng.Rows.Count, 1 To 1) ' <-- unnecessary
  
  ' Copy range values into array variable
  t1 = Timer
  For i = 1 To N
    varArray() = Rng.Value
  Next
  t1 = Timer - t1
  Debug.Print "varArray() ", "t1=" & Round(t1, 3)
  
  ' Copy range values to variant type variable
  t2 = Timer
  For i = 1 To N
    varVariable = Rng.Value
  Next
  t2 = Timer - t2
  Debug.Print "varVariable", "t2=" & Round(t2, 3)
  
  ' Show the ratio
  Debug.Print "t1 / t2 = " & Round(t1 / t2, 1) & " times"
  
End Sub

Possible rule - rules are good until in good sense

 

[RoryA]

In this case parenthesis cause compiler to create required additional runtime variables.
The same can be achieved for example by Folder & "", Zip & "" as well.

or by just declaring the inputs as Variant rather than String; or set a reference to the Shell library and declare a Shell object.

I suppose I should have explained that the reason for not using parentheses is that you dereference, or evaluate, the variable(s) you pass. 99.9% of the time this is not what you want, especially with objects, but as you say, there is an exception to every rule.

 

[JamesW]

ZVI, you've totally lost me now!