Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

TypeName isn't a great way to check argument types. It literally checks the name and could produce unexpected results in certain situations It literally checks the name and could produce unexpected results in certain situations. I would recommend using TypeOf instead. The Case statement also seems superfluous to me. There's only one branch, so an If statement does the job just fine.

If TypeOf caller Is Excel.Range Then

Note that I fully quantified the type for reasons pointed out in the answer I linked to.

I'm not a huge fan of directly jumping to line labels with GoTo, but I see why you did it. However, it's more semantically correct to raise an error instead. Being performance is a concern though, I would definitely benchmark the change vs. your current code. GoTo isn't entirely evil and this is a good use of it in my opinion.

You access database.Rows(1) a couple of times, you might save a (minuscule) amount of overheard by accessing it once and storing it in a variable instead. The gain won't be much, but could potentially add up if you use this UDF in a large number of cells. Again, benchmark it.

TypeName isn't a great way to check argument types. It literally checks the name and could produce unexpected results in certain situations. I would recommend using TypeOf instead. The Case statement also seems superfluous to me. There's only one branch, so an If statement does the job just fine.

If TypeOf caller Is Excel.Range Then

Note that I fully quantified the type for reasons pointed out in the answer I linked to.

I'm not a huge fan of directly jumping to line labels with GoTo, but I see why you did it. However, it's more semantically correct to raise an error instead. Being performance is a concern though, I would definitely benchmark the change vs. your current code. GoTo isn't entirely evil and this is a good use of it in my opinion.

You access database.Rows(1) a couple of times, you might save a (minuscule) amount of overheard by accessing it once and storing it in a variable instead. The gain won't be much, but could potentially add up if you use this UDF in a large number of cells. Again, benchmark it.

TypeName isn't a great way to check argument types. It literally checks the name and could produce unexpected results in certain situations. I would recommend using TypeOf instead. The Case statement also seems superfluous to me. There's only one branch, so an If statement does the job just fine.

If TypeOf caller Is Excel.Range Then

Note that I fully quantified the type for reasons pointed out in the answer I linked to.

I'm not a huge fan of directly jumping to line labels with GoTo, but I see why you did it. However, it's more semantically correct to raise an error instead. Being performance is a concern though, I would definitely benchmark the change vs. your current code. GoTo isn't entirely evil and this is a good use of it in my opinion.

You access database.Rows(1) a couple of times, you might save a (minuscule) amount of overheard by accessing it once and storing it in a variable instead. The gain won't be much, but could potentially add up if you use this UDF in a large number of cells. Again, benchmark it.

Source Link
RubberDuck
  • 31.2k
  • 6
  • 73
  • 176

TypeName isn't a great way to check argument types. It literally checks the name and could produce unexpected results in certain situations. I would recommend using TypeOf instead. The Case statement also seems superfluous to me. There's only one branch, so an If statement does the job just fine.

If TypeOf caller Is Excel.Range Then

Note that I fully quantified the type for reasons pointed out in the answer I linked to.

I'm not a huge fan of directly jumping to line labels with GoTo, but I see why you did it. However, it's more semantically correct to raise an error instead. Being performance is a concern though, I would definitely benchmark the change vs. your current code. GoTo isn't entirely evil and this is a good use of it in my opinion.

You access database.Rows(1) a couple of times, you might save a (minuscule) amount of overheard by accessing it once and storing it in a variable instead. The gain won't be much, but could potentially add up if you use this UDF in a large number of cells. Again, benchmark it.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /