0

I built a calculator for calculating the sales commission on a certain sale.

The value of the commission is depended on a lot of factors. Therefore the parameters to have to be passed to the function in which I calculate the commission is very large too. The function itself is also very confusing, because some of the factors are booleans (new customer, f.e.) and some are floats (transport costs, f.e.).

How can I make these kind of "calculation functions" more readable? Should I break them up in smaller pieces?

Here is the function, although I am looking also for more general advice how to tackle these kind of functions. 

def get_provision_sales(
    verkaufspreis_hw,
    einkaufspreis_hw,
    vk_software,
    ek_software,
    bonafide,
    laufzeit,
    abloese,
    gebraucht,
    neukunde,
    spedition,
    sonstige_kosten,
    provision_pct = 0.33,
    service_pct=0.035,
    solutions=False ):

    if spedition:
        spedition_amount = 89.99
    else:
        spedition_amount = 0

    verkaufspreis_hw = verkaufspreis_hw or 0

    if vk_software > 0:
        software_mod = 0.7
    else:
        software_mod = 1    

    if not bonafide:
        provision_modifier = 0.76
    else:
        provision_modifier = 1

    if neukunde:
        provision_pct = 0.27

    laufzeit = laufzeit or 0
    abloese = abloese or 0

    if gebraucht:
        db_modifier = 1
        sonstige_kosten = 0
        deckungsbeitrag = 0.6 * float(verkaufspreis_hw)
    else:
        db_modifier = 0.85
        deckungsbeitrag = float(verkaufspreis_hw) - ( float(einkaufspreis_hw) + float(abloese) + float(spedition_amount) )

    provision_sales = (deckungsbeitrag * db_modifier + float(sonstige_kosten)) * provision_pct * provision_modifier

    provision_sales += software_mod*0.15*(float(vk_software)-float(ek_software))

    return provision_sales
Дмитрий Шулицкий
  • 566
  • 1
  • 4
  • 17
Jelle van der Zwaag
  • 117
  • 1
  • 4
  • You could start by eliminating unused function parameters and useless statements like `abloese = abloese or 0`. Using `float()` all the time also seems unnecessary. In my opinion it also makes the code less readable if german variable names are mixed with english language keywords. – mkrieger1 Aug 06 '18 at 10:23
  • @mkrieger1 Thanks for the comment. The useless statements are there, because the variable could also be None. Should solve that case diffferently probably. And I use float(...), to force the variable into a float. Not very pretty. – Jelle van der Zwaag Aug 06 '18 at 10:32
  • This is probably more suitable for code review stackexchange; you should probably translate the variables into English: some of them could obviously be grouped in classes, but not understanding what they are precisely makes it more difficult. – Reblochon Masque Aug 06 '18 at 11:35
  • That should absolutely be split up into multiple functions. Try and see what happens if you give every calculation it's own function (a calculation simple enough that it can be described in at most 3 words and fit in return statement) and let the top function handle the final thing. – Mast Aug 06 '18 at 11:41

1 Answers1

0

You could break it up, though that might not be necessary. That depends partly on the meaning of the code whether it makes sense.

Otherwise, you could add documentation, esp docstrings, and even use type hinting if you really wanted to go wild

joel
  • 6,359
  • 2
  • 30
  • 55