8
\$\begingroup\$

I'm logging data from an embedded platform over UART. This gets saved to a log file and I want to process the saved log with Excel.

I have no idea whether what I did was even remotely a good solution. My VBA experience so far consists mainly of code only used once. However, since I'll have to use this for every measurement series I conduct it's probably a good idea to clean it up. Not in the least because I can't stand ugly code, I'm about to expand on it and colleagues may want to use it in the near future as well.

Example input:

00032
00016
00016
00016
00032
00064
00080
00096
00112
00128
00160
00192

The size of the input can be thousands of values. All leading zeroes get dropped as intended. Values can be negative.

Option Explicit

Public Sub Stats()
'
' Stats Macro
' Give stats to column A and draw chart with trendline.
'
' Keyboard Shortcut: Ctrl+Shift+M
'
    Columns("A:A").Select
    Selection.FormatConditions.AddColorScale ColorScaleType:=3
    Selection.FormatConditions(Selection.FormatConditions.Count).SetFirstPriority
    Selection.FormatConditions(1).ColorScaleCriteria(1).Type = _
        xlConditionValueLowestValue
    With Selection.FormatConditions(1).ColorScaleCriteria(1).FormatColor
        .Color = 7039480
        .TintAndShade = 0
    End With
    Selection.FormatConditions(1).ColorScaleCriteria(2).Type = _
        xlConditionValuePercentile
    Selection.FormatConditions(1).ColorScaleCriteria(2).Value = 50
    With Selection.FormatConditions(1).ColorScaleCriteria(2).FormatColor
        .Color = 8711167
        .TintAndShade = 0
    End With
    Selection.FormatConditions(1).ColorScaleCriteria(3).Type = _
        xlConditionValueHighestValue
    With Selection.FormatConditions(1).ColorScaleCriteria(3).FormatColor
        .Color = 8109667
        .TintAndShade = 0
    End With

    ActiveSheet.Shapes.AddChart2(227, xlLine).Select
    ActiveChart.SetSourceData Source:=Range("Sheet1!$A:$A")
    ActiveChart.FullSeriesCollection(1).Trendlines.Add Type:=xlMovingAvg, Period _
        :=2, Forward:=0, Backward:=0, DisplayEquation:=0, DisplayRSquared:=0, _
        Name:="2 per. Mov. Avg. (Series1)"
    ActiveChart.ClearToMatchStyle
    ActiveChart.ChartStyle = 233
    ActiveChart.FullSeriesCollection(1).Trendlines(1).Select
    With Selection.Format.Line
        .Visible = msoTrue
        .ForeColor.ObjectThemeColor = msoThemeColorAccent2
        .ForeColor.TintAndShade = 0
        .ForeColor.Brightness = -0.25
        .Transparency = 0
    End With

    Range("C1").Select
    ActiveCell.FormulaR1C1 = "Average:"
    Range("D1").Select
    ActiveCell.FormulaR1C1 = "=AVERAGE(C[-3])"
    Range("C2").Select
    ActiveCell.FormulaR1C1 = "Minimum:"
    Range("D2").Select
    ActiveCell.FormulaR1C1 = "=MIN(C[-3])"
    Range("C3").Select
    ActiveCell.FormulaR1C1 = "Maximum:"
    Range("D3").Select
    ActiveCell.FormulaR1C1 = "=MAX(C[-3])"
    Range("C4").Select
    ActiveCell.FormulaR1C1 = "Median:"
    Range("D4").Select
    ActiveCell.FormulaR1C1 = "=MEDIAN(C[-3])"
End Sub

Result looks like this:

Screenshot of result

I'm mainly worried about the lack of being generic. While ActiveCell and ActiveSheet and ActiveChart work wonders, I haven't figured out how to apply the same logic everywhere. There are still values like Sheet1 which should be set automatically for the current sheet/object/etc..

\$\endgroup\$
3
\$\begingroup\$

The golden rule of using the Macro Recorder:

The code is more what you'd call "guidelines" than actual rules.

It's there to tell you which objects/properties/methods to look at. Not to write your code for you.


Step 1: Clean up all the implicit references (Selection, Active*), indenting and line continuations

When the Recorder spits out this code:

Thing.Select
Selection.DoThing
Selection.Child.DoThing
Selection.Child.Child.Property =  _
    Value

