0
\$\begingroup\$

This code checks if "B2" and so on is filled in and if it is then it will generate a new ID number for the row of data that was transferred to the worksheet on the next empty row, I know this code creates opportunities for bugs so I want to make it better but I don't really know how I could do it. (PS: I am very bad at loops so if the solution is a loop please word yourself as if you were talking to a newcomer

'Creating a variable for all the inputs from the textboxes
Dim inputs As Variant
    inputs = Array(TextBox1.Text, TextBox2.Text, TextBox3.Text, TextBox4.Text, TextBox10.Text, TextBox6.Text, TextBox7.Text, TextBox8.Text, TextBox9.Text)
    ' shipFrom, shipTo, shipDate, NP, desc, gramEx, tareWeight, weight, dims

'Declaring a variable for the next available row where the inputs are going
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


'Delaring ID number for each row as a variable
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 'How can I make this line better? I know this way of doing it creates oppertunities for many bugs

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

    Else


        Sheets("Arkiv").Range("A2").Value = "1"

    End If

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

        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
\$\endgroup\$
  • 2
    \$\begingroup\$ What's using these ID values, and what happens when a row is deleted? \$\endgroup\$ – Mathieu Guindon Jul 10 at 14:32
  • 3
    \$\begingroup\$ Step 1) Update your form and rename your text boxen. There's no reason to have a comment that TextBox1 = shipFrom when you could simply rename TextBox1 to shipFrom. When someone accidentally deletes that comment line, you and everyone who comes after you will thank you for taking the 5 minutes to do that rename! \$\endgroup\$ – FreeMan Jul 10 at 14:35
  • \$\begingroup\$ Could you use the row number as your index, or if you have a header, the row number - 1? \$\endgroup\$ – Ryan Wildry Jul 11 at 2:10
3
\$\begingroup\$

Creating an Unique ID Without Loops

The problem with incrementing the next ID based on the last cell in a column is you will have to sort the data to ensure that it is in ID order. This really can take away from the user experience. A better approach would be to use the WorksheetFunction.Max() to find the current max ID.

"I am very bad at loops"

You should start by watching these videos:

Notice that the first video teaches you how to define ranges. Well, If you are having problems with loops in Excel then chances are you are having problems defining the range to loop over.

The best advice that I can give you when you are having problems with a coding pattern is to practice it. Printout a worksheet with several different types of loops and every morning choose a couple of the patterns and type them up several times until you can recite them from memory. Another great way to practice is to start answering questions on Stackoverflow.com. This will challenge you with unique situations that you would not normally see. Your answers will get progressively better or time as you respond to the comments from other users.

Miscellaneous

TextBox1, TextBox2 TextBox3 ... ugh!! In the time it takes you to the write comments to describe what each one does, you could have just gave them meaningful names. I generally preface all my textboxes with txt like this txtShipFrom, txtShipTo, txtShipDate but simply using shipFrom, shipTo, shipDate also works.

Sheets("Arkiv") is used 6 times. Using With Sheets("Arkiv") will not only make you code easier to read, modify, and debug but is also more efficient (the compiler only has to resolve the reference 1 time).

Refactored code:

Sub AddNewShippingRow_Click()
    Dim inputs As Variant
    Dim ID As Long
    Dim nextRow As Range

    ID = getNextShippingID
    Set nextRow = getNextShippingRow

    inputs = Array(ID, shipFrom.Text, shipTo.Text, shipDate.Text, NP.Text, desc.Text, gramEx.Text, tareWeight.Text, Weight.Text, dims.Text)

    nextRow.Resize(UBound(inputs) + 1).Value = inputs
End Sub

Function getNextShippingRow() As Range
    With Sheets("Arkiv")
        Set getNextShippingRow = .Cells(.Rows.Count, 1).End(xlUp).Offset(1)
    End With
End Function

Function getNextShippingID() As Long
    With Sheets("Arkiv")
        getNextShippingID = WorksheetFunction.Max(.Range("A1", .Cells(.Rows.Count, 1).End(xlUp))) + 1
    End With
End Function

In my refactored code I simplified the main subroutine by using helper functions to perform some of the tasks. The fewer tasks that a subroutine performs the easier it is to read, modify and debug.

For instance say that you wanted to change the way you create your ID's because we are using a helper function we can modify and test that function without have to run the larger block of code.

Here I modify and tested 4 different versions getNextShippingID() without having to make any changes to the main routine. Of course, I would have to change the datatype of the main routines ID variable but that is not to after I completed my testing.

Function getNextShippingID() As Double
    getNextShippingID = CLng(Date) & (Timer * 100)
End Function

Function getNextShippingID() As Double
    Const StartDate As Date = #1/1/2019#
    getNextShippingID = CLng(Date - StartDate) & (Timer * 100)
End Function

Function getNextShippingID() As Double
    getNextShippingID = Format(Now, "ddmmyyhhmmssnn")
End Function

Function getNextShippingID() As String
    Dim s As String
    s = CreateObject("Scriptlet.TypeLib").GUID
    getNextShippingID = Mid(s, 2, Len(s) - 4)
End Function
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! Great response, I have been wanting to find some good videos to teach myself loops because I notice how often I feel like a loop would be possible if I understood them an knew the different ways to do them. By the way, I have never programmed before and this is like 4 days into my first language, is this something I should be proud over or am I hanging far behind? \$\endgroup\$ – SanderJensen Jul 11 at 5:27
  • \$\begingroup\$ @SanderJensen you are doing great! \$\endgroup\$ – TinMan Jul 11 at 12:39

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.