Converting Data in Rows and Columns to Rows in VBA for Excel

Question

I have a working VBA script for Excel that converts a matrix of data with multiple records in per row to multiple rows with one record per row.
A StackOverflow user told me that the code could use significant improvement, specifically mentioning implicit variants (not quite sure where I went wrong there), difficult to read code, splitting responsibilities and something about GoTo, the 1980's and raptors...

The script takes data like this:

Materials       Person1     Person2
---------       ---------   ---------
563718          20          40
837563          15          35

And can convert it to this:

Person          Materials   Data
---------       ---------   ---------
Person1         563718      20
Person1         837563      15
Person2         563718      40
Person2         837563      35

Data is supplied by a third party. Each record/transaction (ex. quantity purchased, for each materials type, by customer) needs to be formatted in a separate row.

The script asked the the user about this data. In this example the user specifies 1 "Header Column" (one column beginning on the left that will remain as is). Then gives a name of "Person" for a new field made from values in the headers of the remaining columns to the right. The values (amounts) below these headers are also made into a new field, called "Data" by default.

I am open to any advice, but I am most interested in (1) writing better code in general, and (2) making this script adaptable and easier for others to use.

The script below was originally written by Peter T Oboyski. I extensively modified it.

Option Explicit

Sub MatrixConverter2_3()

'--------------------------------------------------
' This section declares variables for use in the script

Dim book, head, cels, mtrx, dbase, v, UserReady, columnsToCombine, RowName, DefaultRowName, DefaultColName1, DefaultColName2, ColName As String
Dim defaultHeaderRows, defaultHeaderColumns, c, r, selectionCols, ro, col, newro, newcol, rotot, coltot, all, rowz, colz, tot As Long
Dim headers(100) As Variant
Dim dun As Boolean


'--------------------------------------------------
' This section sets the script defaults

defaultHeaderRows = 1
defaultHeaderColumns = 2

DefaultRowName = "MyColumnName"

'--------------------------------------------------
' This section asks about data types, row headers, and column headers

UserReady = MsgBox("Have you selected the entire data set (not the column headers) to be converted?", vbYesNoCancel)
If UserReady = vbNo Or UserReady = vbCancel Then GoTo EndMatrixMacro

all = MsgBox("Exclude zeros and empty cells?", vbYesNoCancel)
If all = vbCancel Then GoTo EndMatrixMacro


' UN-COMMENT THIS SECTION TO ALLOW FOR MULTIPLE HEADER ROWS
rowz = 1
' rowz = InputBox("How many HEADER ROWS?" & vbNewLine & vbNewLine & "(Usually 1)", "Header Rows & Columns", defaultHeaderRows)
' If rowz = vbNullString Then GoTo EndMatrixMacro

colz = InputBox("How many HEADER COLUMNS?" & vbNewLine & vbNewLine & "(These are the columns on the left side of your data set to preserve as is.)", "Header Rows & Columns", defaultHeaderColumns)
If colz = vbNullString Then GoTo EndMatrixMacro


'--------------------------------------------------
' This section allows the user to provide field (column) names for the new spreadsheet

selectionCols = Selection.Columns.Count ' get the number of columns in the selection
For r = 1 To selectionCols
    headers(r) = Selection.Cells(1, r).Offset(rowOffset:=-1, columnOffset:=0).Value ' save the column headers to use as defaults for user provided names
Next r

colz = colz * 1
columnsToCombine = "'" & Selection.Cells(1, colz + 1).Offset(rowOffset:=-1, columnOffset:=0).Value & "' to '" & Selection.Cells(1, selectionCols).Offset(rowOffset:=-1, columnOffset:=0).Value & "'"

Dim Arr(20) As Variant
newcol = 1
For r = 1 To rowz
    If r = 1 Then RowName = DefaultRowName
    Arr(newcol) = InputBox("Field name for the fields/columns to be combined" & vbNewLine & vbNewLine & columnsToCombine, , RowName)
    If Arr(newcol) = vbNullString Then GoTo EndMatrixMacro
    newcol = newcol + 1
Next
For c = 1 To colz
    ColName = headers(c)
    Arr(newcol) = InputBox("Field name for column " & c, , ColName)
    If Arr(newcol) = vbNullString Then GoTo EndMatrixMacro
    newcol = newcol + 1
Next
Arr(newcol) = "Data"
v = newcol