You re-write it like so:

Thing.DoThing
Thing.Child.DoThing
Thing.Child.Child.Property = Value

And then like so:

With Thing
    .DoThing
    With .Child
        .DoThing
        .Child.Property = Value
    End With
End With

And then you go and put Thing in a proper variable so you can apply it to any generic ThingObject:

Dim targetThing as ThingObject
Set targetThing = Thing

With targetThing 
    .DoThing
    With .Child
        .DoThing
        .Child.Property = Value
    End With
End With

Applying to your code:

Dim targetCol As Range
Set targetCol = Columns("A:A")

With targetCol
    .FormatConditions.AddColorScale ColorScaleType:=3
    .FormatConditions(.FormatConditions.Count).SetFirstPriority
    .FormatConditions(1).ColorScaleCriteria(1).Type = xlConditionValueLowestValue
End With

With targetCol.FormatConditions(1).ColorScaleCriteria(1).FormatColor
    .Color = 7039480
    .TintAndShade = 0
End With

With targetCol.FormatConditions(1).ColorScaleCriteria(2)
    .Type = xlConditionValuePercentile
    .value = 50
    With .FormatColor
        .Color = 8711167
        .TintAndShade = 0
    End With
End With

With targetCol.FormatConditions(1).ColorScaleCriteria(3)
    .Type = xlConditionValueHighestValue
    With .FormatColor
        .Color = 8109667
        .TintAndShade = 0
    End With
End With

Dim targetSheet As Worksheet
Set targetSheet = ActiveSheet

Dim targetChart As Chart
Set targetChart = targetSheet.AddChart2(227, xlLine)

targetChart.SetSourceData source:=Range("Sheet1!$A:$A")
targetChart.FullSeriesCollection(1).Trendlines.Add Type:=xlMovingAvg, Period:=2, Forward:=0, Backward:=0 _
, DisplayEquation:=0, DisplayRSquared:=0, Name:="2 per. Mov. Avg. (Series1)"
targetChart.ClearToMatchStyle
targetChart.ChartStyle = 233

With targetChart.FullSeriesCollection(1).Trendlines(1).Format.Line
    .Visible = msoTrue
    .ForeColor.ObjectThemeColor = msoThemeColorAccent2
    .ForeColor.TintAndShade = 0
    .ForeColor.Brightness = -0.25
    .Transparency = 0
End With

Range("C1").FormulaR1C1 = "Average:"
Range("D1").FormulaR1C1 = "=AVERAGE(C[-3])"
Range("C2").FormulaR1C1 = "Minimum:"
Range("D2").FormulaR1C1 = "=MIN(C[-3])"
Range("C3").FormulaR1C1 = "Maximum:"
Range("D3").FormulaR1C1 = "=MAX(C[-3])"
Range("C4").FormulaR1C1 = "Median:"
Range("D4").FormulaR1C1 = "=MEDIAN(C[-3])"

and already it's much clearer how things are structured and what's going on. It's also clearly separated into 3 distinct operations (Add conditional Formatting, Add Chart, Add Summary Stats) which should be refactored into separate Subs.


Step 2: Initial Refactoring

Moving the 3 stages to separate subs. Tweaked the naming. Made AddLineChart a function which returns the chart object after creation for further manipulation (if desired). Made the summary stat ranges relative to the topLeftCell so you can move the table around by changing just one range declaration:

Option Explicit

Public Sub Stats()

    Dim dataRange As Range
    Set dataRange = Columns("A:A")

    AddConditionalFormatting dataRange


    Dim targetSheet As Worksheet
    Set targetSheet = ActiveSheet

    Dim targetChart As Chart
    Set targetChart = CreateLineChart(dataRange, targetSheet)


    Dim summaryStartCell As Range
    Set summaryStartCell = targetSheet.Range("C1")
    AddSumaryStats summaryStartCell '/ Moved the range declarations to be relative to base cell for portability.

End Sub

