5
\$\begingroup\$

I want to nest a for loop to cycle through the Range.Value but ("B4") throws me off and this can't be avoided. I was considering using an array to do this but can't seem to get the format correct. "Q3" is a spreadsheet and "Bac Form" is the attached table imported from a PDF that must be kept. This code works wonderfully - but it is clunky! I have 3 other unique PDFs with which to complete a similar task and rather than just forcing this thing to work would like to use this opportunity to improve my VBA for-looping skills.

Private Sub CommandButton1_Click()

Dim lr As Long
Dim FolderPath As String

FolderPath = "C:\Users\Joe.Dimaggio\Desktop\PDFs"

lr = Cells(Rows.Count, 1).End(xlUp).Row
lc = Cells(1, Columns.Count).End(xlToLeft).Column

For x = 2 To lr
    Sheets("Bac Form").Range("A2").Value = _
    Sheets("Q3").Cells(1, 1).Value & Sheets("Q3").Cells(x, 1).Value
    Sheets("Bac Form").Range("A3").Value = _
    Sheets("Q3").Cells(1, 2).Value & Sheets("Q3").Cells(x, 2).Value & _
    " (Third Bacterial Quarter)"
    Sheets("Bac Form").Range("A4").Value = _
    Sheets("Q3").Cells(1, 3).Value & Sheets("Q3").Cells(x, 3).Value
    Sheets("Bac Form").Range("B4").Value = _
    Sheets("Q3").Cells(1, 4).Value & Sheets("Q3").Cells(x, 4).Value
    Sheets("Bac Form").Range("A5").Value = _
    Sheets("Q3").Cells(1, 5).Value & Sheets("Q3").Cells(x, 5).Value
    Sheets("Bac Form").Range("A6").Value = _
    Sheets("Q3").Cells(1, 6).Value & Sheets("Q3").Cells(x, 6).Value
    Sheets("Bac Form").Range("A7").Value = _
    Sheets("Q3").Cells(1, 7).Value & Sheets("Q3").Cells(x, 7).Value
    Sheets("Bac Form").Range("A8").Value = _
    Sheets("Q3").Cells(1, 8).Value & Sheets("Q3").Cells(x, 8).Value
    Sheets("Bac Form").Range("A9").Value = _
    Sheets("Q3").Cells(1, 9).Value & Sheets("Q3").Cells(x, 9).Value
    Sheets("Bac Form").Range("A10").Value = _
    Sheets("Q3").Cells(1, 10).Value & Sheets("Q3").Cells(x, 10).Value
    Sheets("Bac Form").Range("A11").Value = _
    Sheets("Q3").Cells(1, 11).Value & Sheets("Q3").Cells(x, 11).Value
    Sheets("Bac Form").Range("A13").Value = _
    Sheets("Q3").Cells(1, 12).Value & Sheets("Q3").Cells(x, 12).Value
    Sheets("Bac Form").Range("A14").Value = _
    Sheets("Q3").Cells(1, 13).Value & Sheets("Q3").Cells(x, 13).Value
    Sheets("Bac Form").Range("A15").Value = _
    Sheets("Q3").Cells(1, 14).Value & Sheets("Q3").Cells(x, 14).Value & _
    " colony/100 ml"
    Sheets("Bac Form").Range("A16").Value = _
    Sheets("Q3").Cells(1, 15).Value & Sheets("Q3").Cells(x, 15).Value
    Sheets("Bac Form").Range("A17").Value = _
    Sheets("Q3").Cells(1, 16).Value & Sheets("Q3").Cells(x, 16).Value & _
    " MPN/100 ml"
    Sheets("Bac Form").Range("A18").Value = _
    Sheets("Q3").Cells(1, 17).Value & Sheets("Q3").Cells(x, 17).Value



        Worksheets("Bac Form").ExportAsFixedFormat Type:=xlTypePDF, Filename:=FolderPath & "\" & _
        Worksheets("Bac Form").Name & "(Q3)" & (x - 1), openafterpublish:=False

Next x



End Sub

Here is the PDF form after importing it into Excel:

Bac Form

\$\endgroup\$
3
  • \$\begingroup\$ Are you intentionally skipping over Sheets("Bac Form").Range("A12").Value \$\endgroup\$ Jan 14, 2016 at 10:14
  • \$\begingroup\$ It would be really useful if you could include a screenshot of what your sheets actually look like (data removed if need be). \$\endgroup\$
    – Kaz
    Jan 14, 2016 at 12:31
  • \$\begingroup\$ To answer the question @ThunderFrame I did intend to skip A12 \$\endgroup\$
    – MisterGT
    Jan 15, 2016 at 22:13