'--------------------------------------------------
' This section creates the new spreadsheet, names it, and color codes the new worksheet tab

mtrx = ActiveSheet.Name
Sheets.Add After:=ActiveSheet
dbase = "DB of " & mtrx

'--------------------------------------------------
' If the proposed worksheet name is longer than 28 characters, truncate it to 29 characters.
    If Len(dbase) > 28 Then dbase = Left(dbase, 28)


'--------------------------------------------------
' This section checks if the proposed worksheet name
'  already exists and appends adds a sequential number
'  to the name
    Dim sheetExists As Variant
    Dim Sheet As Worksheet
    Dim iName As Integer

    Dim dbaseOld As String
    dbaseOld = dbase    ' save the original proposed name of the new worksheet

    iName = 0

    sheetExists = False
CheckWorksheetNames:

    For Each Sheet In Worksheets    ' loop through every worksheet in the workbook
        If dbase = Sheet.Name Then
            sheetExists = True
            iName = iName + 1
            dbase = Left(dbase, Len(dbase) - 1) & " " & iName
            GoTo CheckWorksheetNames
            ' Exit For
        End If
    Next Sheet


'--------------------------------------------------
' This section notify the user if the proposed
' worksheet name is already being used and the new
' worksheet was given an alternate name

    If sheetExists = True Then
        MsgBox "The worksheet '" & dbaseOld & "' already exists.  Renaming to '" & dbase & "'."
    End If


'--------------------------------------------------
' This section creates and names a new worksheet
    On Error Resume Next    'Ignore errors
        If Sheets("" & Range(dbase) & "") Is Nothing Then   ' If the worksheet name doesn't exist
            ActiveSheet.Name = dbase    ' Rename newly created worksheet
        Else
            MsgBox "Cannot name the worksheet '" & dbase & "'.  A worksheet with that name already exists."
            GoTo EndMatrixMacro
        End If
    On Error GoTo 0         ' Resume normal error handling

    Sheets(dbase).Tab.ColorIndex = 41 ' color the worksheet tab


'--------------------------------------------------
' This section turns off screen and calculation updates so that the script
' can run faster.  Updates are turned back on at the end of the script.
    Application.Calculation = xlCalculationManual
    Application.ScreenUpdating = False


'--------------------------------------------------
'This section determines how many rows and columns the matrix has

dun = False
rotot = rowz + 1
Do
    If (Sheets(mtrx).Cells(rotot, 1) > 0) Then
        rotot = rotot + 1
    Else
        dun = True
    End If
Loop Until dun
rotot = rotot - 1

dun = False
coltot = colz + 1
Do
    If (Sheets(mtrx).Cells(1, coltot) > 0) Then
        coltot = coltot + 1
    Else
        dun = True
    End If
Loop Until dun
coltot = coltot - 1


'--------------------------------------------------
'This section writes the new field names to the new spreadsheet

For newcol = 1 To v
    Sheets(dbase).Cells(1, newcol) = Arr(newcol)
Next


'--------------------------------------------------
'This section actually does the conversion

tot = 0
newro = 2
For col = (colz + 1) To coltot
    For ro = (rowz + 1) To rotot 'the next line determines if data are nonzero
        If ((Sheets(mtrx).Cells(ro, col) <> 0) Or (all <> 6)) Then   'DCB modified ">0" to be "<>0" to exclude blank and zero cells
            tot = tot + 1
            newcol = 1
            For r = 1 To rowz            'the next line copies the row headers
                Sheets(dbase).Cells(newro, newcol) = Sheets(mtrx).Cells(r, col)
                newcol = newcol + 1
            Next
            For c = 1 To colz         'the next line copies the column headers
                Sheets(dbase).Cells(newro, newcol) = Sheets(mtrx).Cells(ro, c)
                newcol = newcol + 1
            Next                                'the next line copies the data
            Sheets(dbase).Cells(newro, newcol) = Sheets(mtrx).Cells(ro, col)
            newro = newro + 1
        End If
    Next
Next


'--------------------------------------------------
'This section displays a message box with information about the conversion

book = "Original matrix = " & ActiveWorkbook.Name & ": " & mtrx & Chr(10)
head = "Matrix with " & rowz & " row headers and " & colz & " column headers" & Chr(10)
cels = tot & " cells of " & ((rotot - rowz) * (coltot - colz)) & " with data"


