VBA Automatic insert in a Excel Spreadsheet

Question

Since I'm new at this site I will try to start with something very simple.

I'm making an "Automatic insert" in a Excel Spreadsheet and my code is working correctly, but I'm not so sure about my way of coding. Does the code follow good VBA programming practices?

Here is my code:

Option Explicit

Sub inserisciAutomatico()
Application.ScreenUpdating = False
Application.EnableEvents = False

Dim KitNr As String
Dim KitNrAppoggio As String
Dim kitNrSecondoAppoggio As String
Dim provaFormula As Range    
Dim primoValore As String    
Dim numeroID As String    
Dim descrizione As String
Dim gruppo As String

Dim concatena As String
Dim disegno As String
Dim treD As String
Dim distinta As String
Dim immagine As String
Dim formulaCopy As String


Dim cell As Range
Dim risultato As Range
Dim risultatoAppoggio As Range


Dim valore As String
Dim appoggio As String

Dim provissima As Integer

valore = Worksheets("CODICI").ComboBox1.Value
appoggio = "fine"

Set cell = Worksheets("CODICI").Range("D4:D250").Find(valore)

'Debug.Print cell.Address


Set risultato = Worksheets("CODICI").Range("D4:D250").Find(appoggio, LookIn:=xlValues, After:=cell)
Set risultatoAppoggio = Worksheets("CODICI").Range("D4:D250").Find(Worksheets("CODICI").ComboBox1.Value, LookIn:=xlValues)

'Debug.Print risultato.Address
'Debug.Print risultatoAppoggio.Address


'kitNr = risultato.Offset(0, -2)
kitNrSecondoAppoggio = risultatoAppoggio.Offset(-1, -3)

Set provaFormula = Worksheets("CODICI").Range("Z1")

provaFormula.FormulaArray = "=MAX(IF(LEFT(B7:B250,1)=""" & kitNrSecondoAppoggio & """, B7:B250))"

KitNr = Worksheets("CODICI").Range("Z1").Value

Debug.Print provaFormula.FormulaArray

KitNrAppoggio = KitNr + 1

'Application.WorksheetFunction.Max (Range("a:a"))

Range(risultato.Address).EntireRow.Insert

primoValore = risultato.Offset(-1, -3).Address
numeroID = risultato.Offset(-1, -2).Address
descrizione = risultato.Offset(-1, 0).Address
gruppo = risultato.Offset(-1, 1).Address
concatena = risultato.Offset(-1, 2).Address
disegno = risultato.Offset(-1, 3).Address
treD = risultato.Offset(-1, 4).Address
distinta = risultato.Offset(-1, 5).Address
immagine = risultato.Offset(-1, 6).Address
formulaCopy = risultato.Offset(-2, 2).Address

Range(primoValore).Value = "T0"

Range(numeroID).Value = KitNrAppoggio
Range(numeroID).Borders(xlEdgeBottom).LineStyle = XlLineStyle.xlContinuous
'Range(numeroID).Borders(xlEdgeBottom).Weight = xlHairline
Range(numeroID).Font.Bold = False
Range(numeroID).Font.Name = "Calibri"
Range(numeroID).Font.Size = 10


Range(descrizione).Font.Bold = False
Range(descrizione).Font.Name = "Calibri"
Range(descrizione).Font.Size = 10
Range(descrizione).Value = Worksheets("CODICI").TextBox1.Value
Range(descrizione).Borders(xlEdgeBottom).LineStyle = XlLineStyle.xlContinuous
'Range(descrizione).Borders(xlEdgeBottom).Weight = xlHairline


Range(gruppo).NumberFormat = "@"
Range(gruppo).Value = Worksheets("CODICI").ComboBox2.Value
Range(formulaCopy).Select
Selection.Copy
Range(concatena).PasteSpecial xlPasteFormulas
'Range(concatena).Value = "T0" + KitNrAppoggio + Worksheets("CODICI").ComboBox2.Value

Range(disegno).Value = ".dwg"
Range(treD).Value = ".asm"
Range(distinta).Value = ".xls"
Range(immagine).Value = ".jpg"

Answer 1

The comments I have fall into three categories that relate to general programming practices that would be applicable to any language, not just VBA.

1. Raw Strings
2. Don't Repeat Yourself (DRY)
3. 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.