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          40837563          15          35And can convert it to this:Person          Materials   Data---------       ---------   ---------Person1         563718      20Person1         837563      15Person2         563718      40Person2         837563      35Data 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 ExplicitSub MatrixConverter2_3()'--------------------------------------------------' This section declares variables for use in the scriptDim book, head, cels, mtrx, dbase, v, UserReady, columnsToCombine, RowName, DefaultRowName, DefaultColName1, DefaultColName2, ColName As StringDim defaultHeaderRows, defaultHeaderColumns, c, r, selectionCols, ro, col, newro, newcol, rotot, coltot, all, rowz, colz, tot As LongDim headers(100) As VariantDim dun As Boolean'--------------------------------------------------' This section sets the script defaultsdefaultHeaderRows = 1defaultHeaderColumns = 2DefaultRowName = MyColumnName'--------------------------------------------------' This section asks about data types, row headers, and column headersUserReady = MsgBox(Have you selected the entire data set (not the column headers) to be converted?, vbYesNoCancel)If UserReady = vbNo Or UserReady = vbCancel Then GoTo EndMatrixMacroall = MsgBox(Exclude zeros and empty cells?, vbYesNoCancel)If all = vbCancel Then GoTo EndMatrixMacro' UN-COMMENT THIS SECTION TO ALLOW FOR MULTIPLE HEADER ROWSrowz = 1' rowz = InputBox(How many HEADER ROWS? & vbNewLine & vbNewLine & (Usually 1), Header Rows & Columns, defaultHeaderRows)' If rowz = vbNullString Then GoTo EndMatrixMacrocolz = 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 spreadsheetselectionCols = Selection.Columns.Count ' get the number of columns in the selectionFor 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 namesNext rcolz = colz * 1columnsToCombine = ' & 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 Variantnewcol = 1For 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 + 1NextFor 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 + 1NextArr(newcol) = Datav = newcol'--------------------------------------------------' This section creates the new spreadsheet, names it, and color codes the new worksheet tabmtrx = ActiveSheet.NameSheets.Add After:=ActiveSheetdbase = 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 = FalseCheckWorksheetNames:    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 hasdun = Falserotot = rowz + 1Do    If (Sheets(mtrx).Cells(rotot, 1) > 0) Then        rotot = rotot + 1    Else        dun = True    End IfLoop Until dunrotot = rotot - 1dun = Falsecoltot = colz + 1Do    If (Sheets(mtrx).Cells(1, coltot) > 0) Then        coltot = coltot + 1    Else        dun = True    End IfLoop Until duncoltot = coltot - 1'--------------------------------------------------'This section writes the new field names to the new spreadsheetFor newcol = 1 To v    Sheets(dbase).Cells(1, newcol) = Arr(newcol)Next'--------------------------------------------------'This section actually does the conversiontot = 0newro = 2For 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    NextNext'--------------------------------------------------'This section displays a message box with information about the conversionbook = 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 = TrueMsgBox (book & head & cels)'--------------------------------------------------' This is an end point for the macroEndMatrixMacro:End Sub
Converting Data in Rows and Columns to Rows in VBA for Excel
vba;excel;matrix
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 StringDim defaultHeaderRows, defaultHeaderColumns, c, r, selectionCols, ro, col, newro, newcol, rotot, coltot, all, rowz, colz, tot As LongThere'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 scriptDim bookDim headDim celsDim mtrxDim dbaseDim vDim UserReadyDim columnsToCombineDim RowNameDim DefaultRowNameDim DefaultColName1Dim DefaultColName2Dim ColName As StringDim defaultHeaderRowsDim defaultHeaderColumnsDim cDim rDim selectionColsDim roDim colDim newroDim newcolDim rototDim coltotDim allDim rowzDim colzDim tot As LongDim headers(100) As VariantDim dun As BooleanFun 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 headersIn 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 EndMatrixMacroThat UserReady variable should have been declared like this:Dim UserReady As VbMsgBoxResultActually, 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 EndMatrixMacroSame 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)) ThenWhat'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 EndMatrixMacroAgain 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 SubIf 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 * 1And 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 smartypantsThis 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 neededAnd 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 nameThis is how I read it:'--------------------------------------------------' This section belongs in its own procedure or functionMight 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 =)