Public Sub AddConditionalFormatting(ByRef dataRange As Range)

    With dataRange
        .FormatConditions.AddColorScale ColorScaleType:=3
        .FormatConditions(.FormatConditions.Count).SetFirstPriority
        .FormatConditions(1).ColorScaleCriteria(1).Type = xlConditionValueLowestValue
    End With

    With dataRange.FormatConditions(1).ColorScaleCriteria(1).FormatColor
        .Color = 7039480
        .TintAndShade = 0
    End With

    With dataRange.FormatConditions(1).ColorScaleCriteria(2)
        .Type = xlConditionValuePercentile
        .value = 50
        With .FormatColor
            .Color = 8711167
            .TintAndShade = 0
        End With
    End With

    With dataRange.FormatConditions(1).ColorScaleCriteria(3)
        .Type = xlConditionValueHighestValue
        With .FormatColor
            .Color = 8109667
            .TintAndShade = 0
        End With
    End With

End Sub

Public Function CreateLineChart(ByRef dataRange As Range, ByRef targetSheet As Worksheet) As Chart

    Dim newChart As Chart
    Set newChart = targetSheet.AddChart2(227, xlLine)

    With newChart
        .SetSourceData source:=dataRange

        .FullSeriesCollection(1).Trendlines.Add Type:=xlMovingAvg, Period:=2, Forward:=0, Backward:=0 _
        , DisplayEquation:=0, DisplayRSquared:=0, Name:="2 per. Mov. Avg. (Series1)"

        .ClearToMatchStyle
        .ChartStyle = 233
    End With

    With newChart.FullSeriesCollection(1).Trendlines(1).Format.Line
        .Visible = msoTrue
        With .ForeColor
            .ObjectThemeColor = msoThemeColorAccent2
            .TintAndShade = 0
            .Brightness = -0.25
        End With
        .Transparency = 0
    End With

    Set CreateLineChart = newChart

End Function

Public Sub AddSummaryStats(ByRef baseCell As Range)

    Dim rowOffset As Long

    rowOffset = 0
    baseCell.Offset(rowOffset, 0).FormulaR1C1 = "Average:"
    baseCell.Offset(rowOffset, 1).FormulaR1C1 = "=AVERAGE(C[-3])"

    rowOffset = rowOffset + 1
    baseCell.Offset(rowOffset, 0).FormulaR1C1 = "Minimum:"
    baseCell.Offset(rowOffset, 1).FormulaR1C1 = "=MIN(C[-3])"

    rowOffset = rowOffset + 1
    baseCell.Offset(rowOffset, 0).FormulaR1C1 = "Maximum:"
    baseCell.Offset(rowOffset, 1).FormulaR1C1 = "=MAX(C[-3])"

    rowOffset = rowOffset + 1
    baseCell.Offset(rowOffset, 0).FormulaR1C1 = "Median:"
    baseCell.Offset(rowOffset, 1).FormulaR1C1 = "=MEDIAN(C[-3])"

End Sub

And now we've successfully split our task into 3 small, distinct, operations that can be further analysed/improved/refactored as necessary.

\$\endgroup\$
4
\$\begingroup\$

Avoid using Active*

You can use Worksheets collection to access a specified Worksheet object represents a worksheet.

Worksheets("YOUR SHEET NAME")

This can be useful to do explicit job, without changing the active sheet. The same way for ActiveCell, ActiveChart and ActiveChart, you can always access the target object by the collection.

Use With rather than Selection

Aside from error-raising macro problems, a recorded macro is less efficient, because it mimics all the mouseclicks and keystrokes (every cough and camera flash) that occurred while the recording was taking place. A recorded macro clicks on every object to select it, then performs an action on the selection:

Columns("A:A").Select
Selection.FormatConditions.AddColorScale ColorScaleType:=3
Selection.FormatConditions(Selection.FormatConditions.Count).SetFirstPriority

If you have two or more property or method statements that work on the same object, wrap them in a With / End With block:

With Worksheets("Sheet1").Columns("A")
    ' read / write object's properties here
    ' e.g. add new color scale rule
    .FormatConditions.AddColorScale ColorScaleType:=3
End With

This block can increase the code efficiency and readability.

Check the existence of target object

The macro record is powerful, but it's more difficult to understand whether something exists already. For example, we may not want to apply 10 rules of color scale to the column A.

' drop color scale if exists
For Each fc In .FormatConditions
    If fc.Type = xlColorScale Then
        fc.Delete
    End If
Next

Here's just an example, there must be better logic for treating existing object.


As for your script, here's my suggest edits.

