Bad coding habits

[Sissyfoo]

Hello,

I have this macro here which is pretty bulky and could probably be made more efficient. It isn't really important or critical to my project buuuut I couldn't think if there was a more efficient/shorter way to code it what with being a n00b an' all. :)

I'd be much obliged if someone could cast an eye over it and suggest any improvements etc. Again, it isn't really important to my project but if ya don't ask questions, ya never learn.


Sub Perf_Import()

Dim SAQtr, SAMnth1, SAMnth2, SAMnth3 As Range 'Imported data stored on sheet("blah") for SA
Dim WAQtr, WAMnth1, WAMnth2, WAMnth3 As Range 'Imported data stored on sheet("blah") for WA
Dim NSWQtr, NSWMnth1, NSWMnth2, NSWMnth3 As Range 'Imported data stored on sheet("blah") for NSW
Dim QLDQtr, QLDMnth1, QLDMnth2, QLDMnth3 As Range 'Imported data stored on sheet("blah") for QLD
Dim VICQtr, VICMnth1, VICMnth2, VICMnth3 As Range 'Imported data stored on sheet("blah") for VIC
Dim PerfSA1, PerfSA2, PerfSA3, PerfSA4 As Range 'chart range of SA from Qtr-Mnth3
Dim PerfWA1, PerfWA2, PerfWA3, PerfWA4 As Range 'chart range of WA from Qtr-Mnth3
Dim PerfNSW1, PerfNSW2, PerfNSW3, PerfNSW4 As Range 'chart range of NSW from Qtr-Mnth3
Dim PerfQLD1, PerfQLD2, PerfQLD3, PerfQLD4 As Range 'chart range of QLD from Qtr-Mnth3
Dim PerfVIC1, PerfVIC2, PerfVIC3, PerfVIC4 As Range 'chart range of VIC from Qtr-Mnth3

Set SAQtr = Sheets("blah").Range("q1:s16")
Set SAMnth1 = Sheets("blah").Range("q18:s33")
Set SAMnth2 = Sheets("blah").Range("q35:s50")
Set SAMnth3 = Sheets("blah").Range("q52:s67")
Set WAQtr = Sheets("blah").Range("t1:v16")
Set WAMnth1 = Sheets("blah").Range("t18:v33")
Set WAMnth2 = Sheets("blah").Range("t35:v50")
Set WAMnth3 = Sheets("blah").Range("t52:v67")
Set NSWQtr = Sheets("blah").Range("w1:y16")
Set NSWMnth1 = Sheets("blah").Range("w18:y33")
Set NSWMnth2 = Sheets("blah").Range("w35:y50")
Set NSWMnth3 = Sheets("blah").Range("w52:y67")
Set QLDQtr = Sheets("blah").Range("z1:ab16")
Set QLDMnth1 = Sheets("blah").Range("z18:ab33")
Set QLDMnth2 = Sheets("blah").Range("z35:ab50")
Set QLDMnth3 = Sheets("blah").Range("z52:ab67")
Set VICQtr = Sheets("blah").Range("ac1:ae16")
Set VICMnth1 = Sheets("blah").Range("ac18:ae33")
Set VICMnth2 = Sheets("blah").Range("ac35:ae50")
Set VICMnth3 = Sheets("blah").Range("ac52:ae67")

Set PerfSA1 = Sheets("Performance Plan").Range("g43:i58")
Set PerfSA2 = Sheets("Performance Plan (2)").Range("g43:i58")
Set PerfSA3 = Sheets("Performance Plan (3)").Range("g43:i58")
Set PerfSA4 = Sheets("Performance Plan (4)").Range("g43:i58")
Set PerfWA1 = Sheets("Performance Plan").Range("g74:i89")
Set PerfWA2 = Sheets("Performance Plan (2)").Range("g74:i89")
Set PerfWA3 = Sheets("Performance Plan (3)").Range("g74:i89")
Set PerfWA4 = Sheets("Performance Plan (4)").Range("g74:i89")
Set PerfNSW1 = Sheets("Performance Plan").Range("g105:i120")
Set PerfNSW2 = Sheets("Performance Plan (2)").Range("g105:i120")
Set PerfNSW3 = Sheets("Performance Plan (3)").Range("g105:i120")
Set PerfNSW4 = Sheets("Performance Plan (4)").Range("g105:i120")
Set PerfQLD1 = Sheets("Performance Plan").Range("g136:i151")
Set PerfQLD2 = Sheets("Performance Plan (2)").Range("g136:i151")
Set PerfQLD3 = Sheets("Performance Plan (3)").Range("g136:i151")
Set PerfQLD4 = Sheets("Performance Plan (4)").Range("g136:i151")
Set PerfVIC1 = Sheets("Performance Plan").Range("g167:i182")
Set PerfVIC2 = Sheets("Performance Plan (2)").Range("g167:i182")
Set PerfVIC3 = Sheets("Performance Plan (3)").Range("g167:i182")
Set PerfVIC4 = Sheets("Performance Plan (4)").Range("g167:i182")

