3
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ 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! \$\endgroup\$ – SanderJensen Jul 10 at 5:19
  • \$\begingroup\$ 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? \$\endgroup\$ – SanderJensen Jul 10 at 5:35
  • \$\begingroup\$ 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? \$\endgroup\$ – SanderJensen Jul 10 at 6:07
  • \$\begingroup\$ @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. \$\endgroup\$ – AJD Jul 10 at 20:48
  • \$\begingroup\$ @SanderJensen: OK - see you have already done that with a successful outcome. Good! \$\endgroup\$ – AJD Jul 10 at 22:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.