'--------------------------------------------------
' This section turns screen and calculation updates back ON.
    Application.Calculation = xlCalculationAutomatic
    Application.ScreenUpdating = True


MsgBox (book & head & cels)


'--------------------------------------------------
' This is an end point for the macro

EndMatrixMacro:

End Sub

Answer 1

Declaring variables at the top of a procedure was a recommended practice in 90's VB code, because "it makes it easy to see everything that the procedure needs at once". When a procedure would fit on a single screen, it wasn't too bad - but when a procedure scrolls several screens down, and uses 30-40 local variables, that "wall of declarations" actually made it harder to read (/maintain) the code, because you'd constantly need to scroll back up to see the declaration of a given variable, and then you'd waste considerable time locating which line you were looking at on your way back down. Been there, done that.

So to avoid the "wall of declarations" you could make a single instruction to declare a list of variables, like this:

Dim book, head, cels, mtrx, dbase, v, UserReady, columnsToCombine, RowName, DefaultRowName, DefaultColName1, DefaultColName2, ColName As String
Dim defaultHeaderRows, defaultHeaderColumns, c, r, selectionCols, ro, col, newro, newcol, rotot, coltot, all, rowz, colz, tot As Long

There's a trap though: Dim foo, bar, baz As String declares the last one (baz) as a String, and leaves foo and bar implicitly Variant - which incurs useless overhead and requires more storage/memory than needed (not that that is a problem nowadays though).

'--------------------------------------------------
' This section declares variables for use in the script

Dim book
Dim head
Dim cels
Dim mtrx
Dim dbase
Dim v
Dim UserReady
Dim columnsToCombine
Dim RowName
Dim DefaultRowName
Dim DefaultColName1
Dim DefaultColName2
Dim ColName As String

Dim defaultHeaderRows
Dim defaultHeaderColumns
Dim c
Dim r
Dim selectionCols
Dim ro
Dim col
Dim newro
Dim newcol
Dim rotot
Dim coltot
Dim all
Dim rowz
Dim colz
Dim tot As Long

Dim headers(100) As Variant
Dim dun As Boolean

Fun fact: the first three variables are actually Strings that are concatenated into a message for a MsgBox that's displayed at the very end of the procedure.

So we have that wall of declarations, and that banner comment telling us that we're looking at a wall of declarations. Comments shouldn't state the obvious like that; good comments tell us what the code can't say all by itself: it tells us why the code does something.

But back at these variables: DefaultColName1, DefaultColName2 and defaultHeaderRows are never never used! Actually DefaultColName1 and DefaultColName2 aren't even assigned, and never referred to, not even in dead/commented-out code - but who could have known? That's why declaring variables closer to where they're used is a much better practice: no wall of declarations, and it's much harder to declare a variable that's left unused, without noticing.

---

'--------------------------------------------------
' This section asks about data types, row headers, and column headers

In other words, this section is collecting user input - it should be a separate procedure!

UserReady = MsgBox("Have you selected the entire data set (not the column headers) to be converted?", vbYesNoCancel)
If UserReady = vbNo Or UserReady = vbCancel Then GoTo EndMatrixMacro

That UserReady variable should have been declared like this:

Dim UserReady As VbMsgBoxResult

Actually, since the only thing we're using it for is effectively to cancel the whole thing, might as well not declare it at all and do this instead:

If MsgBox("Have you selected the entire data set (not the column headers) to be converted?", vbYesNoCancel) <> vbYes Then Exit Sub

...And we just eliminated a GoTo jump!

all = MsgBox("Exclude zeros and empty cells?", vbYesNoCancel)
If all = vbCancel Then GoTo EndMatrixMacro

Same thing here: all should have been declared As VbMsgBoxResult, and there's no need to GoTo EndMatrixMacro either. The name isn't ideal, too: vbYes stands for "exclude zeros and empty cells", and vbNo stands for "include zeros and empty cells" - which means the true meaning of all is the exact opposite of what it appears to be! I'd rename it to IsExcludingZeroAndEmpty, and declare it As Boolean, because we don't really care about the MsgBox result here, all that matters is whether or not we're to include zeros and empty values.

That all variable is used here:

If ((Sheets(mtrx).Cells(ro, col) <> 0) Or (all <> 6)) Then