1 Answer 1

8
\$\begingroup\$

I'm having difficulty in visualising your data here, a screenshot would be really useful.

In the meantime:


Option Explicit

This should be at the top of every code module you ever create in VBA. Go to Tools --> Options --> Require Variable Declaration and it will automatically insert it for you from now on.

This is important because without it, VBA will interpret any new variable names (including mis-spellings) as entirely new variables, instead of what you intended.

It also forces you to declare your variables. So you must explicitly give them a type (Long, String, Variant etc.) and a scope (Procedure Dim var As, Module Private Var As, Project Public Var As).

This will then automatically catch all sorts of unintended situations (such as accidentally setting a number equal to an object) which would not be caught if VBA has to assume that all your variables are Variant because you never explicitly declared them.


Magic Variables

And not the good kind of magic either. A Magic Variable is any value which is hardcoded. Especially if it's hardcoded in multiple places.

lr = Cells(Rows.Count, 1).End(xlUp).Row

Why 1? How was that number determined?

"Bac Form", "A1", "Q3" etc.

Why those values in particular? If you can't look at a variable and know what it represents, then it has appeared in your code as if by magic.


If you have to hardcode values, then they should be hardcoded precisely once, into a descriptively named variable.

This:

FolderPath = "C:\Users\Joe.Dimaggio\Desktop\PDFs"

is the right way to hardcode a value. It's written once, and I can see FolderPath later on in the code and know exactly what it is.

This:

For x = 2 To lr
    Sheets("Bac Form").Range("A2").Value = _
    Sheets("Q3").Cells(1, 1).Value & Sheets("Q3").Cells(x, 1).Value
    Sheets("Bac Form").Range("A3").Value = _
    Sheets("Q3").Cells(1, 2).Value & Sheets("Q3").Cells(x, 2).Value & _
    " (Third Bacterial Quarter)"
    Sheets("Bac Form").Range("A4").Value = _
    Sheets("Q3").Cells(1, 3).Value & Sheets("Q3").Cells(x, 3).Value
    Sheets("Bac Form").Range("B4").Value = _
    Sheets("Q3").Cells(1, 4).Value & Sheets("Q3").Cells(x, 4).Value
    Sheets("Bac Form").Range("A5").Value = _
    Sheets("Q3").Cells(1, 5).Value & Sheets("Q3").Cells(x, 5).Value
    Sheets("Bac Form").Range("A6").Value = _
    Sheets("Q3").Cells(1, 6).Value & Sheets("Q3").Cells(x, 6).Value
    Sheets("Bac Form").Range("A7").Value = _
    Sheets("Q3").Cells(1, 7).Value & Sheets("Q3").Cells(x, 7).Value
    Sheets("Bac Form").Range("A8").Value = _
    Sheets("Q3").Cells(1, 8).Value & Sheets("Q3").Cells(x, 8).Value
    Sheets("Bac Form").Range("A9").Value = _
    Sheets("Q3").Cells(1, 9).Value & Sheets("Q3").Cells(x, 9).Value
    Sheets("Bac Form").Range("A10").Value = _
    Sheets("Q3").Cells(1, 10).Value & Sheets("Q3").Cells(x, 10).Value

is not. What happens if the name of your sheet changes? Or your data table moves because someone inserted/deleted a column/row? You'll have to go and re-code every single value.


Naming

Variable names should be Concise, Descriptive and, above all, Unambiguous.

FolderPath

is a good name because it's clear exactly what the variable contains/represents.

lr

is not. If I encounter that line halfway through your code, I'm going to go all the way back through to remind myself what it's meant to be representing. Just call it lastRow and you avoid all that trouble.

VBA Naming Conventions:

Typical VBA Naming conventions go as follows:

camelCase for local variables.

PascalCase for Module/Global Variables.

SHOUTY_SNAKE_CASE for constants.

Get in the habit of following them.


Bringing it all together

Step 1, put your worksheets in variables:

