byronwall / bUTL

Excel add-in with helpers for charting, formatting, and general pain points
http://byroni.us/bUTL
MIT License
16 stars 3 forks source link

Join columns only works for a single row of data #10

Closed byronwall closed 9 years ago

byronwall commented 9 years ago

It would be good if this feature was able to iterate through a block of rows and process each row individually. This is the same behavior as the split feature which will go through an entire row.

RaymondWise commented 9 years ago

Is this in reference to CombineCells()?

byronwall commented 9 years ago

Yes, I should have been more explicit. There is probably a better name for that Sub as well. I think something like below will work. The idea is that you iterate through Rows and do the combination for each row on its own. For this one as it sits, it does not maintain any discontinuity in the input range. Not sure how best to handle that going forward. There is also the question of how to handle blanks. When this Sub was created, it was meant to be the reverse of the split into columns option so blanks were meaningful and needed to be delimited. I think there was a specific reason I wanted to do that back then.

Sub CombineCells()

    Dim rngInput As Range
    On Error GoTo errHandler
    Set rngInput = Application.InputBox("Select the range of cells to combine:", Type:=8)

    Dim strDelim As String
    strDelim = Application.InputBox("Delimeter:")
    If strDelim = "" Then GoTo errHandler
    If strDelim = "False" Then GoTo errHandler
    Dim rngOutput As Range
    Set rngOutput = Application.InputBox("Select the output range:", Type:=8)

    If rngOutput Is Nothing Then
        GoTo errHandler
    End If

    'only keep the upper right corner of output
    Set rngOutput = rngOutput.Cells(1, 1)

    Dim rngRow As Range
    For Each rngRow In rngInput.rows
        Dim arr_values As Variant
        arr_values = Application.Transpose(Application.Transpose(rngRow.Value))

        rngOutput = Join(arr_values, strDelim)
        'next cell will output one row lower
        Set rngOutput = rngOutput.Offset(1)
    Next rngRow

    Exit Sub
errHandler:
    MsgBox ("No Range or Delimiter Selected!")
End Sub
RaymondWise commented 9 years ago

The problem with Join is that it requires a one dimensional array. If you still want to use the array for speed, I'm sure it could be adopted. I went at it like this -

(ha! I finally figured out markdown here)

Sub CombineCells()
    'collect all user data up front
    Dim rngInput As Range
    On Error GoTo errHandler
    Set rngInput = Application.InputBox("Select the range of cells to combine:", Type:=8)

    Dim strDelim As String
    strDelim = Application.InputBox("Delimeter:")
    If strDelim = "" Then GoTo errHandler
    If strDelim = "False" Then GoTo errHandler
    Dim rngOutput As Range
    Set rngOutput = Application.InputBox("Select the output range:", Type:=8)

    'Check the size of input and adjust output
    Dim y As Long
    y = rngInput.Columns.Count

    Dim x As Long
    x = rngInput.Rows.Count

    rngOutput = rngOutput.Resize(x, 1)

    'Read input rows into a single string
    Dim strOutput As String
    For i = 1 To x
        strOutput = vbNullString
        For j = 1 To y
            strOutput = strOutput & strDelim & rngInput(i, j)
        Next
        'Get rid of the first character (strDelim)
        strOutput = Right(strOutput, Len(strOutput) - 1)
        'Print it!
        rngOutput(i, 1) = strOutput
    Next
    Exit Sub
errHandler:
    MsgBox ("No Range or Delimiter Selected!")
End Sub
byronwall commented 9 years ago

I am cheating with the Join call using the double Transpose to force the 2D array (.Value) back down to 1D (arr_values) and usingJoinon that 1D array. Since I am iterating by rows, the underlying data is 1D but.Value` returns the 2D array by default. That code I posted was tested and it worked.

If we want to be fast with an array for output, the code above could use Join for each row the way it is above but output the result into an array which is then dumped back into the sheet.

As a general point, I know that it's good form to work with arrays when outputting data back to a multi-cell Range. I've always been lazy about using them because I normally don't notice the speed impact with the size of ranges I deal with. I also have generally erred on the side of allowing discontinuous ranges as input/output (if you can't tell :smile:) and array outputs tend to break that (or at least overwrite cells in middle that should have not been touched).

I'd support using the array for output for an example like this one because I don't think the discontinuous range is of much concern and it's possible this is done to a large range of data.

byronwall commented 9 years ago

This was also addressed with PR #22. I went with your version for now. It works when I tested it, and I am not too worried about implementation. We probably want to come back to this one and have it output the array instead of going cell by cell.

I will run through the code and find other places where array output would be more appropriate. From there, I will create new issues looking into that.