VBA: Can someone tell me if this code is correct?

[Hip]

Hey guys,

 

can someone tell me if this code is correct?

I have two quality types and want to calculate the prices. If someone picks the product up at warehouse the price is 5% less.

Option Explicit
Function calc(quality As String, product1 As Double, product2 As Double, pickUp As String, costs As Double) As Double
    Const PRICE_TYPE1 As Double = 10
    Const PRICE_TYPE2 As Double = 20
    Const COST_PICKUP As Double = 0.95
    
    Dim qualityPrice As Double
    Dim price As Double
    Dim product1Price As Double
    Dim product2Price As Double
    
    If quality = "TYPE1" Then
        qualityPrice = PRICE_TYPE1
    Else quality = "TYPE2" Then
        qualityPrice = PRICE_TYPE2
    End If
    
    product1Price = product1 * qualityPrice
    product2Price = product2 * qualityPrice
    price = product1Price + product2Price * 0.95
    
    If pickUp = "YES" Then
        price = price + price * product2
    End If
    
    calc = price
End Function

 

Thank you in advance!

[mariushm]

Seems OK, but pay attention to order of operations.

 

edit : also the pickup  part doesn't seem right, as the guy below me says

 

Multiplication and divisoon has higher importance than addition and substraction so it's done first

 

So in the line below, the program first does product2Price x 0.95  then adds product1Price to the result and puts the total into price variable. if you want to add both prices then multiply everything with 0.95,  use (  )   to force  addition first :

 

price = product1Price + product2Price * 0.95

[Franck]

No that is not correct

 

price = product1Price + product2Price * 0.95

should only be the total. Your 0.95 you said is for the warehouse only so it should be :

 

price = product1Price + product2Price

 

Also the pickup code is then wrong and should be replaced with :

 If pickUp = "YES" Then
        price = price * COST_PICKUP 
 End If

 

And finally you have no error checking whatsoever. If someone pass the quality "abc" your code multiply by zero as you do not have a fallback conditions or error checking for bad input. Also i would recommend using boolean type for pickUp variable.

 

[Hip]

1 hour ago, Franck said:

No that is not correct

 

price = product1Price + product2Price * 0.95

should only be the total. Your 0.95 you said is for the warehouse only so it should be :

 

price = product1Price + product2Price

 

Also the pickup code is then wrong and should be replaced with :

 If pickUp = "YES" Then
        price = price * COST_PICKUP 
 End If

 

And finally you have no error checking whatsoever. If someone pass the quality "abc" your code multiply by zero as you do not have a fallback conditions or error checking for bad input. Also i would recommend using boolean type for pickUp variable.

 

Option Explicit
Function calc(quality As String, product1 As Double, product2 As Double, pickUp As String, costs As Double) As Double
    Const PRICE_TYPE1 As Double = 10
    Const PRICE_TYPE2 As Double = 20
    Const COST_PICKUP As Double = 0.95
    
    Dim qualityPrice As Double
    Dim price As Double
    Dim product1Price As Double
    Dim product2Price As Double
    
    If quality = "TYPE1" Then
        qualityPrice = PRICE_TYPE1
    Else quality = "TYPE2" Then
        qualityPrice = PRICE_TYPE2
    End If
    
    product1Price = product1 * qualityPrice
    product2Price = product2 * qualityPrice
    price = product1Price + product2Price * 0.95
    
    If pickUp = "YES" Then
        price = price * COST_PICKUP 
    End If
    
    calc = price
End Function

 

I think I have said it wrong. The Discount should only apply to product 2

[Franck]

So the end should be :

 

product1Price = product1 * qualityPrice
product2Price = product2 * qualityPrice
price = product1Price
    
    If pickUp = "YES" Then
        price = price + (product2Price * COST_PICKUP)
    Else
        price = price  + product2Price 
    End If

 

[darkDOS]

Pre-face: I don't really know VBA but just some thoughts.

 

Is there a requirement for the function to to return the price of two items?

It seems cumbersome for reasons like,

Is the quality of the two items always the same?

Are you always picking up both items? Picking up one item and not the other? etc.

 

I would think it would be easier to re-write the function for a single item, then add the return values outside the function.

 

Also, you send costs as a parameter, then never use it.

And define the cost for pickup as a constant, then use a hardcoded number.

 

Might not be exact, but I would personally prefer something like this then calling the function on a per item basis.

Option Explicit
Function calc(quality As String, product As Double,  pickUp As String) As Double
    Const PRICE_TYPE1 As Double = 10
    Const PRICE_TYPE2 As Double = 20
    Const COST_PICKUP As Double = 0.95
    
    If (quality = "TYPE1") And (pickUP = "YES") Then
        calc = (product * PRICE_TYPE1) * COST_PICKUP
    Elseif (quality = "TYPE1") And (pickUP = "No") Then
        calc = (product * PRICE_TYPE1)
    Elseif (quality = "TYPE2") And (pickUP = "YES") Then
        calc = (product * PRICE_TYPE2) * COST_PICKUP
    Elseif (quality = "TYPE2") And (pickUP = "No") Then
        calc = (product * PRICE_TYPE2)
    End If

End Function