# ERP-system with UserForm that transfers data to next available row in an archive spreadsheet

I made this huge code (Edit: huge for such a simple task) for transferring data and adding an ID number to every new row of data. The only functionality flaw I see is that it creates new ID numbers if you transfer blank cells. I was wondering if anyone can help me improve this code? I'm very bad at loops so if there is any possibilities for loops that I overlooked please tell me.

Edit: also had to add another worksheet for temporarily storing the data, so I could copy it as a range, since it is first saved as a value.

Private Sub CommandButton2_Click()

'Variabler for inputs
shipFrom = TextBox1.Text
shipTo = TextBox2.Text
shipDate = TextBox3.Text
NP = TextBox4.Text
desc = TextBox10.Text
gramEx = TextBox6.Text
tareWeight = TextBox7.Text
weight = TextBox8.Text
dims = TextBox9.Text

Sheets("test").Range("b2").Value = shipFrom
Sheets("test").Range("c2").Value = shipTo
Sheets("test").Range("d2").Value = shipDate
Sheets("test").Range("e2").Value = NP
Sheets("test").Range("f2").Value = desc
Sheets("test").Range("g2").Value = gramEx
Sheets("test").Range("h2").Value = tareWeight
Sheets("test").Range("i2").Value = weight
Sheets("test").Range("j2").Value = dims

Dim sf As Range
Set sf = Sheets("test").Range("b2")

Dim st As Range
Set st = Sheets("test").Range("c2")

Dim d As Range
Set d = Sheets("test").Range("d2")

Dim l As Range
Set l = Sheets("test").Range("e2")

Dim i As Range
Set i = Sheets("test").Range("f2")

Dim g As Range
Set g = Sheets("test").Range("g2")

Dim t As Range
Set t = Sheets("test").Range("h2")

Dim w As Range
Set w = Sheets("test").Range("i2")

Dim di As Range
Set di = Sheets("test").Range("j2")

Dim nextRowB As Range
Set nextRowB = Sheets("Arkiv").Range("B" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowC As Range
Set nextRowC = Sheets("Arkiv").Range("C" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowD As Range
Set nextRowD = Sheets("Arkiv").Range("D" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowE As Range
Set nextRowE = Sheets("Arkiv").Range("E" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowF As Range
Set nextRowF = Sheets("Arkiv").Range("F" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowG As Range
Set nextRowG = Sheets("Arkiv").Range("G" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowH As Range
Set nextRowH = Sheets("Arkiv").Range("H" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowI As Range
Set nextRowI = Sheets("Arkiv").Range("I" & Rows.Count).End(xlUp).Offset(1, 0)

Dim nextRowJ As Range
Set nextRowJ = Sheets("Arkiv").Range("J" & Rows.Count).End(xlUp).Offset(1, 0)

sf.Copy nextRowB
st.Copy nextRowC
d.Copy nextRowD
l.Copy nextRowE
i.Copy nextRowF
g.Copy nextRowG
t.Copy nextRowH
w.Copy nextRowI
di.Copy nextRowJ

Dim ID As Range
Set ID = Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Offset(1, 0)

If IsEmpty(Sheets("Arkiv").Range("B2").Value) Then

MsgBox "Please fill in the <Ship From> box"

Else

'Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Offset(1, 0) = "1"
Sheets("Arkiv").Range("A2").Value = "1"

End If

If IsEmpty(Sheets("Arkiv").Range("C2").Value) Then

Else

ID = Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Value + 1

End If

Sheets("Arkiv").Activate
MsgBox "The ID number for this data is: " & Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Value

End Sub


Ouch. I say this in a kind way - the code is painful to read.

1. Use Option Explicit at the top of every module - always!
2. Properly indent your code - it makes it easier to read, easier to pick up inconsistent logic and easier to maintain.

Arrays, and Excel's ability to convert values of ranges to arrays is your friend in this instance. No need for loops.

Private Sub CommandButton2_Click()
Dim inputs as Variant 'Variabler for inputs
inputs = Array(TextBox1.Text, TextBox2.Text, TextBox3.Text, TextBox4.Text, _
TextBox10.Text, TextBox6.Text, TextBox7.Text, TextBox8.Text, _
TextBox9.Text)
' order is important for you here.
' shipFrom, shipTo, shipDate, NP, desc, gramEx, tareWeight, weight, dims

Dim nextRowB As Range
Set nextRowB = Sheets("Arkiv").Range("B" & Rows.Count & ":J"& Rows.Count).End(xlUp).Offset(1, 0)
nextRowB.Value = inputs

' Now that we put everything into the array, the individual ranges and the ".copy" is not longer required
' sf.Copy nextRowB etc <-- not needed, see line of code above!

Dim ID As Range
Set ID = Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Offset(1, 0)
If IsEmpty(Sheets("Arkiv").Range("B2").Value) Then '<-- This is your logic, but not sure if this is really what you want.
' Is it really only row 2 that you check?
MsgBox "Please fill in the <Ship From> box"
Else
'Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Offset(1, 0) = "1"
Sheets("Arkiv").Range("A2").Value = "1"
End If
If Not IsEmpty(Sheets("Arkiv").Range("C2").Value) Then ' added "Not" here, why have an empty code block?
'Else <-- why have an empty code block?
ID = Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Value + 1
End If
Sheets("Arkiv").Activate
MsgBox "The ID number for this data is: " & Sheets("Arkiv").Range("A" & Rows.Count).End(xlUp).Value
End Sub


Two other things to look at to make this more readable:

• Consider assigning "Sheets("Arkiv")" to a variable (e.g. ws) and using that throughout.
• Consider using With Sheets("Archiv") block.

And finally, ensure that all references to range objects and methods are fully qualified. This includes the Rows elements within the Range(...) code.

Edit (From Stack Overflow @AhmedAU, alternate way of inserting the values - this has been tested and works according to OP):

Dim inputs As Variant
inputs = Array(TextBox1.Text, TextBox2.Text, TextBox3.Text, TextBox4.Text, TextBox10.Text, TextBox6.Text, TextBox7.Text, TextBox8.Text, TextBox9.Text)
Dim nextRowB As Range
Set nextRowB = Sheets("Arkiv").Range("B" & Rows.Count).End(xlUp).Offset(1, 0)
nextRowB.Resize(1, UBound(inputs) + 1).Value = inputs

• Thank you!, this is what I have been looking for, I did not know how to use arrays like that since I have only learnt VBA three days ago. I'll try this now! – SanderJensen Jul 10 '19 at 5:19
• when using the inputs = array(all textboxes I get expected list separator on the textboxes that comes after the "_" sign on the row below, any idea why? – SanderJensen Jul 10 '19 at 5:35
• Ok, I did the inputs = arrays without _ all in one line, the weird thing is that it only copies the first value over, gives ID number and then stops, any idea why? I could link a picture of my userform and my worksheet "Arkiv" if you want? – SanderJensen Jul 10 '19 at 6:07
• @SanderJensen: Post a new question on Stack Overflow - along with any relevant code and a table/image of your data. In that question, describe (using an example data set) what you expect to happen, and what actually happens. What you are describing here does not match the code as written so we must dig into it a bit further. – AJD Jul 10 '19 at 20:48
• @SanderJensen: OK - see you have already done that with a successful outcome. Good! – AJD Jul 10 '19 at 22:03