5
\$\begingroup\$

Beginner Python developer here. I was tinkering around with Python and decided to build a currency converter. It takes in input from the user and prints out the converted currency values. The fixer.io API is used to get currency rates. Looking for any improvements that I can make in it.

GitHub

Code

# importing required libraries
import requests
import pycountry
from _datetime import datetime
from babel import numbers


def error_sev():
    print("Sorry! There seems to be an error. Please check if your network is working")


def error_inp():
    print("Sorry! There seems to be an error. Please check if the currencies entered are valid.")


def currency_print(input_cur, output_cur, input_currency_name, output_currency_name, amount, rate):
    # printing out exchange rate
    print("The rate for {} to {} as on {} is: "
          .format(input_currency_name, output_currency_name, date.strftime("%d-%m-%Y")), end='')
    print(numbers.format_currency(1, input_cur, locale='en') + " = " +
          numbers.format_currency(rate, output_cur, locale='en'))

    # printing converted value
    print("\t", end='')
    print(numbers.format_currency(amount, input_cur, locale='en') + " = " +
          numbers.format_currency(amount * rate, output_cur, locale='en'))

    print('-'*100)


# list of available currencies
currencies = [
    'USD', 'JPY', 'BGN', 'CZK', 'DKK', 'GBP', 'HUF', 'PLN', 'RON', 'SEK', 'CHF', 'NOK', 'HRK', 'RUB', 'TRY',
    'AUD', 'BRL', 'CAD', 'CNY', 'HKD', 'IDR', 'ILS', 'INR', 'KRW', 'MXN', 'MYR', 'NZD', 'PHP', 'SGD', 'THB',
    'ZAR', 'ISK'
]

# printing the list of available currencies for the user
print("Available currencies: ", end='')
for item in sorted(currencies)[:-1]:
    print(item, end=', ')
print(sorted(currencies)[-1])

# taking user input
try:
    amount = float(input("Enter amount: "))
# checking for input errors
except ValueError:
    print("Invalid input. Please enter only numbers.")

else:
        # taking currency values as input from user
        input_cur = input("Enter base currency code: ").upper()
        output_cur = input("Enter desired currency code (leave blank for all currencies): ").upper()

        # if user has specified desired currency
        if output_cur != '':
            response_url = "http://api.fixer.io/latest?base={}&symbols={}".format(input_cur, output_cur)
            response = requests.get(response_url)
            # checking for validity of inputs

            if output_cur in currencies and input_cur in currencies:

                # checking for validity of server
                if response.status_code is 200:
                    # parsing JSON response
                    data = response.json()
                    date = datetime.strptime(data['date'], "%Y-%m-%d")

                    rate = data['rates'][output_cur]

                    print('-' * 100)

                    # Getting currency names
                    input_currency_name = pycountry.currencies.get(alpha_3=input_cur).name
                    output_currency_name = pycountry.currencies.get(alpha_3=output_cur).name

                    currency_print(input_cur, output_cur, input_currency_name, output_currency_name, amount, rate)
                else:
                    # printing a server error
                    error_sev()
            else:
                # printing an input error
                error_inp()

        # if user has not specified desired currency, print out all conversions
        else:
            response_url = "http://api.fixer.io/latest?base={}".format(input_cur)
            response = requests.get(response_url)

            # checking for input validity
            if input_cur in currencies:

                # checking for validity of inputs and server
                if response.status_code is 200:
                    # parsing JSON response
                    data = response.json()
                    date = datetime.strptime(data['date'], "%Y-%m-%d")

                    print('-' * 100)

                    # looping through all rates
                    rates = data['rates']
                    for rate in sorted(rates):

                        cur_rate = rates[rate]
                        input_currency_name = pycountry.currencies.get(alpha_3=input_cur).name
                        output_currency_name = pycountry.currencies.get(alpha_3=rate).name

                        print("{} ({})".format(output_currency_name, rate))
                        print("\t", end='')

                        currency_print(input_cur, rate, input_currency_name, output_currency_name, amount, cur_rate)

                else:
                    # printing a server error
                    error_sev()
            else:
                # printing an input error
                error_inp()
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Code Style Improvements

  • "Flat is better than nested". You can make an early exit in case of invalid input:

    import sys
    
    try:
        amount = float(input("Enter amount: "))
    except ValueError:
        print("Invalid input. Please enter only numbers.")
        sys.exit(1)
    

    That will allow you to remove the else: part and continue on the top-level. Or, you can let the user retry the input until it is valid

  • on the same topic of decreasing nestedness depth - add more early exists. For instance, if input currencies are invalid, throw an error and exit. Then, remove the else: and continue with your "positive case" logic on the same level. This should improve overall readability
  • define the constants, like the list of currencies, as per PEP8 - in upper case (reference)
  • put the main execution logic to under if __name__ == '__main__':
  • you can simplify if output_cur != '': with just if output_cur:
  • I'm not sure why you are importing datetime from _datetime (with underscore). I would expect the import to be from datetime import datetime
  • don't put comment for obvious parts of the code. For example, "importing required libraries" does not provide any useful information.
  • organize imports per PEP8 - stdlib libraries, then a newline, third-parties, a new line and then your "local" dependencies, all sorted alphabetically:

    from datetime import datetime
    
    from babel import numbers
    import pycountry
    import requests
    

