Repository: exercism/python (python)

Issue:
[Currency Exchange] Inconsistent change value type (`float`/`int`)
Exchanged value is of type `float`:

https://github.com/exercism/python/blob/48e47c1f6af8050e135747461085d713a3231164/exercises/concept/currency-exchange/exchange.py#L6

Remaining value is of type `float` as well:

https://github.com/exercism/python/blob/48e47c1f6af8050e135747461085d713a3231164/exercises/concept/currency-exchange/exchange.py#L17

Value exchangeable in bills of a certain denomination is of type `int`, but that is justifiable because denominations are integers:

https://github.com/exercism/python/blob/48e47c1f6af8050e135747461085d713a3231164/exercises/concept/currency-exchange/exchange.py#L52

However, non-exchangeable value is of type `int` as well, which I believe is inconsistent:

https://github.com/exercism/python/blob/48e47c1f6af8050e135747461085d713a3231164/exercises/concept/currency-exchange/exchange.py#L65
 
Given the established structure, I see no reason why the non-exchangeable value should be integer. I mean, it clearly is not integer, but has to be rounded artificially to pass the tests.

---

Overall, I was surprised how convoluted this seemingly simple exercise was.

For one, there are few opportunities to reuse code in `exchangeable_value` (task 5) and `non_exchangeable_value` (task 6).
The essence of both of these functions is to calculate the exchangeable amount given the spread, which could've been implemented in `exchange_money` (task 1).
Then `exchangeable_value` could potentially reuse `get_value_of_bills` (task 3), whereas `non_exchangeable_value` could potentially reuse `get_change` (task 2) -- except for the fact that it expects an integer, therefore this issue.

As a side note, if we're really looking to encourage the usage of the modulo operator `%`, we could introduce a task to convert amounts using different denominations (and e.g. calculate the remainder that cannot be converted).

Happy to discuss & potentially submit a PR.

Pull request:
[Currency Exchange] Improve the `int`/`float` usage consistency
Fixes #3157 and is somewhat discussed there.

Basically, what I ended up doing is:
- Added a new task asking to implement `get_leftover_of_bills`, which encourages using modulo operator `%`.
- Removed task 6, which was a weird semi-duplicate of task 5 (and also potentially hinted at using `%`).

Also, along the way:
- Tried to make `int` and `float` usage more intuitive.
- Used `assertAlmostEqual` method to compare floating point values in tests.
- Modified some test values to improve the variety of cases being tested.

Patch:
diff --git a/exercises/concept/currency-exchange/.docs/hints.md b/exercises/concept/currency-exchange/.docs/hints.md
index 95906ef1..b54f0d34 100644
--- a/exercises/concept/currency-exchange/.docs/hints.md
+++ b/exercises/concept/currency-exchange/.docs/hints.md
@@ -24,7 +24,12 @@
 
   **Note:** The `//` operator also does floor division. But, if the operand has `float`, the result is still `float`.
 
-## 5. Calculate value after exchange
+## 5. Calculate leftover after exchanging into bills
+
+- You need to find the remainder of `budget` that does not equal a whole `denomination`.
+- The Modulo operator `%` can help find the remainder.
+
+## 6. Calculate value after exchange
 
 - You need to calculate `spread` percent of `exchange_rate` using multiplication operator and add it to `exchange_rate` to get the exchanged currency.
 - The actual rate needs to be computed. Remember to add exchange rate and exchange fee.
