The comments I have fall into three categories that relate to general programming practices that would be applicable to any language, not just VBA.
- Raw Strings
- Don't Repeat Yourself (DRY)
- Single Responsibility Principle (SRP)
Raw Strings:
There are a lot of raw strings (examples: "CODICI", "Z12", "fine") in the code. I'm guessing that the ease of Copy-Paste made it simple to paste Worksheet("CODICI")
in the 10 places where a reference to sheet "CODICI" is needed. However, should you ever change the name of this sheet someday (and chances are excellent that you will want to), by then there will be at least 10 places to get wrong - and the compiler/interpreter will not help you find them. By adding a module level variable for the worksheet and assigning it once, you avoid this maintenance issue and reduce the volume of text in your subroutine. There is a performance improvement here as well, but it will not be noticeable - performance is not the motivation for the change. Raw strings like "CODICI" are generally to be avoided within your code for the above reasons. Strings that never change for you subroutine are excellent candidates to be declared as module level constants. And, when you want to change the name of the worksheet "CODICI", you only need to change your code in a single place (easy to get right!).
DRY:
DRY is important from both readability and maintenance of your code. Eliminating repeated blocks of code by writing small, focused, methods to eliminate the repeated code is the typical example of DRY. However, repeatedly writing raw strings or copy-pasting multiple line expressions falls under this principle as well.
SRP:
A simple search will provide you a better definition of SRP better than I can/will provide here. As the name implies, SRP encourages that each subroutine, function you write (in VBA or anything else) only does one thing. A good criteria for SRP is this: a subroutine (or function) has a single responsibility if it has only one reason to change. Your code has as single subroutine. Consequently, it contains many reasons to change because it does everything. Some of the reasons to change have been mentioned above within the DRY section. Full disclosure: I regularly fail to achieve SRP my own code, but it is certainly a good principle to aspire to.
Sub inserisciAutomatico()
is the macro entry point for the code - I would suggest that it's responsibility is to declare constants, initialize the necessary module level variables and call a sequence of operations to accomplish the task. The rewrite provided below does not accomplish this goal completely, but hopefully does so to a degree that is useful as an example.
By consolidating these tasks/responsibilities to smaller, single responsibility methods - you will make your code far easier to understand and maintain. As you have already done, declaring Option Explicit
at the top of your module is a good practice. It forces the explicit declaration of all variables used. A good indicator that Sub inserisciAutomatico()
is doing 'too much' is the long list of variables it has declared. Creating smaller focused subroutines and functions to accomplish the macro's goal will make many of these disappear.
The rewrite below removes many of raw strings by declaring them as constants. It also reduces the number of local variable declarations needed by calling subroutines focused on the tasks associated with the original variable. It also moves the remaining local variable declarations closer to their usage (generally a good practice). I'll apologize in advance for my lack of any skills in Italian. Some of the subroutines are named a bit strangely. The names were chosen to identify the principle that motivated their creation.
Option Explicit
Private Const TheMainWorksheetName As String = "CODICI"
Private Const DefaultFont As String = "Calibri"
Private Const DefaultFontSize As Long = 10
Private Const Appoggio As String = "fine"
Private Const ProvaFormulaRange As String = "B7:B250"
Private Const ValoreRange As String = "D4:D250"
Private Const DWG As String = ".dwg"
Private Const ASM As String = ".asm"
Private Const XLS As String = ".xls"
Private Const JPG As String = ".jpg"
Private Const ProvaFormulasAddress As String = "Z1"
Private Const PrimoValoreT0 = "T0"
'DRY - use a variable (needs a better name) to avoid
'repeating the expression 'Worksheets("CODICI")'
Private theMainWorksheet As Worksheet
Sub inserisciAutomatico()
Application.ScreenUpdating = False
Application.EnableEvents = False
Set theMainWorksheet = Worksheets(TheMainWorksheetName)
Dim risultati As Range
Set risultati = theMainWorksheet.Range(ValoreRange)
'Dim valore As String
'ComboBox1 needs a meaningful name to indicate what
'value it is providing - for this example, it has been named 'valoreComboBox'
'valore = theMainWorksheet .ComboBox1.value
'Renaming ComboBox1 eliminates variable 'valore' and there is no longer
'any uncertainty what is provided by the control
Dim cell As Range
Set cell = risultati.Find(theMainWorksheet .valoreComboBox.value)
Dim risultato As Range
Set risultato = risultati.Find(Appoggio, LookIn:=xlValues, After:=cell)
Dim risultatoAppoggio As Range
Set risultatoAppoggio = risultati.Find(theMainWorksheet .valoreComboBox.value, LookIn:=xlValues)
Dim kitNrSecondoAppoggio As String
kitNrSecondoAppoggio = risultatoAppoggio.Offset(-1, -3)
Dim provaFormula As Range
Set provaFormula = theMainWorksheet.Range(ProvaFormulasAddress)
provaFormula.FormulaArray = "=MAX(IF(LEFT(" & ProvaFormulaRange & ",1)=""" & kitNrSecondoAppoggio & """, " & ProvaFormulaRange & "))"
Range(risultato.Address).EntireRow.Insert
SRP_SetupPrimoValore risultato.Offset(-1, -3).Address, PrimoValoreT0
'KitNR was originally declared as a String, so the original KitNr + 1 operation below acted like integer addition
'because VBA implicitly converts KitNr to a number. Declare KitNr as an actual number type and eliminate any confusion
Dim KitNr As Long
KitNr = CLng(theMainWorksheet.Range(ProvaFormulasAddress).value)
'The subrouting wants a string...so explicitly provide it...again to eliminate any confusion
SRP_SetupNumeroID risultato.Offset(-1, -2).Address, CStr(KitNr + 1)
'TextBox1 needs a meaningful name to indicate what it is providing
SRP_SetupDescrizione risultato.Offset(-1, 0).Address, wksht.TextBox1.value
'ComboBox2 needs a meaningful name to indicate what it is providing
SRP_SetupGruppo risultato.Offset(-1, 1).Address, wksht.ComboBox2.value
SRP_SetupTheFormula risultato.Offset(-1, 2).Address, risultato.Offset(-2, 2).Address
Range(risultato.Offset(-1, 3).Address).value = DWG
Range(risultato.Offset(-1, 4).Address).value = ASM
Range(risultato.Offset(-1, 5).Address).value = XLS
Range(risultato.Offset(-1, 6).Address).value = JPG
End Sub
Private Sub SRP_SetupPrimoValore(ByVal addr As String, ByVal value As String)
Range(addr).value = value
End Sub
Private Sub SRP_SetupNumeroID(ByVal addr As String, ByVal kitNrAppoggio As String)
Range(addr).value = kitNrAppoggio
DRY_AvoidRepeatingTheFormattingOfNumeroIDAndDescrizione Range(addr)
End Sub
Private Sub SRP_SetupDescrizione(ByVal addr As String, ByVal value As String)
Range(addr).value = value
DRY_AvoidRepeatingTheFormattingOfNumeroIDAndDescrizione Range(addr)
End Sub
Private Sub SRP_SetupGruppo(ByVal addr As String, ByVal comboBox2Value As String)
Range(addr).value = comboBox2Value
Range(addr).NumberFormat = "@"
End Sub
Private Sub SRP_SetupTheFormula(ByVal addr As String, ByVal formulaAddress As String)
Range(formulaAddress).Select
Selection.Copy
Range(addr).PasteSpecial xlPasteFormulas
End Sub
Private Sub DRY_AvoidRepeatingTheFormattingOfNumeroIDAndDescrizione(ByRef theCell As Range)
theCell.Font.Bold = False
theCell.Font.Name = DefaultFont
theCell.Font.Size = DefaultFontSize
theCell.Borders(xlEdgeBottom).LineStyle = XlLineStyle.xlContinuous
End Sub
The 'raw' numbers within the Offset(...)
statements are still a maintence issue waiting to happen. I would consider creating a small ClassModule or UserDefinedType (UDT) to host the rowOffset and columnOffset values. After creating the Class or UDT, create module level variables with names for each Offset pair to identify what the offsets mean (for example, "risultatoToNumerIDOffset"). The result is that risultato.Offset(-1, -2)
becomes something like risultato.Offset(risultatoToNumerIDOffset.Columns, risultatoToNumerIDOffset.Rows)
and the offset values are no longer spread throughout the code.
<title>
should succinctly state what the code does.<body>
should tell us about what the code does (any why you've made any unusual choices, if you have). It should also include a completely functional routine (or more than one as necessary). Including what you're looking for in a review (as you've done) is also helpful. \$\endgroup\$