My code as a whole works for me without issues, but what I want to improve is a small part right out of the middle of it. I hope it's ok to post a simplified snippet here that just shows the structure (it's not something you can copy paste and it works), but I'll try to explain what it is about. If this is against the rules, let me know.
I have a workbook that functions as a database (wbDB
). I look for the number of an object (objectNumber
) and, if found, add it to a sheet (wsTest
) in another workbook via Sub AddObject
.
Option Explicit
Public Const fRowType As Long = 4
Public Const fRowClosing As Long = fRowType + 1
Public Const fRowLoan As Long = fRowType + 4
'[there are about 30 of those fRow variables; they are needed in another _
module, that's why they are public]
Dim wbDB As Workbook
Dim wsDB As Worksheet, wsTest As Worksheet
Dim lColTest As Long
Sub AddObject(ByVal objectNumber As String, ByVal rngObjects As Range)
Const colType As String = "A"
Const colClosing As String = "BN"
Const colLoan As String = "CD"
'[same as fRows, I have over 30 of these "col"-variables]
Dim rowObject As Long
Dim foundCell As Range
Set wbDB = Application.Workbooks("Database.xlsx")
Set wsDB = wbDB.Sheets(2)
Set wsTest = ActiveSheet
'---Find object in rngObjects and set rowObject to respective row
Set foundCell = rngObjects.Find(What:=objectNumber, LookAt:=xlWhole)
If foundCell Is Nothing Then
MsgBox "Object " & objectNumber & "wasn't found and will be skipped."
Exit Sub
Else
rowObject = foundCell.Row
End If
lColTest = wsTest.Cells(12, wsTest.Columns.Count).End(xlToLeft).Column
'Type
Call ImportData(rowObject, colType, fRowType)
'Closing
Call ImportData(rowObject, colClosing, fRowClosing)
'Loan
Call ImportData(rowObject, colLoan, fRowLoan)
'[again, there are 30+ of these calls]
End Sub
Sub ImportData(ByVal rowDB As Long, ByVal colDB As String, ByVal fRowSection As Long)
wsTest.Cells(fRowSection, lColTest + 1).Value = wsDB.Cells(rowDB, colDB).Value
End Sub
If you look at Sub ImportData
: It requires lColTest
, a module-wide variable, which I initialize in Sub AddObject
. On the other hand, rowObject
is needed just as often, but I put it as one of the parameters of ImportData
. What is better practice? From what I've read is that the wider the scope of a variable, the less desirable it is. However, if it is one of the parameters, I currently copy paste it about 30 times with every Call
and every additional parameter makes it a little less readable too.
My idea as the "cleanest" solution would be: put it in as parameter and instead of typing out every Call
, I loop through it. Would you agree with that? If the answer is yes, I'd need an array or something similar.
What's the best option here since I need always need a pair of values (column and first row)?
Is there an efficient or readable way to fill the array with all these variables?
-
1\$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 2 → 1 \$\endgroup\$Sᴀᴍ Onᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2020年02月27日 23:04:45 +00:00Commented Feb 27, 2020 at 23:04
-
1\$\begingroup\$ FWIW a simplified snippet isn't against the rules per se, but makes you needlessly walk a thin blurry line between actual code and pseudocode, the latter being off-topic. Best include the 30+ constants instead of just saying "there are 30 more like this" - code that's not in the post isn't reviewable, and you generally get better feedback when you simply include your code exactly as you have it. Don't worry about making a long post, we're used to that on this site (and the post length limit is twice that of Stack Overflow for a reason!). \$\endgroup\$Mathieu Guindon– Mathieu Guindon2020年02月27日 23:44:59 +00:00Commented Feb 27, 2020 at 23:44
-
\$\begingroup\$ @SᴀᴍOnᴇᴌᴀ Thank you, I'll keep that in mind in the future. \$\endgroup\$Alex– Alex2020年02月28日 08:47:54 +00:00Commented Feb 28, 2020 at 8:47
-
\$\begingroup\$ @MathieuGuindon Thank you for the tips. In this case, if I were to include everything and not at least simplify it, the code would be fairly long and, more importantly, without both the workbooks/data structure you can't get it to run. So what is the best thing to do in this case? Upload the file somewhere? Because that was my issue - I thought if you can't run it anyway, removing the clutter might make it more understandable. \$\endgroup\$Alex– Alex2020年02月28日 08:52:38 +00:00Commented Feb 28, 2020 at 8:52
1 Answer 1
Public Const fRowType As Long = 4
Public Const fRowClosing As Long = fRowType + 1
Public Const fRowLoan As Long = fRowType + 4
'[there are about 30 of those fRow variables; they are needed in another _
module, that's why they are public]
First of all, they're not "variables" they're "constants".
Second, that's screaming for an array or scripting.dictionary
to hold them instead of a hoard of Const
. They could even be pulled into a class that can easily be initialized provided with Get
ter properties (if the values are as fixed as Const
would indicate) then the class can be passed around wherever it's needed.
-
\$\begingroup\$ Fixed the constant/variable mix up. I have to look into classes, I don't have experience with them. As for an array: I get that it makes sense to store them in there, but to make sure I'm not missing something very basic: It wouldn't save lines of code, I'd still have to initiliaze them the same way? \$\endgroup\$Alex– Alex2020年02月27日 16:45:37 +00:00Commented Feb 27, 2020 at 16:45
-
2\$\begingroup\$ @Alex "Saving lines of code" is a pointless exercise, though, actually it will. You'll have one
Dim rowClosing(30) as Long
statement, et voila 30 variables declared. Yes, you'll have to populate them, but that can now be done in a loop instead of 1 by 1. \$\endgroup\$FreeMan– FreeMan2020年02月27日 17:55:35 +00:00Commented Feb 27, 2020 at 17:55 -
1
-
\$\begingroup\$ I'm gonna read up on that, I have come in touch with OOP a few times, but always kind of shied away from it. To make sure I get what you mean by "you'll have to populate them, but that can now be done in a loop ": obviously an array will allow me to do the
Call ImportData
in one loop, but initializing the variables would still need to be one by one (Public Const fRowType As Long = 4
etc and populalting the array would then bearrayRows = (fRowType, fRowClosing...)
. I still see the advantage, but I wanna make sure I understand you correctly. \$\endgroup\$Alex– Alex2020年02月28日 09:02:35 +00:00Commented Feb 28, 2020 at 9:02 -
1\$\begingroup\$ @Alex perhaps if you provide more of your code (in particular the initialization code) I could rewrite it into a loop for you. At this point, all I know is one fixed value and a couple of minor variations on it. I'd need to see how the different values vary in order to know how to build the loop. \$\endgroup\$FreeMan– FreeMan2020年02月28日 12:35:11 +00:00Commented Feb 28, 2020 at 12:35