Return to Answer

• 1

• 263
• 1
• 6

• 263
• 1
• 6

Just Evaluating the Script, Not the Application of it.

Indentation

I see you have used some indentation in your Public Function cartMaxCalc(i As Long), and pretty good at that, but not the rest of the Subs or Functions. Always make sure that you indent your lines referring to With, For, etc statements.

Variable Naming

This is the first place anyone will comment/review (as I am doing). It’s always good to give descriptive names. I know it’s so easy and quick to use For i / j / k to ..... but really just adding a few extra characters never hurt anyone, you also have some good descriptive names cartMaxCalc so you can do it, just need to keep doing it. It will also help the next person in 5 years time.

If I had to redo your Main Sub, and I am but no means the best.

Sub PopulatingColumnOWithCartMaxData()
 
 Application.ScreenUpdating = False
 
 'Write Column Header
 Range("O1").Value = "CART_MAX"
 
 Dim FinalRow As Integer
 Dim RowReference As Integer
 
 'Loop to get cells with values for calculations
 FinalRow = ThisWorkbook.Sheets("Sheet1").Cells(Rows.Count, 1).End(xlUp).Row
 For RowReference = 2 To FinalRow
 
 'Output results to cells and columns
 ThisWorkbook.Sheets("Sheet1").Cells(RowReference, 15).Value = cartMaxCalc(RowReference)
 
 Next
 
 Application.ScreenUpdating = True
 
End Sub

In the above notice the following:

Descriptive Sub Title

Moving the Column Title Insertion to above the For Loop, as it has nothing to do with the Loop.

Adding the WorkBook and WorkSheet reference to the the various places where Cells(....) is used. If there if another WorkBook was active or another sheet then you could get wrong data.

Using RowReference instead of i. Once a variables has been Dimed then its in the database so then just CTRL + SPACE and start typing and it will appear quickly.

Spacing between ideas in your Sub. Try and have an empty line between parts of your code which then sort of separates the ideas in your Sub.

I also used Integer instead of Long, Integer just doesn’t have a decimal place. More of a preference really.

Also ThunderFrame has some great data here which provides the automatic restoration of the ScreenUpdating and others. If your script bombs out and your have not restored the updating then the user will not know what to do.

In the Public Function cartMaxCalc(i As Long)

There are a few Variables which haves been Dim but not used.

When you assigning Values to contQty and contQty you didn’t set the Data Type (Range, String, Long). Also, only if you want to use a Range must you use Set. I would have done the following:

Dim contQty as Double ‘Double has decimal places
contQty = ThisWorkbook.Sheets("Sheet1").Cells(i, 6).Value
Dim contTypeRange
contTypeRange = ThisWorkbook.Sheets("Sheet1").Cells(i, 5).Value

Also change the i to something like CurrentRowReference to be more descriptive. Once in the Function is possible, at first glance, to not see what the i is referring to. Always remember your WorkBook and WorkSheet Reference. If you using the WorkSheet Reference more than twice then I would recommend Setting it up as a variable.

In Sub minFinder()

Sub Title is not bad, can just be ..................more descriptive. Dont worry, it has taken me a while to come out of my Non-descriptive ways.

Remember your WorkBook and WorkSheet reference when setting a Range.

Fairly good use of indents.

You did not set the Data Type for cartmax.

Also I am struggling to see what you trying to do with the line:

cartmax = Evaluate("=MIN(IF( " & cel.Address(0, 0) & "=" & rng.Address(0, 0) & "," & minRng.Address(0, 0) & "))")

Firstly, if you want to use the worksheet functions, just use WorksheetFunction. So for your case where you want to use Min then use WorksheetFunction.Min.