# Currency Exchange exercise -- suggested naming tweaks

Hello,

I just finished the currency exchange exercise. The exercise itself is straight forward, and the explanations are excellent (seriously!), but I managed to confuse myself, and I spent a bit of time trying to understand why.

It was only after I got the last task wrong that I realized that I had spent the entire exercise assuming that the `number_of_bills` thing had to do with the original currency.

In retrospect (a) this is a totally silly assumption, and (b) the instructions clearly state that The total you receive must be divisible by the value of one “bill” or unit. (Emphasis added)

However, I was paying a lot more attention to the code than the instructions, and in the code it says:

``````def get_number_of_bills(budget, denomination):
"""

:param budget: float - the amount of money you are planning to exchange.
:param denomination: int - the value of a single bill.
:return: int - number of bills after exchanging all your money.
"""

pass
``````

The key bit that I think led me down the garden path was the name `budget` and the description `the amount of money you are planning to exchange`. To me budget definitely has the flavor of the amount of money that I’m starting out with, rather than the amount of money that I receive after making the exchange.

I can think of two ways to tweak this.

1. Make sure the two methods are clearly talking about what you have/receive after the exchange.
2. Go more generic, and not talk about which side of the exchange it’s on (since technically it works for any currency). So instead of `budget`, maybe `amount` or `funds` or `total_value`.

Here’s an example of what “more generic” could look like:

``````def get_value_of_bills(denomination, number_of_bills):
"""

:param denomination: int - the value of a bill.
:param number_of_bills: int - number of bills of that denomination.
:return: int - total value of the bills.
"""

pass

def get_number_of_bills(amount, denomination):
"""

:param amount: float - the total amount of money available.
:param denomination: int - the value of a single bill.
:return: int - number of bills that can be obtained with the available funds.
"""

pass
``````

The second thing that I was confused about was the `exchangeable_value()`. I assumed that `exchangeable` meant the amount that could be exchanged. In other words, I thought this was about the original currency rather than the target currency. After reading through the task description a few times I did figure it out, but I wonder if there’s a name that would help make it clearer that it’s about the target currency after the exchange. I can’t think of anything great off the top of my head, but maybe something like `obtainable_value`?

I’d love to hear your thoughts.

If, after discussing this the community does come up with better names, I’d be happy to submit a pull request adjusting the exercise.

Hi @MangoSeed welcome to the Exercism forums!

Thank you for your comments on `Currency Exchange` - this exercise has been a challenging one to develop for the Python track. We’ve even considered deprecating it in favor of another numbers-based challenge.

It has the lowest completion rate of any concept exercise after `Guido's Gorgeous Lasagna`, and I suspect that the wording of the exercise and explanations has been a big part of that. Many contributors have worked very hard to clarify this exercise and improve the instructions – and we clearly have room for more improvements.

Hard agree here. I think `budget` may have been chosen for some sort of consistency, but is highly confusing for these tasks, given the parameters and instructions on `task 6`.

I like your option 2: go more generic with the parameter naming and docstring (I still am mulling over whether or not we go more generic in the instruction text, and leaning toward being more specific there). Since the parameters can be named anything and not change the requirements, its not going to have an impact on already existing student solutions.

I might even go even more generic:

``````def get_value_of_bills(denomination, number_of_bills):
"""

:param denomination: int - the value of a bill.
:param number_of_bills: int - total number of bills.
:return: int - calculated value of the bills.
"""

pass

def get_number_of_bills(amount, denomination):
"""

:param amount: float - the total starting value.
:param denomination: int - the value of a single bill.
:return: int - number of bills that can be obtained from the amount.
"""

pass
``````

I think it would then follow that we would want to also rename `budget` in task 5:

``````def get_leftover_of_bills(amount, denomination):
"""

:param amount: float - the total starting value.
:param denomination: int - the value of a single bill.
:return: float - the amount that is "leftover", given the current denomination.
"""

pass
``````

Just so we have the stub for the task being discussed:

``````
"""

:param budget: float - the amount of your money you are planning to exchange.
:param exchange_rate: float - the unit value of the foreign currency.
:param spread: int - percentage that is taken as an exchange fee.
:param denomination: int - the value of a single bill.
:return: int - maximum value you can get.
"""

pass
``````

I think the parameters are correctly named. `budget` is indeed referring to the amount of money to be exchanged, and the function does indeed calculate what value you would get back from that exchange - effectively the “exchangeable value” (you might have \$127.50 in your `budget`, but you aren’t going to get more than an €80.00 value from that sleazy exchange booth). With the other parameter renames, perhaps it becomes clearer what this function is expected to do?

My concern here is that changing the function name would invalidate roughly 26, 883 passing student solutions. That is fine if we truly need to do that.

But I am not sure that swapping `obtainable` for `exchangeable` in the function name is an important enough clarification (both read equally obscure to me). Is there anything we can do with the instructions or examples that might help clarify this, or a different way of phrasing the docstring that could help?

@IsaacG (because you mentor this a lot) , @vaeng (because you’ve helped with wording here in the past), @kytrinyx (as a follow-on to your last PR) – your thoughts would be most welcome here.

The TL;DR:

I would welcome a PR for the parameters (and the associated text changes in the `introduction.md` and the `instructions.md`) – but need more discussion/convincing for the function name change.

I like the idea of making the parameter names more generic (`amount`). I think that makes the function more clear and demonstrates how you’d write a “generic” exchange-oriented function to solve a specific use case (having a budget).

I also agree that renaming the function is a big ask when it’s not obvious that a new name would make a significant difference. Would it make sense to instead change the function’s docstring to try to explain what it is supposed to be doing?

Thank you @BethanyG – I like your suggestion of going even more generic. That seems very clean and understandable.

I will submit a PR making the suggested changes.

On the second one, I agree that `obtainable` isn’t great, and especially in light of invalidating several thousand solutions, is definitely not worth doing.

Thanks so much for taking the time to read and talk through this.

I’ve just been notified that my post was flagged as spam by the community.

It says:

Your post was flagged as spam : the community feels it is an advertisement, something that is overly promotional in nature instead of being useful or relevant to the topic as expected.