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]

For example:

.Range("T8").AutoFill (.Range("T8:T" & NPILRow))

should be:

.Range("T8").AutoFill .Range("T8:T" & NPILRow)

Although in this instance it will work as you are not specifying the optional argument for autofill, if you tried:

.Range("T8").AutoFill (.Range("T8:T" & NPILRow), xlfilldefault)

it will fail to compile as it should be:

.Range("T8").AutoFill .Range("T8:T" & NPILRow), xlfilldefault

In other instances, if you are passing an Object to a sub, you will get an Object required message - for example:

Sub testcall()
   test (Range("A1"))
End Sub
Sub test(rng As Range)
   MsgBox rng.Address
End Sub

The calling sub should be:

Sub testcall()
   test Range("A1")
End Sub

or:

Sub testcall()
   Call test(Range("A1"))
End Sub

 

[JamesW]

Ahh I see, thanks Rory - I never knew about that, and it's actually helped me out on another project as I was getting the Object required error!

 

[RoryA]

That's how good this site is - we even answer the questions you haven't posted yet.

 

[JamesW]

Jon and Mike inspired me last night to kick start the learning process (or was it the beer?).

You have been very helpful though Rory, beer shall be purchased (and consumed) for you!

 

[RoryA]

If Jon's involved, surely you mean Shandy, and not much of it?

 

[JamesW]

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

 

[RoryA]

With how much lemonade?

 

[JamesW]

None as far as I am aware.

 

[RoryA]

Bless - he's growing up.

 

[chuckles1066]

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.

I assume the same applies with GoSub?

But if you've used GoSub you usually need a GoTo to jump over the subroutine otherwise it executes twice (once as part of the initial call and once as the normal run through of code).

Alas, old habits die hard for those of us who learned our programming 30 years ago on ZX Sinclair Spectrums