HiveBrain v1.2.0
Get Started
← Back to all entries
snippetMinor

Spreadsheet to calculate how to cut pipes

Submitted by: @import:stackexchange-codereview··
0
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:

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:

Solution

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.

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 Sub
Dim initialPipeLength As Long
Dim lastPipeRow As Long
Dim totalPipes As Long
Dim pipeNumber As Long
Dim rowCounter As Long
Dim y As Long
For x = 2 To lastPipeRow
    If firstPipeRow -  Then
        x = x - 1
    End If
 Next x
For x = lastPipeRow to 2 Step -1
Next
lastPipeRow = ActiveSheet.Cells(Rows.Count, 1).End(xlUp).Row

Context

StackExchange Code Review Q#154115, answer score: 7

Revisions (0)

No revisions yet.