diff --git a/exercises/concept/currency-exchange/.docs/instructions.md b/exercises/concept/currency-exchange/.docs/instructions.md
index 8e41ec0c..03f7e6a1 100644
--- a/exercises/concept/currency-exchange/.docs/instructions.md
+++ b/exercises/concept/currency-exchange/.docs/instructions.md
@@ -62,7 +62,19 @@ Effectively, you are rounding _down_ to the nearest whole bill/denomination.
 25
 ```
 
-## 5. Calculate value after exchange
+## 5. Calculate leftover after exchanging into bills
+
+Create the `get_leftover_of_bills()` function, taking `budget` and `denomination`.
+
+This function should return the _leftover amount_ that cannot be exchanged from your _budget_ given the denomination of bills.
+It is very important to know exactly how much the booth gets to keep.
+
+```python
+>>> get_leftover_of_bills(127.5, 20)
+7.5
+```
+
+## 6. Calculate value after exchange
 
 Create the `exchangeable_value()` function, taking `budget`, `exchange_rate`, `spread`, and `denomination`.
 
@@ -81,21 +93,3 @@ Remember that the currency *denomination* is a whole number, and cannot be sub-d
 >>> exchangeable_value(127.25, 1.20, 10, 5)
 95
 ```
-
-## 6. Calculate non-exchangeable value
-
-Create the `non_exchangeable_value()` function, taking `budget`, `exchange_rate`, `spread`, and `denomination`.
-
-This function should return the value that is *not* exchangeable due to the *denomination* of the bills.
-Remember - this booth gets to keep the change _in addition_ to charging an exchange fee.
-Start by calculating the value you would receive if you were able to keep subdivided bills, then subtract the amount you would receive in whole bills.
-Both amounts should take the spread, or the exchange fee into account.
-
-**Note:** Returned value should be `int` type.
-
-```python
->>> non_exchangeable_value(127.25, 1.20, 10, 20)
-16
->>> non_exchangeable_value(127.25, 1.20, 10, 5)
-1
-```
diff --git a/exercises/concept/currency-exchange/.meta/config.json b/exercises/concept/currency-exchange/.meta/config.json
index 11c09f84..e0026902 100644
--- a/exercises/concept/currency-exchange/.meta/config.json
+++ b/exercises/concept/currency-exchange/.meta/config.json
@@ -2,6 +2,7 @@
   "blurb": "Learn about numbers by solving Chandler's currency exchange conundrums.",
   "icon": "hyperia-forex",
   "authors": ["Ticktakto", "Yabby1997", "limm-jk", "OMEGA-Y", "wnstj2007", "J08K"],
