# VBA Automatic insert in a Excel Spreadsheet

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)

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)

'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(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"

• Since you are new to the site, you would be well served to read through the help center to find out how to ask a good question here. The TL;DR: <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. – FreeMan Feb 14 at 16:16
• @FreeMan thank you for answering. So basically I should comment every line of my code and write what it actually do? – ChangeWorld Feb 14 at 16:22
• No, you should just use the space above an below were you put the code to explain what it does. Also you should make sure that you only post code that works to this site. – K00lman Feb 14 at 16:54
• @ChangeWorld Downvotes are always an indicator you should go (back) to the help center and check all aspects that make up a good question and you're not failing to be eventually being off-topic at all. – πάντα ῥεῖ Feb 14 at 17:09
• @ChangeWorld Please follow K00lman's instructions. It seems I wasn't quite clear enough. You've got to give us something to work with in order to write a review for you. Otherwise, we're left guessing and we could guess wrong. Since a good review can take hours for someone to write up, it's considerate of the asker to give as much info as possible so as to not waste the reviewers time doing the wrong thing. – FreeMan Feb 14 at 17:41

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
provaFormula.FormulaArray = "=MAX(IF(LEFT(" & ProvaFormulaRange & ",1)=""" & kitNrSecondoAppoggio & """, " & ProvaFormulaRange & "))"

'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
'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

'ComboBox2 needs a meaningful name to indicate what it is providing

End Sub

Private Sub SRP_SetupPrimoValore(ByVal addr As String, ByVal value As String)
End Sub

Private Sub SRP_SetupNumeroID(ByVal addr As String, ByVal kitNrAppoggio As String)
End Sub

Private Sub SRP_SetupDescrizione(ByVal addr As String, ByVal value As String)
End Sub

Private Sub SRP_SetupGruppo(ByVal addr As String, ByVal comboBox2Value As String)
End Sub

Selection.Copy

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.