snippetMinor
Spreadsheet to calculate how to cut pipes
Viewed 0 times
pipescutspreadsheetcalculatehow
Problem
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:
Here is my code:
And this is what my output looks like:
This is what my input sheet looks like:
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 SubAnd this is what my output looks like:
Solution
You didn't declare
Always turn on
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
Variable names - give your variables meaningful names.
Standard VBA naming conventions have
A loop like this -
Could probably just be
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.
Also, since you reuse your range, I'd add something to catch if you're accidentally overwriting data
I'd also clear out old data
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.
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 SubVariable 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 LongA loop like this -
For x = 2 To lastPipeRow
If firstPipeRow - Then
x = x - 1
End If
Next xCould probably just be
For x = lastPipeRow to 2 Step -1
NextBut, 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).RowAlso, 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 IfI'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 IfTo 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.
Code Snippets
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 SubDim initialPipeLength As Long
Dim lastPipeRow As Long
Dim totalPipes As Long
Dim pipeNumber As Long
Dim rowCounter As Long
Dim y As LongFor x = 2 To lastPipeRow
If firstPipeRow - Then
x = x - 1
End If
Next xFor x = lastPipeRow to 2 Step -1
NextlastPipeRow = ActiveSheet.Cells(Rows.Count, 1).End(xlUp).RowContext
StackExchange Code Review Q#154115, answer score: 7
Revisions (0)
No revisions yet.