VBA code feedback wanted!

    Hello everyone!


    I have made my first Addin. It calculates a price depending on the supplier price. This is my first bit of code that I have done myself and not pasted...:-). If someone more experiénced programmer could give me some feedback on this I would be very thankfull. Dont hold back! I´m a big boy I can take it!


    TIA Mats





    Function PrisI(Fpris)
    ' Net price rounded
    If Fpris < 20 Then fprisav = WorksheetFunction.Round(Fpris / 0.5, 0) * 0.5
    ElseIf Fpris < 50 Then
    fprisav = WorksheetFunction.Round(Fpris, 0)
    ElseIf Fpris < 100 Then
    fprisav = WorksheetFunction.Round(Fpris / 2, 0) * 2
    ElseIf Fpris < 200 Then
    fprisav = WorksheetFunction.Round(Fpris / 5, 0) * 5
    ElseIf Fpris >= 200 Then
    fprisav = WorksheetFunction.Round(Fpris / 10, 0) * 10
    End If
    ' The rounded price is used to calculate consumer price
    If fprisav <= 110 Then
    capris = fprisav * 2.03
    ElseIf Fpris <= 170 Then
    capris = 223.3 + (fprisav - 110) * 1.91
    ElseIf Fpris &gt; 170.01 Then
    capris = 223.3 + 114.58 + (fprisav - 170.01) * 1.81
    End If
    ' VAT is added vat 6%
    caprism = capris * 1.06
    ' Price incl VAT is rounded upwards to even krona.
    baspris = WorksheetFunction.RoundUp(caprism, 0)
    PrisI = baspris
    ' Baseprice=capriset is change to the I-scale
    If baspris < 8.49 Then
    PrisI = baspris + 3
    ElseIf baspris < 30.99 Then
    PrisI = baspris + 4
    ElseIf baspris < 53.99 Then
    PrisI = baspris + 5
    ElseIf baspris < 88.99 Then
    PrisI = baspris + 7
    ElseIf baspris >= 89 Then
    PrisI = baspris + 12
    End If


    End Function

    Hi Mats,


    OK, a few (constructive) comments:


    1. Declare the variable type of the function itself.
    2. Declare the variable type of the argument passed to the function.
    3. Consider using Select Case instead of the multiple Ifs.


    HTH

    Hej Mats,


    I agree with Richie


    In addtion to what Richie recommend here are some more ideas:


    Add on top of the module Option Explicit in that way You will be forced to declare all the variables.


    The correct syntax is:
    Application.WorksheetFunction


    For values that may be changed in the future like VAT and so on use constants in the code.


    In that way it will be much more easier to maintain the code:


    Private Const VAT = 1,06


    caprism = capris * VAT


    Finally some errorhandling:


    Konstant Felnummer Cellfelmeddelande

    xlErrDiv0 2007 #Division/0!
    xlErrNA 2042 #Saknas!
    xlErrName 2029 #Namn?
    xlErrNull 2000 #Skärning!
    xlErrNum 2036 #Ogiltigt!
    xlErrRef 2023 #Referens!
    xlErrValue 2015 #Värdefel!


    If Err.Number <> 0 Then PrisI = CVErr(xlErrNA)


    Good luck and please mail back with the updated version :thumbup:

    Thanks Richie and Dennis for your feedback!


    I will go trough with your suggestions and post it back to the board.


    As always, you´ll always have free coffee and cokies on me if you come to the lovely town of Halmstad, Sweden!


    :beergrin: Mats

    Hej Mats,


    Quote


    As always, you´ll always have free coffee and cokies on me if you come to the lovely town of Halmstad, Sweden!


    Last time it was free dinner and now only coffee and cookies :nana:


    Anyway, put the kettle on for 86 people in mid of july. We will have a family-meeting not far away You :D :D

    Hi Dennis!


    You have already earned your dinner! I promise you, you´ll be treated to a meal at a restuarant called Little Hell next time you are in Halmstad :baddevil: http://www.lillahelfwetet.se. It´s a place for XL fanatics, very hot!


    But on todays services you have earned coffee at my bookstore as well!


    :nana::mad::saint:Mats


    PS You being from the north can´t you bring some Skogsstjärnan with you?

    As you have asked for feed back – I have not read the other guy’s posts, so I might repeat the same.



    Option Explicit - I would add this


    If Fpris < 20 Then fprisav = WorksheetFunction.Round(Fpris / 0.5, 0) * 0.5


    This might be better as Application.worksheetfunction.function


    Replace the last function for your requirements


    - Same here
    If Fpris < 20 Then fprisav = WorksheetFunction.Round(Fpris / 0.5, 0) * 0.5



    I might look at declaring variables as you have Option Explicit, which will force you to do so, it’s a better way to program.


    //


    I would re reading your code seriously look at spacing your code out a bit, so its nice and easy to read, is line spaces.


    And lastly looks like rather that all the ifs, I might opt for SELECT CASE approach


    Somme thought, and please don’t take my comments as negative, I do the same thing let gusy read my work and pick it to bits. It’s a fast solid way to learn.


    Hope that helps a little.



    Kindest possible regards


    Jack in the UK

Participate now!

Don’t have an account yet? Register yourself now and be a part of our community!

Register Yourself Login