2
\$\begingroup\$

I have details of key decisions made by a business in a worksheet "Decision Record."

My data is laid out as follows:

Range sample

I have written some code to create a timeline of these decisions using an XY scatter chart, like this one:

Timeline sample

The code works fine, but as with most of the code I write, it doesn't look tidy to me. Perhaps it's because of the long with statement, which seems like it could be simplified, maybe with a separate with statement for the DataLabels method.

Is this good practice? Is there something else I should be doing to tidy this up?

Sub UpdateDecisionTimelineChart() Dim scount As Integer Dim labelrotation As Integer Dim c As Range ActiveSheet.ChartObjects("chtDecisionTimeline").Activate scount = 0 For Each c In Range(Worksheets("Decision Record").Range("C7"), Worksheets("Decision Record").Range("C7").End(xlDown)) scount = scount + 1 ActiveChart.SeriesCollection.NewSeries With ActiveChart.SeriesCollection(scount) .Name = "='Decision Record'!" & c.Offset(0, 1).Address .XValues = "='Decision Record'!" & c.Address .Values = "='Decision Record'!" & c.Offset(0, -1).Address .MarkerStyle = 8 .MarkerSize = 7 .MarkerBackgroundColor = RGB(228, 10, 56) .MarkerForegroundColor = -2 .Format.Line.Visible = False .ApplyDataLabels .DataLabels.ShowValue = False .DataLabels.ShowSeriesName = True .DataLabels.Position = xlLabelPositionAbove .DataLabels.Orientation = -45 End With Next End Sub 
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Your variable names could use some tidying up-

Instead of c why not record as they are in the "Decision Record"

labelrotation should be labelRotation - Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

Same goes for scount - sCount or even recordCount


Integers - integers are obsolete. According to msdn VBA silently converts all integers to long.


Be sure to avoid things like .Activate - it just slows the code down by needing to fiddle with the spreadsheet while doing everything else behind the scenes. There's a good question on StackOverflow addressing this - https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba-macros .

Dim timelineChart As Chart Set timelineChart = ActiveSheet.ChartObjects("chtDecisionTimeline") ... timelineChart.SeriesCollection.NewSeries With timelineChart.SeriesCollection(scount) 

Speaking of structure, your spacing is a little off - your with block is the same level as your for block. Try pushing the loop back so alignment is correct -

Option Explicit Sub UpdateDecisionTimelineChart() Dim sCount As Long Dim labelrotation As Long Dim record As Range Dim timelineChart As Chart Set timelineChart = ActiveSheet.ChartObjects("chtDecisionTimeline") sCount = 0 For Each record In Range(Worksheets("Decision Record").Range("C7"), Worksheets("Decision Record").Range("C7").End(xlDown)) sCount = sCount + 1 timelineChart.SeriesCollection.NewSeries With timelineChart.SeriesCollection(sCount) .Name = "='Decision Record'!" & record.Offset(0, 1).Address .XValues = "='Decision Record'!" & record.Address .Values = "='Decision Record'!" & record.Offset(0, -1).Address .MarkerStyle = 8 .MarkerSize = 7 .MarkerBackgroundColor = RGB(228, 10, 56) .MarkerForegroundColor = -2 .Format.Line.Visible = False .ApplyDataLabels .DataLabels.ShowValue = False .DataLabels.ShowSeriesName = True .DataLabels.Position = xlLabelPositionAbove .DataLabels.Orientation = -45 End With Next End Sub 

The object model for the series object doesn't really specify any of the attributes' defaults, so I think your With block is pretty clean. You might want to create some variables or something like -

Dim decisionRecord As String decisionRecord = "='Decision Record'!" .Name = decisionRecord & record.Offset(0, 1).Address .XValues = decisionRecord & record.Address .Values = decisionRecord & record.Offset(0, -1).Address 

Your xlDown could be fixed like this -

Dim decisionRecordSheet As Worksheet Set decisionRecordSheet = Worksheets("Decision Record") Dim lastRow As Long lastRow = decisionRecordSheet.Cells(Rows.Count, 3).End(xlUp).Row Dim recordRange As Range Set recordRange = decisionRecordSheet.Range(Cells(7, 3), Cells(lastRow, 3)) ... For Each record In recordRange 

With all these variables using the sheet, why not use the CodeName - Worksheets have a CodeName property - View Properties window (F4) and the (Name) field can be used as the worksheet name. This way you can avoid Sheets("mySheet") and instead just use mySheet.


Unless you need it to be Public you should make it private

Private Sub UpdateDecisionTimelineChart() 

Also, I don't see you ever using the labelRotation variable, so unless that part was removed, you don't need it.

\$\endgroup\$
4
  • \$\begingroup\$ to the editor @Jon Peltier if you come back, I rolled back because I don't see why VBE would convert back to integer. If you have a source for that, I would be happy to include it. \$\endgroup\$ Commented May 9, 2016 at 11:09
  • \$\begingroup\$ Because your variable is an Integer, and VBA tries to put the Long value back into your Integer. If the Long value is too large for an Integer, you blow up the variable. No source for that other than private conversation with forgotten individuals long ago. \$\endgroup\$ Commented May 14, 2016 at 23:52
  • \$\begingroup\$ @JonPeltier the type checking still occurs. As far as the VBE is concerned, it's still an 16 bit int. it's just represented as a 32 bit integer at the OS level. I'd be interested in hearing more about about what you were trying to say/have experienced with this. \$\endgroup\$ Commented May 15, 2016 at 13:54
  • \$\begingroup\$ Maybe it's this checking that fails, because the value is too large to fit in an Integer, not an actual conversion back to Integer. I must admit I'm not really an expert here, just telling a story that made sense to me when I heard it ages ago. \$\endgroup\$ Commented May 17, 2016 at 0:47

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.