What's that magic number 6? If the variable would have been declared As VbMsgBoxResult, the VBE's IntelliSense would have suggested to use vbYes instead of its underlying numeric value. But that's all moot with a proper Boolean:

If Sheets(mtrx).Cells(ro, col) <> 0 Or Not IsExcludingZeroAndEmpty Then

(note, I might have gotten confused with the reversed "all" logic here... but you get the point I'm sure - which one is easier to understand?)

Next the script prompts for how many header columns we're looking at:

colz = InputBox("How many HEADER COLUMNS?" & vbNewLine & vbNewLine & "(These are the columns on the left side of your data set to preserve as is.)", "Header Rows & Columns", defaultHeaderColumns)
If colz = vbNullString Then GoTo EndMatrixMacro

Again Exit Sub makes the GoTo jump unnecessary. But there's a problem with using vbNullString with an InputBox - not in this context because it doesn't matter, but that condition will be true regardless of whether the user entered an empty string or hit the Cancel button; in a case where you would need to differenciate these inputs, you'd be stuck here.

If StrPtr(colz) = 0 Then Exit Sub

If StrPtr(InputBoxResult) returns a non-zero value, then there was a user input. If it's zero, then the user cancelled out.

There's a worse problem though. colz is a Variant, so it will happily be assigned to potato and nothing will happen until execution reaches this line:

colz = colz * 1

And then boom, runtime error 13 Type Mismatch strikes. The funny part is that it seems this no-op multiplication is there to prevent the next line from blowing up ...with the exact same runtime error:

columnsToCombine = "'" & Selection.Cells(1, colz + 1).Offset(rowOffset:=-1, columnOffset:=0).Value & "' to '" & Selection.Cells(1, selectionCols).Offset(rowOffset:=-1, columnOffset:=0).Value & "'"

A (much) better way would have been to validate the user's input:

If Not IsNumeric(colz) Then 'user is playing smartypants

---

This is an interesting comment:

'--------------------------------------------------
' If the proposed worksheet name is longer than 28 characters, truncate it to 29 characters.
    If Len(dbase) > 28 Then dbase = Left(dbase, 28)

Which one is wrong? Is the typo in the comment or in the code? We'll never know... but this is why comments shouldn't rephrase what the code is already saying: when the code changes, the comments don't always get updated, and are left there dangling half-truths that no one dares fixing. This would have been much better:

    ' Maximum length allowed for a sheet name is 31 characters
    If Len(dbase) > 28 Then dbase = Left(dbase, 28)

...which begs the question, why aren't we seeing this?

Private Const SHEETNAME_MAXLENGTH As Integer = 28 ' actually it's 31, but we're keeping a little buffer to append a digit if needed

And then do we need a comment to explain this line?

If Len(dbase) > SHEETNAME_MAXLENGTH Then dbase = Left(dbase, SHEETNAME_MAXLENGTH)

---

Everytime there's one such "banner comment":

'--------------------------------------------------
' This section checks if the proposed worksheet name
'  already exists and appends adds a sequential number
'  to the name

This is how I read it:

'--------------------------------------------------
' This section belongs in its own procedure or function

Might be a bit wrong - I haven't gone into the nitty-gritty details of how the procedure actually does its thing. But usually when a comment says "this chunk of code does XYZ", it can very well be moved into a procedure with a name that says "this procedure does XYZ".

I'll let other reviewers tackle the actual meat of the subject =)

Answer 2

Readability

1 - Variable naming. I did a double-take when I first looked at the code, because it was tagged vba instead of lolcode. While variable names like colz and dun are cute, columnCount and finished make it a lot easier to tell at a glance what the values you're storing in them actually represent (especially if I have to strain my scroll-wheel finger to find the declarations).

2 - Side scrolling. VBA has a line continuation operator, _. Even with my monitor set to its usual eyestrain-o-vision™ resolution setting and the VBE window maximized, lines like this still require scrolling to the side:

columnsToCombine = "'" & Selection.Cells(1, colz + 1).Offset(rowOffset:=-1, columnOffset:=0).Value & "' to '" & Selection.Cells(1, selectionCols).Offset(rowOffset:=-1, columnOffset:=0).Value & "'"

Breaking them up into multiple lines makes it easier to see the whole thing at the same time.

3 - Superfluous syntax. This is probably personal preference more than anything, but using parentheses when they aren't required just adds noise:

If (Sheets(mtrx).Cells(1, coltot) > 0) Then

They also add an albeit trivial amount of work for the compiler when it evaluates the line of code to determine that it can simply ignore them. This is what the compiler sees:

If Sheets(mtrx).Cells(1, coltot) > 0 Then

Do the compiler a solid and just leave them out. Granted it isn't this...

If ((Sheets(mtrx).Cells(((((1)))), ((coltot)))) > (((0)))) Then

...but I'd reserve parentheses for places where you need to either override or emphasize the operator precedence.

---

Object references

Every time you use Sheets, you are implicitly referencing ActiveWorkbook, and every time you use Range, you are implicitly referencing ActiveSheet. On top of that, you are repeatedly retrieving the same object from the Sheets collection when all you need to do is grab a reference. You get the name of the ActiveSheet here...

mtrx = ActiveSheet.Name

... and then every time you need to reference that sheet, you retrieve it from the Sheets collection, i.e.:

If (Sheets(mtrx).Cells(rotot, 1) > 0) Then

From a performance standpoint, this isn't free - not only is it a function call, Excel has to find the reference and return it. It would make much more sense to just get the reference...

Dim matrix as Worksheet
Set matrix = ActiveSheet

...and use it:

If matrix.Cells(rotot, 1) > 0 Then

Same thing with Sheets(dbase) - you are actually discarding the reference that Excel returns for you here:

Sheets.Add After:=ActiveSheet

Again, just grab the reference for use later:

Dim dbase As Worksheet
Set dbase = matrix.Parent.Sheets.Add(After:=matrix)

Finally, take advantage of With blocks, especially in loops. This section of code...

Do
    If (Sheets(mtrx).Cells(rotot, 1) > 0) Then
        rotot = rotot + 1
    Else
        dun = True
    End If
Loop Until dun

...is repeating the call Sheets(mtrx) every time through the loop. Behind the scenes, Excel dereferences the implicit . in Sheets to ActiveWorkbook, then calls its Sheets property, and finally requests and returns the mtrx item. And... it is always going to return the exact same thing. That is a ton of meaningless processing to do potentially thousands of times. If you wrap the loop in a With block, you are only requesting the reference once (although see below under Refactoring opportunities about this specific code section):

With Sheets(mtrx)
    Do
        If .Cells(rotot, 1) > 0 Then
            rotot = rotot + 1
        Else
            dun = True
        End If
    Loop Until dun
End With

---

Variables

You have way to many. I'd start by getting rid of the ones that @Mat'sMug already identified as not being used, and then move on to variables like v. You set v on line 68...

v = newcol

...and never assign to it again until it pops up 105 lines later as a loop limit:

For newcol = 1 To v

Neither value changes in between, and newcol mysteriously switches from being the limit to being the counter. If you use a generic loop counter more than once, just reuse one (although if you follow some of the refactoring suggestions this becomes much less necessary).

I already touched on using meaningful names earlier in this ramble, but this seems like an appropriate place to bring up naming conventions. While this is arguably an issue of taste, the current suggested .NET naming conventions are a good place to start. The specific convention doesn't matter nearly as much as consistency, because it provides hints as to what exactly an identifier represents. Currently, your variables use a seemingly random mixture of Pascal case (RowName), camel case (columnsToCombine), all lower case (newcol), Hungarian notation (iName), Code Golf notation (r), and Pirate notation (Arr).

---

Refactoring opportunities

1 - This section of code is incredibly inefficient (and should be properly structured instead of using a Goto - why the much preferable Exit For is commented out is completely beyond me...):

    Dim sheetExists As Variant
    Dim Sheet As Worksheet
    Dim iName As Integer

    Dim dbaseOld As String
    dbaseOld = dbase    ' save the original proposed name of the new worksheet

    iName = 0

    sheetExists = False
CheckWorksheetNames:

    For Each Sheet In Worksheets    ' loop through every worksheet in the workbook
        If dbase = Sheet.Name Then
            sheetExists = True
            iName = iName + 1
            dbase = Left(dbase, Len(dbase) - 1) & " " & iName
            GoTo CheckWorksheetNames
            ' Exit For
        End If
    Next Sheet

First, there is no reason for sheetExists to be a Variant - it is only used as a Boolean, but the real reason is that you are repeatedly iterating through the exact same collection of Worksheets. Remember, this isn't free from a performance standpoint. I'd collect the names once, and then find a unique name. A Scripting.Dictionary would be my weapon of choice for it's fast hash lookups, and I'd extract it to it's own function:

Private Function GetUniqueSheetName(book As Workbook, ByVal proposed As String) As String
    Dim existing As New Scripting.Dictionary
    Dim sheet As Worksheet

    For Each sheet In book.Worksheets
        existing.Add sheet.Name, vbNull
    Next

    Dim unique As String
    unique = proposed
    Dim suffix As Long
    Do
        If Not existing.Exists(unique) Then
            GetUniqueSheetName = unique
            Exit Function
        End If
        suffix = suffix + 1
        unique = proposed & " " & suffix
    Loop
End Function

2 - The code under the banner...

'--------------------------------------------------
'This section determines how many rows and columns the matrix has

...can be pretty much eliminated - there are much easier methods for determining the last row and column of a Worksheet. You can simply do this:

With Sheets(mtrx)
    'Number of rows in column A
    rotot = .Range("A" & .Rows.Count).End(xlUp).Row
    'Number of columns in row 1
    coltot = .Range("XFD" & 1).End(xlToLeft).Column
End With

3 - I've already spent way to much time on this to do another example refactor (hopefully another user will tackle it), but the workhorse code under the banner...

'--------------------------------------------------
'This section actually does the conversion

... is also really inefficient. Instead of writing individual values, I'd lean more toward generating a Collection of rows to write, and then writing the whole thing at once. Reading from and writing to cells are incredibly expensive operations when there are alternatives. You can get an immediate performace gain by reading the entire working range from Sheets(mtrx) into an array...

Dim matixValues As Variant
With Sheets(mtrx)
    matixValues = .Range(.Cells(rowz + 1, colz + 1), .Cells(rowtot, coltot)).Value
End With

...and then working with the array instead:

For col = LBound(matixValues, 2) To UBound(matixValues, 2)
    For ro = LBound(matixValues, 1) To UBound(matixValues, 1)
        If matixValues(ro, col) <> 0 Or all = vbNo Then
        '...
        End If
    Next
Next

---

Miscellaneous

1 - Use built in constants. You do this in some places, like...

InputBox("How many HEADER ROWS?" & vbNewLine ...

...and...

If colz = vbNullString Then GoTo EndMatrixMacro

...but then use "" and Chr(10) elsewhere. In the first case, vbNullString is a better choice because of how the compiler treats them (building a string literal instead of using a null pointer). In the second case there are 2 issues. First, you are making a function call that will always not only return the same value (vbLf), but return it as a Variant instead of a String (see the section on String-returning built-in functions here). This is essentially the same as CStr(Chr(10)), because you are using it with a string concat operator (&) - and in fact, your usages end up getting cast back to Variants because of the implicit Variant declarations.

2 - Make sure you eliminate the possibility of unintentionally changing the Application state to the extent possible. You call Application.Calculation = xlCalculationManual, but don't have any error handling for the code section that lies between it and the line where you turn it back on. Application.ScreenUpdating will (usually) correct itself when execution stops. Application.Calculation will not - it will persist with the whatever you set it to.

Best practice would actually be not assuming that the user has Application.Calculation set to xlCalculationAutomatic. Instead, cache the current setting and revert to it, i.e.:

    On Error GoTo CleanExit
    Dim calcState As XlCalculation

    calcState = Application.Calculation
    Application.Calculation = xlCalculationManual
    '...
CleanExit:
    Application.Calculation = calcState
End Sub

3 - Only use named parameter notation when you aren't supplying all the parameters. Calls like this...

.Offset(rowOffset:=-1, columnOffset:=0)

...don't need it, and make it harder to pick out the actual values being passed. Even if I didn't already know that they are row and column, I can see it with IntelliSense...

...and if I if that didn't help, I should probably be looking it up in either the Object Browser or the documentation. I'd reserve it for cases where you aren't supplying all of the parameters or skipping an optional one.

4 - You don't need to set variables to their default values immediately after declaring them:

Dim sheetExists As Variant
Dim Sheet As Worksheet
Dim iName As Integer

`... nothing to do with iName or sheetExists here.
iName = 0
sheetExists = False

Note that even though sheetExists is (mis)declared as a Variant, it will still default to False when treated as a Boolean:

Dim foo As Variant
Debug.Print foo = False  'Prints True