+  "contributors": ["pranasziaukas"],
   "files": {
     "solution": ["exchange.py"],
     "test": ["exchange_test.py"],
diff --git a/exercises/concept/currency-exchange/.meta/exemplar.py b/exercises/concept/currency-exchange/.meta/exemplar.py
index fc1b004b..c894f2c0 100644
--- a/exercises/concept/currency-exchange/.meta/exemplar.py
+++ b/exercises/concept/currency-exchange/.meta/exemplar.py
@@ -13,7 +13,7 @@ def get_change(budget, exchanging_value):
     """
 
     :param budget: float - amount of money you own.
-    :param exchanging_value: int - amount of your money you want to exchange now.
+    :param exchanging_value: float - amount of your money you want to exchange now.
     :return: float - amount left of your starting currency after exchanging.
     """
 
@@ -24,11 +24,11 @@ def get_value_of_bills(denomination, number_of_bills):
     """
 
     :param denomination: int - the value of a bill.
-    :param number_of_bills: int amount of bills you received.
+    :param number_of_bills: int - amount of bills you received.
     :return: int - total value of bills you now have.
     """
 
-    return number_of_bills * denomination
+    return denomination * number_of_bills
 
 
 def get_number_of_bills(budget, denomination):
@@ -39,36 +39,32 @@ def get_number_of_bills(budget, denomination):
     :return: int - number of bills after exchanging all your money.
     """
 
-    return int(budget / denomination)
+    return int(budget) // denomination
 
 
-def exchangeable_value(budget, exchange_rate, spread, denomination):
+def get_leftover_of_bills(budget, denomination):
     """
 
-    :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 budget: float - the amount of money you are planning to exchange.
     :param denomination: int - the value of a single bill.
-    :return: int - maximum value you can get.
+    :return: float - the leftover amount that cannot be exchanged given the current denomination.
     """
 
-    exchange_fee = (exchange_rate / 100) * spread
-    actual_rate = exchange_rate + exchange_fee
-    exchangeable_amount = int((budget / actual_rate) / denomination)
-    return exchangeable_amount * denomination
+    return budget % denomination
 
 
-def non_exchangeable_value(budget, exchange_rate, spread, denomination):
+def exchangeable_value(budget, exchange_rate, spread, denomination):
     """
 
-    :param budget: float - amount of money you are planning to exchange.
-    :param exchange_rate: float - unit value of the foreign currency.
-    :param spread: int - the percentage taken as an exchange fee.
-    :param denomination:  int - the value of a single bill.
-    :return: int - the value that cannot be exchanged, due to the denomination.
+    :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.
     """
 
     exchange_fee = (exchange_rate / 100) * spread
-    actual_rate = exchange_rate + exchange_fee
-    non_exchangeable_amount = int((budget / actual_rate) % denomination)
-    return non_exchangeable_amount
+    exchange_value = exchange_money(budget, exchange_rate + exchange_fee)
+    number_of_bills = get_number_of_bills(exchange_value, denomination)
+    value_of_bills = get_value_of_bills(denomination, number_of_bills)
+    return value_of_bills
diff --git a/exercises/concept/currency-exchange/exchange.py b/exercises/concept/currency-exchange/exchange.py
index 29019e8c..5b54fea4 100644
--- a/exercises/concept/currency-exchange/exchange.py
+++ b/exercises/concept/currency-exchange/exchange.py
@@ -13,7 +13,7 @@ def get_change(budget, exchanging_value):
     """
 
     :param budget: float - amount of money you own.
-    :param exchanging_value: int - amount of your money you want to exchange now.
+    :param exchanging_value: float - amount of your money you want to exchange now.
     :return: float - amount left of your starting currency after exchanging.
     """
 
@@ -42,27 +42,25 @@ def get_number_of_bills(budget, denomination):
     pass
 
 
-def exchangeable_value(budget, exchange_rate, spread, denomination):
+def get_leftover_of_bills(budget, denomination):
     """
 
-    :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 budget: float - the amount of money you are planning to exchange.
     :param denomination: int - the value of a single bill.
-    :return: int - maximum value you can get.
+    :return: float - the leftover amount that cannot be exchanged given the current denomination.
     """
 
     pass
 
 
-def non_exchangeable_value(budget, exchange_rate, spread, denomination):
+def exchangeable_value(budget, exchange_rate, spread, denomination):
     """
 
     :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 non-exchangeable value.
+    :return: int - maximum value you can get.
     """
 
     pass
diff --git a/pylintrc b/pylintrc
index 285f630a..745e6f03 100644
--- a/pylintrc
+++ b/pylintrc
@@ -57,51 +57,43 @@ confidence=
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
-#  inconsistent-return-statements,
+# As of Pylint 2.6+, the following options are not supported, so they're commented out:
+#
+# misplaced-comparison-constant
+# relative-import
+# input-builtin
+# inconsistent-return-statements
+# no-absolute-import
+# raising-string
+# round-builtin
+
 disable=arguments-differ,
         attribute-defined-outside-init,
-        duplicate-code,
-        filter-builtin-not-iterating,
         fixme,
         global-statement,
         implicit-str-concat-in-sequence,
         import-error,
         import-self,
-        input-builtin,
         locally-disabled,
-        misplaced-comparison-constant,
-        missing-class-docstring,
-        missing-function-docstring,
-        missing-module-docstring,
-        no-absolute-import,
         no-else-break,
         no-else-continue,
         no-else-raise,
         no-else-return,
         no-member,
         no-name-in-module,
-        no-self-use,
-        raising-string,
-        relative-import,
-        round-builtin,
         signature-differs,
         suppressed-message,
-        too-few-public-methods,
-        too-many-ancestors,
-        too-many-arguments,
         too-many-boolean-expressions,
         too-many-branches,
-        too-many-instance-attributes,
         too-many-locals,
-        too-many-nested-blocks,
         too-many-public-methods,
         too-many-return-statements,
         too-many-statements,
         unnecessary-pass,
         unused-argument,
-        unused-import,
         useless-suppression
 
+
 [REPORTS]
 
 # Set the output format. Available formats are text, parseable, colorized, msvs


Test patch:
diff --git a/exercises/concept/currency-exchange/exchange_test.py b/exercises/concept/currency-exchange/exchange_test.py
index 0eb24a25..694c82d6 100644
--- a/exercises/concept/currency-exchange/exchange_test.py
+++ b/exercises/concept/currency-exchange/exchange_test.py
@@ -5,20 +5,20 @@ from exchange import (
     get_change,
     get_value_of_bills,
     get_number_of_bills,
-    exchangeable_value,
-    non_exchangeable_value)
+    get_leftover_of_bills,
+    exchangeable_value)
 
 
 class CurrencyExchangeTest(unittest.TestCase):
 
     @pytest.mark.task(taskno=1)
     def test_exchange_money(self):
-        input_data = [(100000, 0.84), (700000, 10.1)]
-        output_data = [119047, 69306]
+        input_data = [(100000, 0.8), (700000, 10.0)]
+        output_data = [125000, 70000]
 
         for variant, (input_data, output_data) in enumerate(zip(input_data, output_data), start=1):
             with self.subTest(f"variation #{variant}", input_data=input_data, output_data=output_data):
-                self.assertEqual(int(exchange_money(input_data[0], input_data[1])), output_data)
+                self.assertAlmostEqual(exchange_money(input_data[0], input_data[1]), output_data)
 
     @pytest.mark.task(taskno=2)
     def test_get_change(self):
@@ -27,7 +27,7 @@ class CurrencyExchangeTest(unittest.TestCase):
 
         for variant, (input_data, output_data) in enumerate(zip(input_data, output_data), start=1):
             with self.subTest(f"variation #{variant}", input_data=input_data, output_data=output_data):
-                self.assertEqual(get_change(input_data[0], input_data[1]), output_data)
+                self.assertAlmostEqual(get_change(input_data[0], input_data[1]), output_data)
 
     @pytest.mark.task(taskno=3)
     def test_get_value_of_bills(self):
@@ -48,6 +48,15 @@ class CurrencyExchangeTest(unittest.TestCase):
                 self.assertEqual(get_number_of_bills(input_data[0], input_data[1]), output_data)
 
     @pytest.mark.task(taskno=5)
+    def test_get_leftover_of_bills(self):
+        input_data = [(10.1, 10), (654321.0, 5), (3.14, 2)]
+        output_data = [0.1, 1.0, 1.14]
+
+        for variant, (input_data, output_data) in enumerate(zip(input_data, output_data), start=1):
+            with self.subTest(f"variation #{variant}", input_data=input_data, output_data=output_data):
+                self.assertAlmostEqual(get_leftover_of_bills(input_data[0], input_data[1]), output_data)
+
+    @pytest.mark.task(taskno=6)
     def test_exchangeable_value(self):
         inputs = [
             (100000, 10.61, 10, 1),
@@ -61,17 +70,3 @@ class CurrencyExchangeTest(unittest.TestCase):
         for variant, (inputs, output_data) in enumerate(zip(inputs, output_data), start=1):
             with self.subTest(f"variation #{variant}", inputs=inputs, output_data=output_data):
                 self.assertEqual(exchangeable_value(inputs[0], inputs[1], inputs[2], inputs[3]), output_data)
-
-    @pytest.mark.task(taskno=6)
-    def test_non_exchangeable_value(self):
-        inputs = [
-            (100000, 10.61, 10, 1),
-            (1500, 0.84, 25, 40),
-            (425.33, 0.0009, 30, 700),
-            (12000, 0.0096, 10, 50)]
-
-        output_data = [0, 28, 229, 13]
-
-        for variant, (inputs, output_data) in enumerate(zip(inputs, output_data), start=1):
-            with self.subTest(f"variation #{variant}", inputs=inputs, output_data=output_data):
-                self.assertEqual(non_exchangeable_value(inputs[0], inputs[1], inputs[2], inputs[3]), output_data)