I've replaced the following horrendous Excel formula (values and names have been replaced):
=IF(AND(SourceTable[@Field1]="Value1",SourceTable[@Field2]="Value2"),"Result1",
IF(SourceTable[@Field2]="Value3","Result2",
IF(AND(SourceTable[@Field1]="Value4",SourceTable[@Field3]="Value5"),"Result3",
IF(AND(SourceTable[@Field1]="Value6",OR(SourceTable[@Field3]="Value7",SourceTable[@Field3]="Value8")),"Value9",
VLOOKUP(SourceTable[@Field1];MatchingTable;2;0)))))
With a VBA Function so it would be easier to read and supposedly faster:
Function CUSTOMFUNCTION(oRngField1 As Range, oRngField2 As Range, oRngField3 As Range) As String
'If imbrication
If (oRngField1 = "Value1" And oRngField2 = "Value2") Then
CUSTOMFUNCTION = "Result1"
Exit Function
ElseIf oRngField2 = "Value3" Then
CUSTOMFUNCTION = "Result2"
Exit Function
ElseIf oRngField1 = "Value4" Then
If oRngField3 = "Value5" Then
CUSTOMFUNCTION = "Result3"
Exit Function
ElseIf (oRngField3 = "Value6" Or oRngField3 = "Value7") Then
CUSTOMFUNCTION = "Result4"
Exit Function
End If
End If
'Vlookup if no match before
Dim sTableMatchingFieldLookup As String
Dim sTableMatchingFieldResult As String
sTableMatchingFieldLookup = "MatchingTable[Field1]"
sTableMatchingFieldResult = "MatchingTable[Field2]"
CUSTOMFUNCTION = Range(sTableMatchingFieldResult)(Application.Match(oRngField1, Range(sTableMatchingFieldLookup), 0))
End Function
I get the same result as the function however it does it two times longer than the formula when working with 2k rows. Why isn't the code faster than the Excel formula? Can it be improved? The file is expected to grow so it means the code will be of no use.
Edit: Using more arguments improved the macro, see my response below.
-
4\$\begingroup\$ Why replace the names? It takes away meaning, which makes the code pretty hard to read and ultimately to review. \$\endgroup\$Mathieu Guindon– Mathieu Guindon2014年07月03日 16:59:21 +00:00Commented Jul 3, 2014 at 16:59
-
3\$\begingroup\$ inbuild excel function are almost always faster than UDFs because Communication between Vba and Excel incurs a significant overhead. \$\endgroup\$Siphor– Siphor2014年07月03日 19:51:28 +00:00Commented Jul 3, 2014 at 19:51
-
3\$\begingroup\$ Calling VBA from Excel is pretty fast...as long as you don't use any of Excel's features. Don't use any worksheet functions (use the VBA equivalents); don't use any Range objects. If you really need a speedup, build an XLL. \$\endgroup\$Snowbody– Snowbody2014年07月04日 02:12:14 +00:00Commented Jul 4, 2014 at 2:12
-
\$\begingroup\$ @Mat'sMug I know sorry about that, my company is a little paranoid about everything but I can ensure you it has meaning on the code. I will take time to use fake names instead for better comprehension. \$\endgroup\$Kersijus– Kersijus2014年07月04日 07:10:43 +00:00Commented Jul 4, 2014 at 7:10
2 Answers 2
I'm assuming that names have been changed to protect the innocent, but just in case they've not been...
These are terrible names. All of them.
- Function names should have verb-noun names and be
PascalCased
. So instead ofCUSTOMFUNCTION
should beGetCustomMatch
(or something like that. I can't say because you've ommitted any actual names...) Parameters should be
camelCase
and tell me what kind of data should be passed in and NEVER should they be numbered the way this is. So, maybe this instead (again, I have to guess because I have no clue):Function GetCustomMatch(LookupRange As Range, lastYearsSalesRange as Range, iJustDontKnowWhatThisIsRange as Range) As String
Don't use hungarian notation. Just... please don't. It should be killed with fire and buried out back.
kill it with fire
Other Stuff
- Some of your "Values" are used in multiple places. Store them in a constant and use that constant anywhere they show up. (Of course, use meaningful names for them.)
- I have a feeling that
sTableMatchingFieldLookup
and...Result
should be actual ranges instead of strings. String typing is bad. If you insist on keeping them as strings, store the values as constants. - You should be able to pick up some performance by setting
Application.Calculation = xlCalculationManual
. This Stack Overflow answer has a pretty good example of what you're trying to do.
Things you did right
- You exit early if you've already found a match before performing the lookup rather than nesting
if
statements.
-
8\$\begingroup\$ Hungarian notation doesn't even deserve a proper burial. \$\endgroup\$BeetDemGuise– BeetDemGuise2014年07月03日 17:43:29 +00:00Commented Jul 3, 2014 at 17:43
-
1\$\begingroup\$ I'm sorry if this came off kind of harsh. We don't get many VBA questions, so I always get excited when I see one come through. It's just very difficult to give a proper review when all of the names have been basically obfuscated "to protect the innocent". \$\endgroup\$RubberDuck– RubberDuck2014年07月03日 18:24:56 +00:00Commented Jul 3, 2014 at 18:24
-
4\$\begingroup\$ You're welcome. It's ok to change names for the reason you did, but you should try to make them reasonable. It helps us help you. Your teacher taught you to use Hungarian notation either because that's what he was taught or because he came from a world of untyped (or perhaps weakly typed) languages. I highly recommend that you read this article by Joel Spolsky. If you don't know of Joel, he's largely responsible for the creation of Excel & VBA (as we know it) and Stack Overflow. \$\endgroup\$RubberDuck– RubberDuck2014年07月04日 11:00:08 +00:00Commented Jul 4, 2014 at 11:00
-
3\$\begingroup\$ The short story is that Hungarian notation isn't wrong. The way you (and most people) used it is. \$\endgroup\$RubberDuck– RubberDuck2014年07月04日 11:01:46 +00:00Commented Jul 4, 2014 at 11:01
-
1\$\begingroup\$ ++, haha im loving this answer \$\endgroup\$user28366– user283662014年07月07日 10:37:05 +00:00Commented Jul 7, 2014 at 10:37
Sorry for answering late. I had to change the function due to other macros that glitched and got great improvement in terms of speed through the following code:
Function GetFruitType(colProductTypeRange As Range, colProductNameRange As Range, currentProductNameRange As Range, _
currentCodeRange As Range, currentProductDetailRange As Range) As String
'Tests for the exceptions
If (currentProductNameRange = "BEETROT" And currentProductDetailRange = "SUGAR") Then
GetFruitType = "SUGAR"
Exit Function
ElseIf currentProductDetailRange = "TREE" Then
GetFruitType = "PLANT"
Exit Function
ElseIf currentProductNameRange = "TOMATO" Then
If currentCodeRange = "ADORATION" Then
GetFruitType = "FRUIT"
Exit Function
ElseIf (currentCodeRange = "ALICANTE" Or currentCodeRange = "BEEFSTEAK") Then
GetFruitType = "VEGETABLE"
Exit Function
End If
End If
'Mathing table if no exception found
GetFruitType = colProductTypeRange(Application.Match(currentProductNameRange.Value, colProductNameRange, 0))
End Function
I thought the more arguments I needed the slower it would be but it's actually quite the opposite. Now my function is way faster than the original Excel Formula. Thanks to all that helped, comment if my variable notation is wrong.
-
1\$\begingroup\$ two comments: 1) it's generally best practice to include the final result in the original question, to help future seekers find the result. and 2) you're turning off Screen Update while the macro is running, right? I'm not sure if it's necessary in this code, but that's trick-zero for any fast-and-many calculations in VBA codes. \$\endgroup\$Gürkan Çetin– Gürkan Çetin2015年11月29日 19:44:24 +00:00Commented Nov 29, 2015 at 19:44