Other High-level ideas

  • define custom exceptions. Instead of using the error_sev and error_inp functions where you print errors, define custom exceptions like InvalidCountryValueError. Throw it with your custom message inside
  • since you are posting it on github, consider organizing the project properly - add requirements.txt with the list of dependencies, add more documentation, tests - see more at Open Sourcing a Python Project the Right Way
  • on the related topic: currently, there is only one way to use your program. Consider someone who wants to use your library as an API - not going through the standard in inputs, but calling a function asking for currency rates. Thinking about your program this way may help you to re-design it a bit, apply "Extract Method" and other refactoring methods. Also, if you would try to add tests, you will quickly realize that there is no easy way to unittest the program - usually a red flag when designing clean and modular APIs

Performance notes

  • I'd use a set to keep the supported list of currencies. Since you check the input currencies to be valid with in, this should have a positive impact on performance:

    CURRENCIES = {
        'USD', 'JPY', 'BGN', 'CZK', 'DKK', 'GBP', 'HUF', 'PLN', 'RON', 'SEK', 'CHF', 'NOK', 'HRK', 'RUB', 'TRY',
        'AUD', 'BRL', 'CAD', 'CNY', 'HKD', 'IDR', 'ILS', 'INR', 'KRW', 'MXN', 'MYR', 'NZD', 'PHP', 'SGD', 'THB',
        'ZAR', 'ISK'
    }
    
\$\endgroup\$
5
  • \$\begingroup\$ Completely forgot about the .exit() thing. Thanks :D \$\endgroup\$
    – lakshyaag
    Mar 31, 2017 at 18:35
  • 1
    \$\begingroup\$ @LakshyaAg FYI, still adding notes, stay tuned. Thanks. \$\endgroup\$
    – alecxe
    Mar 31, 2017 at 18:35
  • \$\begingroup\$ I was taught to comment wherever possible by my teacher :P. Will remove that. The import was done automatically by PyCharm. \$\endgroup\$
    – lakshyaag
    Mar 31, 2017 at 18:38
  • \$\begingroup\$ @LakshyaAg okay, I think I'm finished with this code review round now. There are more things to say, but it's perfectly okay to re-iterate over the same problem here on code review - you may apply the suggested changes and post a new question with the updated code and see if there are any more things to improve. Thanks, hope that helps. \$\endgroup\$
    – alecxe
    Mar 31, 2017 at 18:46
  • \$\begingroup\$ I'm not going with the API thing right now (not so advanced). Changed my code on the suggestions. Thanks a ton @alecxe \$\endgroup\$
    – lakshyaag
    Mar 31, 2017 at 18:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.