For x = 2 To lr
    wsBacForm.Range("A2").Value = wsQ3.Cells(1, 1).Value & wsQ3.Cells(x, 1).Value
    wsBacForm.Range("A3").Value = wsQ3.Cells(1, 2).Value & wsQ3.Cells(x, 2).Value & " (Third Bacterial Quarter)"
    wsBacForm.Range("A4").Value = wsQ3.Cells(1, 3).Value & wsQ3.Cells(x, 3).Value
    wsBacForm.Range("B4").Value = wsQ3.Cells(1, 4).Value & wsQ3.Cells(x, 4).Value
    wsBacForm.Range("A5").Value = wsQ3.Cells(1, 5).Value & wsQ3.Cells(x, 5).Value
    wsBacForm.Range("A6").Value = wsQ3.Cells(1, 6).Value & wsQ3.Cells(x, 6).Value
    wsBacForm.Range("A7").Value = wsQ3.Cells(1, 7).Value & wsQ3.Cells(x, 7).Value
    wsBacForm.Range("A8").Value = wsQ3.Cells(1, 8).Value & wsQ3.Cells(x, 8).Value
    wsBacForm.Range("A9").Value = wsQ3.Cells(1, 9).Value & wsQ3.Cells(x, 9).Value
    wsBacForm.Range("A10").Value = wsQ3.Cells(1, 10).Value & wsQ3.Cells(x, 10).Value

And suddenly the structure becomes clear.


Then:

Add options to change the row/column position of your tables.
Loop through rows/columns.
Add separate variables for each sheet to deal with (potentially) separate positions.
Add an index counter to track where to insert extra text.
Put the filename creation on a separate line.
Add proper Range objects.

And now, if you need to change something, or your data tables get bigger, or you need to modify a specific aspect of the execution, you can just change one value and the rest is done for you.

Private Sub CommandButton1_Click()

    Dim fileName As String

    Const FOLDER_PATH As String = "C:\Users\Joe.Dimaggio\Desktop\PDFs"

    Dim wsBacForm As Worksheet, wsQ3 As Worksheet
    Set wsBacForm = Sheets("Bac Form")
    Set wsQ3 = Sheets("Q3")

    Dim q3BaseRow As Long, q3BaseCol As Long '/ Location of the Q3 data table
    q3BaseRow = 1
    q3BaseCol = 1

    Dim bacBaseRow As Long, bacBaseCol As Long '/ Location of the bac output table
    bacBaseRow = 1
    bacBaseCol = 1

    Dim lastRow As Long, lastCol As Long
    lastRow = wsQ3.Cells(Rows.Count, q3BaseCol).End(xlUp).row
    lastCol = wsQ3.Cells(q3BaseRow, Columns.Count).End(xlToLeft).Column

    Dim bacRow As Long, bacCol As Long, q3Row As Long, q3Col As Long
    Dim row As Long, col As Long, counter As Long

    Dim bacOutputCell As Range, q3HeaderCell As Range, q3DataCell As Range
    Dim outputString As String

    For row = q3BaseRow To lastRow
        bacRow = row - q3BaseRow + bacBaseRow
        q3Row = row

        counter = 0
        For col = q3BaseCol To lastCol
            counter = counter + 1
            q3Col = col

            Set bacOutputCell = wsBacForm.Cells(bacRow, bacBaseCol)
            Set q3HeaderCell = wsQ3.Cells(q3BaseRow, q3Col)
            Set q3DataCell = wsQ3.Cells(q3Row, q3Col)

            outputString = q3HeaderCell.Value & q3DataCell.Value

            Select Case counter '/ used a counter so that it is (absolute position of column) agnostic

                Case Is = 2
                outputString = outputString & " (Third Bacterial Quarter)"

                Case Is = 14
                outputString = outputString & " colony/100 ml"

                Case Is = 16
                outputString = outputString & " MPN/100 ml"

            End Select

            bacOutputCell.Value = outputString
        Next col

        fileName = FOLDER_PATH & "\" & wsBacForm.Name & "(" & wsQ3.Name & ")" & (row - 1)
        wsBacForm.ExportAsFixedFormat Type:=xlTypePDF, fileName:=fileName, openafterpublish:=False
    Next row

End Sub
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thank you so much for taking the time to show me all of this! Now to dissect it and learn all about it! Thanks again! \$\endgroup\$
    – MisterGT
    Jan 15, 2016 at 15:13
  • \$\begingroup\$ Nitpick: Option Explicit does not force anyone to declare an explicit type. Just to declare every variable that's used. \$\endgroup\$ Jan 15, 2016 at 22:42
  • \$\begingroup\$ @Mat'sMug True. I'll remember that for future reference. \$\endgroup\$
    – Kaz
    Jan 15, 2016 at 22:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.