Sheets("blah").Activate

SAQtr.Copy
ActiveSheet.Paste Destination:=PerfSA1
SAMnth1.Copy
ActiveSheet.Paste Destination:=PerfSA2
SAMnth2.Copy
ActiveSheet.Paste Destination:=PerfSA3
SAMnth3.Copy
ActiveSheet.Paste Destination:=PerfSA4
WAQtr.Copy
ActiveSheet.Paste Destination:=PerfWA1
WAMnth1.Copy
ActiveSheet.Paste Destination:=PerfWA2
WAMnth2.Copy
ActiveSheet.Paste Destination:=PerfWA3
WAMnth3.Copy
ActiveSheet.Paste Destination:=PerfWA4
NSWQtr.Copy
ActiveSheet.Paste Destination:=PerfNSW1
NSWMnth1.Copy
ActiveSheet.Paste Destination:=PerfNSW2
NSWMnth2.Copy
ActiveSheet.Paste Destination:=PerfNSW3
NSWMnth3.Copy
ActiveSheet.Paste Destination:=PerfNSW4
QLDQtr.Copy
ActiveSheet.Paste Destination:=PerfQLD1
QLDMnth1.Copy
ActiveSheet.Paste Destination:=PerfQLD2
QLDMnth2.Copy
ActiveSheet.Paste Destination:=PerfQLD3
QLDMnth3.Copy
ActiveSheet.Paste Destination:=PerfQLD4
VICQtr.Copy
ActiveSheet.Paste Destination:=PerfVIC1
VICMnth1.Copy
ActiveSheet.Paste Destination:=PerfVIC2
VICMnth2.Copy
ActiveSheet.Paste Destination:=PerfVIC3
VICMnth3.Copy
ActiveSheet.Paste Destination:=PerfVIC4

Application.CutCopyMode = False

End Sub


It is basically a script to copy ranges from one sheet and split them up into ranges on 4 different sheets. I thought about using collections and arrays to scoop up the data but wasn't sure if it would make much difference to the speed or if it would impact my s/s at all.

Cheers,

James

[BlueCactus]

My first observation will actually ADD bulk to your code. When you say
Dim SAQtr, SAMnth1, SAMnth2, SAMnth3 As Range
, only SAMnth3 is of type Range. The others are of type Variant. :devil:

Without thinking about this too hard, your code looks like it would benefit from using arrays instead of all those individual variables.

[Jacob Hilderbrand]

You can use one line for the copy/paste.


SAQtr.Copy Destination:=PerfSA1

[Ken Puls]

If you wanted to cut down on the bulk of code, you could set up a pile of named ranges in your workbook. This would give you some benefits, but also some potential pitfalls as well.

Benefits:
-You could set the named ranges up, and then if you increased the size of the range, you wouldn't need to edit the code as well. (The named ranges take care of that for you.)
-You could avoid all of your variable delcarations in favour of the following syntax (providing that your names are consistent with what you laid out above):

Range("SAQtr").Copy Range("PerfSA1")

Drawbacks:
-Managing the named ranges. Some people don't like a ton of them in their workbooks.
-I believe that I've read that they slow down performance, although someone will have to confirm that.
-You could run into issues if you increase range SAQtr and not PerfSA1 in the example above.