Public Sub Stats2()
'
' Stats Macro
' Give stats to column A and draw chart with trendline.
'
' Modified by Mincong HUANG
'

    ' declarations
    Dim wsName As String
    Dim chartName As String
    Dim srcRange As Range

    ' initialization
    wsName = "Sheet1"
    chartName = "Fancy Chart"
    Set srcRange = Worksheets(wsName).Columns("A")

    ' set format conditions at target column
    With srcRange

        Debug.Print .Address

        ' drop color scale if exists
        For Each fc In .FormatConditions
            If fc.Type = xlColorScale Then
                fc.Delete
            End If
        Next

        ' add new color scale
        With .FormatConditions
            ' add
            .AddColorScale ColorScaleType:=3
            With .Item(.Count)

                .SetFirstPriority

                ' advanced setting (optional)
                ' Optional because these values are the default setting
                ' but you can add other desired color

                .ColorScaleCriteria(1).Type = xlConditionValueLowestValue
                .ColorScaleCriteria(1).FormatColor.Color = 7039480
                .ColorScaleCriteria(1).FormatColor.TintAndShade = 0

                .ColorScaleCriteria(2).Type = xlConditionValuePercentile
                .ColorScaleCriteria(2).FormatColor.Color = 8711167
                .ColorScaleCriteria(2).FormatColor.TintAndShade = 0
                .ColorScaleCriteria(2).Value = 50

                .ColorScaleCriteria(3).Type = xlConditionValueHighestValue
                .ColorScaleCriteria(3).FormatColor.Color = 8109667
                .ColorScaleCriteria(3).FormatColor.TintAndShade = 0

            End With
        End With
    End With

    ' drop log chart if exists
    Debug.Print Worksheets(wsName).ChartObjects.Count & " charts"
    For Each c In Worksheets(wsName).ChartObjects
        If c.Name = chartName Then
            c.Delete
        End If
    Next

    ' add log chart
    With Worksheets(wsName).Shapes.AddChart2(227, xlLine)
        .Chart.Parent.Name = chartName
        .Chart.HasTitle = True
        .Chart.ChartTitle.Text = chartName
        .Chart.SetSourceData Source:=srcRange
        .Chart.FullSeriesCollection(1).Trendlines.Add _
            Type:=xlMovingAvg, _
            Period:=2, _
            Forward:=0, _
            Backward:=0, _
            DisplayEquation:=0, _
            DisplayRSquared:=0, _
            Name:="2 per. Mov. Avg. (Series1)"
        .Chart.ClearToMatchStyle
        .Chart.ChartStyle = 233
        With .Chart.FullSeriesCollection(1).Trendlines(1).Format.Line
            .Visible = msoTrue
            .ForeColor.ObjectThemeColor = msoThemeColorAccent2
            .ForeColor.TintAndShade = 0
            .ForeColor.Brightness = -0.25
            .Transparency = 0
        End With
    End With

    ' release memory
    Set srcRange = Nothing

End Sub

By the way, I'm using Excel 2016. So I'm not sure the code compatibility. Please tell me if there's any error.


References

\$\endgroup\$
3
  • 2
    \$\begingroup\$ Worksheets("YOUR SHEET NAME") How is that an improvement? I'm trying to get rid of such calls. Changing the active sheet seems like a more maintainable and easier to extend approach, avoiding code duplication in the process. Don't get me wrong, I appreciate the suggestion, but I fail to see why it's better. \$\endgroup\$
    – Mast
    Apr 3 '16 at 9:10
  • \$\begingroup\$ Effectively, it depends the usage. I consider it as an improvement, because by precising the target sheet name, this sub Stats can take care multiple sheets. For example, if there're many log, you may want to use a sheet Dashboard to show charts, which referenced log data from sheets log 1, log 2, log 3. But if log and chart are shown together, ActiveSheet is better. \$\endgroup\$ Apr 3 '16 at 9:38
  • \$\begingroup\$ Valid points, I'll have to figure out the final use-case and act accordingly. \$\endgroup\$
    – Mast
    Apr 3 '16 at 12:21
2
\$\begingroup\$

I've found Sheet1 only once in your code:
Range("Sheet1!$A:$A"),
You can replace it to
ActiveSheet.Columms(1)

(Or, to make your code even more general, working also in cases where original data is not in first column: Selection.EntireColumn)

As @MincongHuang has suggested you can do more with With clauses, they can even be embedded in each other.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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