defining LastRow in VBA

Question

I am trying to export data from excelsheet to excel Invoice template. The VBA code which I have, It considers each row as a different Invoice and hence makes a different workbook for each row. So in case I have 1 invoice which has 3 products this code considers each of the product (row) as separate Invoice which is not correct. I want to modify it in a way that if the Invoice number (PiNo) is repeated in the next row then it means the next product (row) belongs to the above Invoice only. I am new to VBA hence I have taken code from another site.

Here is the code:-

   Private Sub CommandButton1_Click()
   Dim customername As String
   Dim customeraddress As String
   Dim invoicenumber As Long
   Dim r As Long
   Dim mydate As String
   Dim path As String
   Dim myfilename As String
   lastrow = Sheets(“CustomerDetails”).Range(“H” & Rows.Count).End(xlUp).Row
   r = 2
   For r = 2 To lastrow

   ClientName = Sheets("CustomerDetails").Cells(r, 6).Value
   Address = Sheets("CustomerDetails").Cells(r, 13).Value
   PiNo = Sheets("CustomerDetails").Cells(r, 5).Value
   Qty = Sheets("CustomerDetails").Cells(r, 9).Value
   Description = Sheets("CustomerDetails").Cells(r, 12).Value
   UnitPrice = Sheets("CustomerDetails").Cells(r, 10).Value
   Salesperson = Sheets("CustomerDetails").Cells(r, 1).Value
   PoNo = Sheets("CustomerDetails").Cells(r, 3).Value
   PiDate = Sheets("CustomerDetails").Cells(r, 4).Value
   Paymentterms = Sheets("CustomerDetails").Cells(r, 7).Value
   PartNo = Sheets("CustomerDetails").Cells(r, 8).Value
   Shipdate = Sheets("CustomerDetails").Cells(r, 14).Value
   Dispatchthrough = Sheets("CustomerDetails").Cells(r, 15).Value
   Modeofpayment = Sheets("CustomerDetails").Cells(r, 16).Value
   VAT = Sheets("CustomerDetails").Cells(r, 17).Value

   Workbooks.Open ("C:\Users\admin\Desktop\InvoiceTemplate.xlsx")
   ActiveWorkbook.Sheets("InvoiceTemplate").Activate
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“Z8”).Value = PiDate
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“AG8”).Value = PiNo
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“AN8”).Value = PoNo
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“B16”).Value = ClientName
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“B17”).Value = Address
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“B21”).Value = Shipdate
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“K21”).Value = Paymentterms
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“T21”).Value = Salesperson
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“AC21”).Value = Dispatchthrough
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“AL21”).Value = Modeofpayment
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“B25”).Value = PartNo
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“J25”).Value = Description
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“Y25”).Value = Qty
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“AF25”).Value = UnitPrice
   ActiveWorkbook.Sheets("InvoiceTemplate").Range(“AL39”).Value = VAT

   path = "C:\Users\admin\Desktop\Invoices\"
   ActiveWorkbook.SaveAs Filename:=path & PiNo & “.xlsx”
   myfilename = ActiveWorkbook.FullName
   ActiveWorkbook.Close SaveChanges:=True

   Next r

   End Sub

"H" is the Product column and the data starts from Row 2. Row 1 are headers.

enter image description here

enter image description here

enter image description here

Answer 1

There are a few areas to address when it comes to what you want to achieve vs. your current code. I'll outline this below in no particular order and provide some sample code to help you achieve what you want.

1. Your code to create, save and close the new workbook is contained within your for loop. This means that if we assume you have 3 products to add to your invoice, your code is going to open the InvoiceTemplate.xlsx from the desktop a total of 3 times and also save it and close it with the PiNo name 3 times. To fix this you should move the code to open the workbook before your For loop starts and move the SaveAs code to after the loop so it's only saving and closing the workbook once.
2. Your code uses .activate and makes reference almost entirely to the ActiveWorkbook (if you don't declare the workbook it will assume ActiveWorkbook). This should be avoided and you should be explicitly defining the object you are using instead (why you should do this has already been discussed at great length - you can read more about it here: How To Avoid Using Select in Excel.
3. You have declared a few variables that you don't actually use. Either use them or remove them to declutter the code.
4. You haven't declared the variables storing the values from your sheet (at the start of your for loop) which means they are automatically created as the data type Variant which is not very efficient.
5. The for loop writes the variables to set cell references which means even if we fix all other issues, the code will just overwrite the same cells with each loop iteration. This can be resolved by setting the row value into a variable and increment it with each loop iteration (assuming this is how the template should be completed).

As a side point; using longer but more descriptive variable names can make life a lot easier when debugging errors, especially when you're new to VBA so you may want to consider this with all of your variables. E.g. LoopRow OR RowCounterForLoop instead of r and InvoiceTemplatePath OR SavedInvoicesPath instead of path.

Here is an example code to give you an idea how to implement some changes taking the above points into account, in it we will use the PiNo variable only (but just copy the changes to each of the other relevant variables):

Private Sub CommandButton1_Click()

Dim LastRow As Long     
Dim RowCounterForLoop As Long 
Dim InvoiceTemplateRowCounter as Long
Dim DesktopFilePath As String
Dim SavedInvoiceFilePath As String

Dim PiNo As String  

LastRow = ThisWorkbook.Sheets("CustomerDetails").Range("H" & Rows.Count).End(xlUp).Row
InvoiceTemplateRowCounter = 8

DesktopFilePath = "C:\Users\admin\Desktop\"
SavedInvoiceFilePath = "C:\Users\admin\Desktop\Invoices\"

Workbooks.Open (DesktopFilePath & "InvoiceTemplate.xlsx")

For RowCounterForLoop = 2 To lastrow       'I've removed the previous assignment of 2 to RowCounterForLoop as it is assigned on this line.

    PiNo = ThisWorkbook.Sheets("CustomerDetails").Cells(r, 5).Value        'I've added ThisWorkbook before the Sheet which explicitly defines the code to affect the workbook the code is running on. It also uses a variable instead of number to allow dynamic referencing to the range. 

    Workbooks("InvoiceTemplate.xlsx").Sheets("InvoiceTemplate").Range("AG" & InvoiceTemplateRowCounter).Value = PiNo        'I've added Workbooks("InvoiceTemplate.xlsx") to explicitly run this code on that workbook which avoids using ActiveWorkbook.
    InvoiceTemplateRowCounter = InvoiceTemplateRowCounter + 1
Next RowCounterForLoop 

Workbooks("InvoiceTemplate.xlsx").SaveAs Filename:=SavedInvoiceFilePath & PiNo & ".xlsx" 
Workbooks("InvoiceTemplate.xlsx").Close SaveChanges:=False    'The file is saved on the previous line so this will avoid saving again and pop up prompts etc. 

End Sub

The above won't fix everything and there are better and more efficient ways to achieve what you want but it does provide an answer to your question.

You may want to consider the following to further improve your code:

1. Using Arrays to store data and then write it to the new workbook. (This may be rather tricky depending on your skills using Arrays with VBA)
2. To enter the PiNo into each row (when there are more than 1 product) you can use the Range.FillDown method (depending on how exactly your sheet works) which you can read about Here.