Personally, I always used named ranges on my worksheets and in my code for one simple reason... I hate inserting a worksheet row and having it break my code. To me, they dynamic update is worth any overhead issues. I also make sure I try to group as much input data in one place as I can though, so that I can use as few named ranges as possible (and hit them in 1-2 blocks.

HTH,

[johnske]

Why declare all these ranges as variables? Also, you can also omit the "Destination:="

This is where shortcut notation (Range("A1:C5") becomes [A1:C5]) comes in handy to make everything more readable.

Unless I missed a copy paste somewhere in the code pane this should do the same thing (I probably did miss one because there's one range left over - but you get the idea)Sub Perf_Import()
'Set SAQtr = Sheets("blah").[q1:s16] ??? this's left over

With Sheets("blah")
.[q1:s16].Copy Sheets("Performance Plan").[g43:i58]
.[q18:s33].Copy Sheets("Performance Plan (2)").[g43:i58]
.[q35:s50].Copy Sheets("Performance Plan (3)").[g43:i58]
.[q52:s67].Copy Sheets("Performance Plan (4)").[g43:i58]
.[t1:v16].Copy Sheets("Performance Plan").[g74:i89]
.[t18:v33].Copy Sheets("Performance Plan (2)").[g74:i89]
.[t35:v50].Copy Sheets("Performance Plan (3)").[g74:i89]
.[t52:v67].Copy Sheets("Performance Plan (4)").[g74:i89]
.[w1:y16].Copy Sheets("Performance Plan").[g105:i120]
.[w18:y33].Copy Sheets("Performance Plan (2)").[g105:i120]
.[w35:y50].Copy Sheets("Performance Plan (3)").[g105:i120]
.[w52:y67].Copy Sheets("Performance Plan (4)").[g105:i120]
.[z1:ab16].Copy Sheets("Performance Plan").[g136:i151]
.[z18:ab33].Copy Sheets("Performance Plan (2)").[g136:i151]
.[z35:ab50].Copy Sheets("Performance Plan (3)").[g136:i151]
.[z52:ab67].Copy Sheets("Performance Plan (4)").[g136:i151]
.[ac1:ae16].Copy Sheets("Performance Plan").[g167:i182]
.[ac18:ae33].Copy Sheets("Performance Plan (2)").[g167:i182]
.[ac35:ae50].Copy Sheets("Performance Plan (3)").[g167:i182]
.[ac52:ae67].Copy Sheets("Performance Plan (4)").[g167:i182]
End With
Application.CutCopyMode = False
End Sub

[Bob Phillips]

This is where shortcut notation (Range("A1:C5") becomes [A1:C5]) comes in handy to make everything more readable.

That is a matter of opinion. In mine, it is less readable, as it hdoesn't reflect the object model, which is fundamental to programming Excel. And it is slower, as it has to be deciphered at run-time.

[johnske]

.... And it is slower, as it has to be deciphered at run-time.Not quite, it is deciphered when it is compiled, it is then converted to "Range("A1") style, so it takes a little longer to compile. However, once it has been compiled and saved it should thereafter take no longer.

Because both the code and the compiled version of the code are different, this makes the file-size marginally larger, if you want to equate a larger file-size to "running slower"... :dunno ... Several of us tested the speed in response to a similar type of comment in regard to my article http://www.vbaexpress.com/forum/articles.php?action=viewarticle&artid=37 and the end result was "no real significant difference".

But, apart from a larger file-size, there are two other disadvantages i) "intellisense" doesn't work with it ii) you cant put anything but ranges inside the quotation marks.

The "Object model"? Personally, I consider anything that is in the VBA Help files that is designed to work with/'run' VBA code as being part of the Object model (and this is in the Help files) so I also consider it part of the object model :devil:

Regards,
John :)

[Ken Puls]

That is a matter of opinion. In mine, it is less readable

Sorry, John. I tend to agree with Bob on this. :stir:

On the runtime though, I was part of the test. To my recollection, you're right. We couldn't discern a real difference at all, despite MS's claims that it is true. :dunno

[Bob Phillips]

The "Object model"? Personally, I consider anything that is in the VBA Help files that is designed to work with/'run' VBA code as being part of the Object model (and this is in the Help files) so I also consider it part of the object model :devil:

That's a strange definition of the object model! To my mind the object model is that which is exposed via the type library. As you state, intellisense doesn't work, which means that it isn't in the type library, which to me means that isn't part of the object model.

[johnske]

That's a strange definition of the object model! To my mind the object model is that which is exposed via the type library. As you state, intellisense doesn't work, which means that it isn't in the type library, which to me means that isn't part of the object model.I have NEVER in all my life, laid claim to being conventional :devil:

[Bob Phillips]

I have NEVER in all my life, laid claim to being conventional :devil:

Not arguing with an Ozzie http://vbaexpress.com/forum/images/smilies/001.gif.

[johnske]

BTW Sissyfoo (as per the thread title), while sometimes convenient to 'read' it's not the best practice to use the sheets name in your code as this can be changed by users and your code immediately malfunctions.

