I have written a piece of code that calculates how to cut pipes based on an order and fixed raw pipe length (in this case 288 feet), but I am not 100% happy with my current code as it counts the pipe pieces that have been created for each length in column C (see comment in code) and clears column C at the end. Is there an way to store the values without having to create a variable for each pipe length?

This is what my input sheet looks like:

enter image description here

Here is my code:

Sub CalcPipe()

Dim StartPipeLng As Long, LastLng As Long, TotalLng As Long, PipeName As Long

StartPipeLng = Cells(2, 4).Value

LastLng = ActiveSheet.UsedRange.Rows(ActiveSheet.UsedRange.Rows.Count).Row

TotalLng = Application.WorksheetFunction.SumProduct(Range("A2:A" & LastLng), Range("B2:B" & LastLng))
TotalLng = Application.WorksheetFunction.RoundUp(TotalLng / StartPipeLng, 0)

Cells(5, 4).Value = TotalLng

PipeName = 1

    For y = 6 To TotalLng + 5

        Cells(1, y).Value = "Pipe " & PipeName
        PipeName = PipeName + 1
        StartPipeLng = Cells(2, 4).Value

        For x = 2 To LastLng
            If StartPipeLng - Cells(x, 1).Value >= 0 And Cells(x, 2).Value <> Cells(x, 3).Value Then
                StartPipeLng = StartPipeLng - Cells(x, 1).Value
                Cells(x, 3).Value = Cells(x, 3).Value + 1 ' Here is the issue: Rather than write the values in column C, I would like to store them rather then write them in a cell
                Cells(x, y).Value = Cells(x, y).Value + 1
                x = x - 1
            End If
        Next x
    Next y

Columns(3).Clear

End Sub

And this is what my output looks like:

enter image description here

up vote 5 down vote accepted

You didn't declare y or x.

Always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

When you don't define your variable, VBA will declare it as a Variant type that can hold any type of data. While this may be more flexible, it adds processing time to your macro as VBA decides or tests for the type.

Additionally, since a Variant can be any type of data, you may miss out on valuable troubleshooting information on Type Mismatch

By not declaring variables, you could possibly be paying a penalty.


It's good practice to indent all of your code that way Labels will stick out as obvious. You also have a ton of unneeded white space. To begin, it might look like this:

Option Explicit

Sub CalcPipe()
    Dim StartPipeLng As Long, LastLng As Long, TotalLng As Long, PipeName As Long
    StartPipeLng = Cells(2, 4).Value
    LastLng = ActiveSheet.UsedRange.Rows(ActiveSheet.UsedRange.Rows.Count).Row
    TotalLng = Application.WorksheetFunction.SumProduct(Range("A2:A" & LastLng), Range("B2:B" & LastLng))
    TotalLng = Application.WorksheetFunction.RoundUp(TotalLng / StartPipeLng, 0)
    Cells(5, 4).Value = TotalLng
    PipeName = 1
    Dim y As Long
    Dim x As Long
        For y = 6 To TotalLng + 5
            Cells(1, y).Value = "Pipe " & PipeName
            PipeName = PipeName + 1
            StartPipeLng = Cells(2, 4).Value
            For x = 2 To LastLng
                If StartPipeLng - Cells(x, 1).Value >= 0 And Cells(x, 2).Value <> Cells(x, 3).Value Then
                    StartPipeLng = StartPipeLng - Cells(x, 1).Value
                    Cells(x, 3).Value = Cells(x, 3).Value + 1
                    Cells(x, y).Value = Cells(x, y).Value + 1
                    x = x - 1
                End If
            Next x
        Next y
    Columns(3).Clear
End Sub

Variable names - give your variables meaningful names. Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names.

Dim initialPipeLength As Long
Dim lastPipeRow As Long
Dim totalPipes As Long
Dim pipeNumber As Long
Dim rowCounter As Long
Dim y As Long

A loop like this -

For x = 2 To lastPipeRow
    If firstPipeRow -  Then
        x = x - 1
    End If
 Next x

Could probably just be

For x = lastPipeRow to 2 Step -1
Next

But, since it's not a standard loop and that won't work, you should use variable names to make it more obvious what is happening there.


There is a standard way to find lastRow and lastColumn. That post explains why.

lastPipeRow = ActiveSheet.Cells(Rows.Count, 1).End(xlUp).Row

Also, since you reuse your range, I'd add something to catch if you're accidentally overwriting data

Dim result As VbMsgBoxResult
If Not IsEmpty(ActiveSheet.Cells(1, 6)) Then
    result = MsgBox("There is still data on this sheet, do you want to overwrite it?", VbMsgBoxStyle:=vbYesNo)
    If result = vbNo Then Exit Sub
End If

I'd also clear out old data

If Not IsEmpty(ActiveSheet.Cells(1, 6)) Then
    result = MsgBox("There is still data on this sheet, do you want to overwrite it?", vbYesNo)
    If result = vbNo Then Exit Sub
    Dim lastColumn As Long
    lastColumn = ActiveSheet.Cells(1, Columns.Count).End(xlToLeft).Column
    ActiveSheet.Range(Columns(6), Columns(lastColumn)).Clear
End If

To avoid using column 3, use an array. But, I'd say since you want to use an array, you could optimize your solution to the cutting stock problem with the Greedy algorithm.

  • 1
    :+1:, except a Variant is not an Object. A Variant can be anything including an object, but it's different. An Object variable can only contain an object reference; a Variant can contain anything, including types that aren't even known to the VBA runtime. – Mathieu Guindon Feb 3 '17 at 19:54
  • @Mat'sMug I gave it another try, thanks. – Raystafarian Feb 3 '17 at 20:02
  • @Raystafarian Thanks so much for taking the time to provide me with your feedback! One thing in regards to the loop, how would For x = lastPipeRow to 2 Step -1 replace For x = 2 To lastPipeRow & 'x = x - 1'? The first loop only loops backwards, while 'x=x-1` is supposed to prevent the loop to move to the next value if the IF statement is valid. – VBA Pete Feb 3 '17 at 20:19
  • 1
    @VBAPete the step-1 won't work for this problem, but when you usually see your structure, you assume it would. – Raystafarian Feb 3 '17 at 20:24
  • 1
    @VBAPete I became curious on this problem and wrote some code that does something along the same lines as yours - codereview.stackexchange.com/questions/154618/… – Raystafarian Feb 6 '17 at 19:15

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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