It's preferable to use the sheets code-name (Sheet1, Sheet2, Sheet3, etc.) as I've shown below. This also makes your code shorter... (The code-name of a sheet can be found in the project explorer - it's the name on the left-hand side).


Sub Perf_Import2()
'Set SAQtr = Sheets("blah").[q1:s16] ??? this's left over
With Sheet1
.[q1:s16].Copy Sheet2.[g43]
.[q18:s33].Copy Sheet3.[g43]
.[q35:s50].Copy Sheet4.[g43]
.[q52:s67].Copy Sheet5.[g43]
.[t1:v16].Copy Sheet2.[g74]
.[t18:v33].Copy Sheet3.[g74]
.[t35:v50].Copy Sheet4.[g74]
.[t52:v67].Copy Sheet5.[g74]
.[w1:y16].Copy Sheet2.[g105]
.[w18:y33].Copy Sheet3.[g105]
.[w35:y50].Copy Sheet4.[g105]
.[w52:y67].Copy Sheet5.[g105]
.[z1:ab16].Copy Sheet2.[g136]
.[z18:ab33].Copy Sheet3.[g136]
.[z35:ab50].Copy Sheet4.[g136]
.[z52:ab67].Copy Sheet5.[g136]
.[ac1:ae16].Copy Sheet2.[g167]
.[ac18:ae33].Copy Sheet3.[g167]
.[ac35:ae50].Copy Sheet4.[g167]
.[ac52:ae67].Copy Sheet5.[g167]
End With
Application.CutCopyMode = False
End Sub Another thing is, seeing you're only copying and pasting a relatively small number of cells, do you really need to paste? You can transfer values when you want like this...

Sub Perf_Import3()
'Set SAQtr = Sheets("blah").[q1:s16] ??? this's left over
With Sheet1
Sheet2.[g43:i58] = .[q1:s16].Value
Sheet3.[g43:i58] = .[q18:s33].Value
Sheet4.[g43:i58] = .[q35:s50].Value
Sheet5.[g43:i58] = .[q52:s67].Value
Sheet2.[g74:i89] = .[t1:v16].Value
Sheet3.[g74:i89] = .[t18:v33].Value
Sheet4.[g74:i89] = .[t35:v50].Value
Sheet5.[g74:i89] = .[t52:v67].Value
Sheet2.[g105:i120] = .[w1:y16].Value
Sheet3.[g105:i120] = .[w18:y33].Value
Sheet4.[g105:i120] = .[w35:y50].Value
Sheet5.[g105:i120] = .[w52:y67].Value
Sheet2.[g136:i151] = .[z1:ab16].Value
Sheet3.[g136:i151] = .[z18:ab33].Value
Sheet4.[g136:i151] = .[z35:ab50].Value
Sheet5.[g136:i151] = .[z52:ab67].Value
Sheet2.[g167:i182] = .[ac1:ae16].Value
Sheet3.[g167:i182] = .[ac18:ae33].Value
Sheet4.[g167:i182] = .[ac35:ae50].Value
Sheet5.[g167:i182] = .[ac52:ae67].Value
End With
Application.CutCopyMode = False
End Sub HTH,
John :)

[geekgirlau]

:2p: If you use range names, you could have something like the following:


Dim strState(0 To 4) As String
Dim i As Integer
Dim n As Integer


strState(0) = "SA"
strState(1) = "WA"
strState(2) = "NSW"
strState(3) = "QLD"
strState(4) = "VIC"

For i = 1 To 4
For n = LBound(strState) To UBound(strState)
If i = 1 Then
Range(strState(n) & "Qtr").Copy Destination:=Range("Perf" & strState(n) & i)
Else
Range(strState(n) & "Mnth" & i - 1).Copy Destination:=Range("Perf" & strState(n) & i)
End If
Next n
Next i

Application.CutCopyMode = False

[Sissyfoo]

Hey!

Thanks for the great replies. :)

The reason I declared the ranges was just so that if I needed to change things then I could easily navigate my way through the modules and code. I think it is just one of my bad habits to limit the amount of numbers I have to trawl through when looking for something.

I liked the idea of transferring the values instead of copying. When I was thinking about the best way to do this report I didn't even think of transferring values. The great thing about excel is that there are so many different ways to do something. The bad thing about excel is that there are so many different ways to do something. It can get a bit confusing when trying to decide on the best method. :)

I agree with you about the sheet names but as I know for a fact that the sheet names will not be changed EVER (i.e. over my dead body) I figured it would be safe to use. Thinking about it now though, the snr directors and general managers would probably have no qualms about killing me and stepping over my corpse (I'm a temp, hence expendable and replaceable) so I reckon I might start using the sheet codes in future.

Thanks for all the great advice. :D

[Ken Puls]

:2p: If you use range names, you could have something like the following...

Cool! Nice work, Anne! :clap:

[geekgirlau]

:curtsey:

[Zack Barresse]

Hmm..

strState(0) = "SA"
strState(1) = "WA"
strState(2) = "NSW"
strState(3) = "QLD"
strState(4) = "VIC"

'...

strState() = Array("SA", "WA", "NSW", "QLD", "VIC")

[Ken Puls]

Hmm..

See... now YOU'RE just being argumentative... ;)

:rotlaugh: