5
\$\begingroup\$

First time asking a question here.

I made a simple application as a task for a position of a junior programmer, but it did not work out the way I hoped. Nevertheless, I would definitely be interested in code improvement suggestions (sorry for not being more specific, I did not get any feedback by the task creators, so I try to get some here).

Desired functionality description is as follows (see https://gist.github.com/MichalCab/c1dce3149d5131d89c5bbddbc602777c):

Create currency converter

What to do?

  • CLI application
  • web API application

What do we expect?

  • show us your best
  • take your time, you have 2 weeks for implementation
  • real-life production ready project

Limitations

  • Python
  • all modules are allowed
  • no other limitations

Parameters

  • amount - amount which we want to convert - float
  • input_currency - input currency - 3 letters name or currency symbol
  • output_currency - requested/output currency - 3 letters name or currency symbol

Functionality

  • if output_currency param is missing, convert to all known currencies

Output

  • json with following structure.

    {
        "input": { 
            "amount": <float>,
            "currency": <3 letter currency code>
        }
        "output": {
            <3 letter currency code>: <float>
        }
    }
    

Examples

CLI

./currency_converter.py --amount 100.0 --input_currency EUR --output_currency CZK
{   
    "input": {
        "amount": 100.0,
        "currency": "EUR"
    },
    "output": {
        "CZK": 2707.36, 
    }
}

./currency_converter.py --amount 0.9 --input_currency ¥ --output_currency AUD
{   
    "input": {
        "amount": 0.9,
        "currency": "CNY"
    },
    "output": {
        "AUD": 0.20, 
    }
}

./currency_converter.py --amount 10.92 --input_currency £ 
{
    "input": {
        "amount": 10.92,
        "currency": "GBP"
    },
    "output": {
        "EUR": 14.95,
        "USD": 17.05,
        "CZK": 404.82,
        .
        .
        .
    }
}

API

GET /currency_converter?amount=0.9&input_currency=¥&output_currency=AUD HTTP/1.1
{   
    "input": {
        "amount": 0.9,
        "currency": "CNY"
    },
    "output": {
        "AUD": 0.20, 
    }
}

GET /currency_converter?amount=10.92&input_currency=£ HTTP/1.1
{
    "input": {
        "amount": 10.92,
        "currency": "GBP"
    },
    "output": {
        "EUR": 14.95,
        "USD": 17.05,
        "CZK": 404.82,
        .
        .
        .
    }
}

And the project itself can be found here: https://github.com/ciso112/kiwi-currencies

I enclose a service.py class which is imported by CLI or web API interface (pls refer to the project link above).

service.py:

import json
import logging
import requests
import requests_cache

# global dictionary filled up at a start of an application
currencies_symbols = {}

requests_cache.install_cache('currency_cache', backend='sqlite', expire_after=21600)    #expires after 6 hours

logging.basicConfig(format='%(levelname)s:%(message)s', level=logging.DEBUG)

#creates an output in JSON format
def create_json(input_currency, output_currency, amount):
    logging.info(" FUNC: create_json parameters: inp:%s out:%s am=%s", input_currency, output_currency, amount)
    if input_currency == None or output_currency == None:
        return "Currency not recognized"

    # if input_currency contains ",", it means a currency sign has different currency representations
    # f.e.: symbol £ can be used for GIP,SYP,SHP,LBP,EGP,GBP,FKP
    if "," in input_currency:
        return "Input currency not clearly defined. Possible currencies with such symbol: " + input_currency

    dict = {}
    dict["input"] = {"amount": float(amount), "currency": input_currency}

    # in case no output currencies, prepare to convert to all known currencies
    if output_currency == "None":
        output_currency = ",".join(list(currencies_symbols.values()))

    output_currencies = output_currency.split(",")
    for curr in output_currencies:
        if not curr == input_currency:
            if "output" not in dict:
                dict["output"] = {}
                dict["output"].update({curr: convert(input_currency, curr, amount)})

    return json.dumps(dict, indent=4, separators=(',', ': '))

#input/output can be in a currency symbol format, but we will need 
#a three-letter currency name
def sign_to_abbreviation(curr):
    logging.info(" FUNC: sign_to_abbreviation parameters: curr:%s", curr)
    if len(curr) == 3:
        return curr
    if curr != "None":
        for key, value in currencies_symbols.items():
            if curr == key:
                curr = value
                return curr
        return None
    return curr

#function run at the start of the program to get the known currencies 
#and their symbols
def create_currencies_dict():
    if not currencies_symbols:
        logging.info(" FUNC: create_currencies_dict")
        api_url = "https://free.currencyconverterapi.com/api/v6/currencies"
        response = requests.get(api_url)
        if response.status_code == 200:
            currencies = json.loads(response.content.decode('utf-8'))
            for key, value in currencies['results'].items():
                if 'currencySymbol' in value.keys():
                    # because some currencies have the same symbol --> we append values to the same key
                    if value['currencySymbol'] in currencies_symbols.keys():
                        new_currency = currencies_symbols.get(value['currencySymbol'])
                        currencies_symbols[value['currencySymbol']] = value['id'] + "," + new_currency
                    else:
                        currencies_symbols[value['currencySymbol']] = value['id']
                else:
                    pass
            return currencies_symbols
        else:
            return None


def convert(input_currency, output_currency, amount):
    logging.info(" FUNC: convert parameters: inp:%s out:%s am=%s", input_currency, output_currency, amount)
    if len(input_currency) and len(output_currency) == 3:
        # returns dictionary with exactly 1 key-value pair
        rate = contact_api(input_currency, output_currency)
        logging.info(" FUNC: convert rate: %s", rate)
        try:
            return round(float(amount) * float(rate.get(input_currency+"_"+output_currency)), 2)
        except TypeError as e:
            print("WARN Check your currencies. Exception: ", e.args)


    else:
        return "Currency not recognized"


# external converter service
def contact_api(inp, out):
    logging.info(" FUNC: contact_api parameters: inp:%s out:%s", inp, out)
    api_url_base = 'http://free.currencyconverterapi.com/api/v5/convert'
    conversion = inp + "_" + out
    payload = {"q": conversion, "compact": "ultra"}
    response = requests.get(api_url_base, params=payload)
    logging.info(" FUNC: contact_api Loading from CACHE: %s", response.from_cache)

    if response.status_code == 200:
        return json.loads(response.content.decode('utf-8'))
    else:
        return None
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CodeReview! I've included the problem statement for you, but please keep in mind next time that posts should be self-sufficient; while it's okay to link to external resources, all relevant information has to be included for a post to be on-topic (the length limit for a post is actually quite high). Hope you get some more useful reviews and enjoy your stay! \$\endgroup\$ – ferada Sep 23 '18 at 15:18
3
\$\begingroup\$

Sorry to hear that; at a first glance the code looks okay, naming and splitting into functionality looks good, as well as the use of appropriate libraries, but there's a number of things that could be improved too.

For starters, on the GitHub README.md you're listing a couple of libraries that have to be installed - so it would be easy enough to add a requirements.txt for pip to install them (or an equivalent setup via virtualenv etc.). As an interviewer the smoother the whole thing goes, from downloading to seeing the expected result, the better! Consider next time how someone else runs your project, make it as easy as possible for them, while not going overboard (like, unless you're going to provide a Docker image for them (which is obviously an option ...) for a Python project: Requiring them to have Python 3.X installed is okay, same for pip, but the specific libraries not; give them a Makefile perhaps such that make dependencies runs pip install -r requirements.txt or so).

Next, the README.md says Run a main.py file..., but instead you're using __main__.py, so the documentation should be updated; like I'd rather have it say "Run PYTHONPATH=. python3 -m pyapi." since I might not be familiar with all variants of how Python programs can be invoked (e.g. I had to play with a few different variants till I figured out how to run it ... an interviewer likely has less patience and time than me.)


Okay, so I run pyapi, then curl the examples from the description page (not yours mind you, it'd be a good idea to have it all on your repository too so it doesn't depend on someone else's files!), the first example fails, the second one looks okayish:

0 ~ % curl "http://localhost:5000/currency_converter?amount=0.9&input_currency=¥&output_currency=AUD"
Currency not recognized%
0 ~ % curl "http://localhost:5000/currency_converter?amount=10.92&input_currency=£"
{
    "input": {
        "amount": 10.92,
        "currency": "\u00c2\u00a3"
    },
    "output": {
        "ALL": null
    }
}%

Though, well, the output is empty. Can't really tell why except it looks like I'm inputting the wrong currencies? Using EUR actually works and gives me output:

0 ~ % curl "http://localhost:5000/currency_converter?amount=10.92&input_currency=EUR"
{
    "input": {
        "amount": 10.92,
        "currency": "EUR"
    },
    "output": {
        "ALL": 1380.78
    }
}%

Now CLI, much better, first example works out of the box, second and third don't, same problem, the currency unit isn't converted somehow. Though if I do it myself it works (JPY/GBP). Ah, right, there's some where the currency unit isn't unique, that's fine I guess.

However, if the output currency isn't specified I read the problem statement such that it should then return the conversion to all other currencies ("if output_currency param is missing, convert to all known currencies"), but that's not what's happening:

0 kiwi-currencies % PYTHONPATH=. python3 -m pycli --amount 10.92 --input_currency EUR
INFO: FUNC: create_currencies_dict
INFO: FUNC: sign_to_abbreviation parameters: curr:EUR
INFO: FUNC: sign_to_abbreviation parameters: curr:None
INFO: FUNC: create_json parameters: inp:EUR out:None am=10.92
INFO: FUNC: convert parameters: inp:EUR out:ALL am=10.92
INFO: FUNC: contact_api parameters: inp:EUR out:ALL
INFO: FUNC: contact_api Loading from CACHE: True
INFO: FUNC: convert rate: {'EUR_ALL': 126.445488}
{
    "input": {
        "amount": 10.92,
        "currency": "EUR"
    },
    "output": {
        "ALL": 1380.78
    }
}

What does ALL mean, "Albanian Leak"?


Now to the actual code, both of the __main__.py look mostly okay to me, but, this isn't production quality code: There are no error checks or fallbacks. If an expected argument is missing e.g. the HTTP API crashes (the CLI is better since the command line argument parser checks for it).

test.py looks pretty good! Though I'd suggest not to use assert like this. It could trivially be disabled when optimisations are turned on:

0 kiwi-currencies % python3 test.py
INFO: FUNC: create_currencies_dict
INFO: TEST: test_contact_api <MagicMock name='contact_api' id='140590866107192'>
INFO: TEST: simple_contact_api
.INFO: TEST: test_sign_to_abbreviation
INFO: FUNC: sign_to_abbreviation parameters: curr:€
INFO: FUNC: sign_to_abbreviation parameters: curr:£
.INFO: TEST: test_unknown_input
INFO: FUNC: convert parameters: inp:USD out:EgUR am=45.2
test.py:54: DeprecationWarning: Please use assertEqual instead.
  "Currency not recognized")
.
----------------------------------------------------------------------
Ran 3 tests in 0.007s

OK

0 kiwi-currencies % python3 -O test.py
INFO: FUNC: create_currencies_dict
INFO: TEST: test_contact_api <MagicMock name='contact_api' id='139676823006232'>
.INFO: TEST: test_sign_to_abbreviation
.INFO: TEST: test_unknown_input
INFO: FUNC: convert parameters: inp:USD out:EgUR am=45.2
test.py:54: DeprecationWarning: Please use assertEqual instead.
  "Currency not recognized")
.
----------------------------------------------------------------------
Ran 3 tests in 0.018s

OK

Notice how the second run doesn't exercise some of the tests!


Now for service.py.

  • You have comments, good, though I also imagine having some of those as docstrings (contact_api) might be nicer.
  • Logging is on, great, that's helpful once you've read the code.
  • sign_to_abbreviation, the for loop should be a dictionary lookup? Also the assignment to curr can be removed and directly returned.
  • create_json, dict = {} is overwritten immediately after. Same with dict["output"] = {}. Also not x == y should be x != y.
  • create_currencies_dict, it might make sense to check for the inverse and return early to keep the indendation shorter, e.g. if currencies_symbols: return and if response.status_code != 200: return None. else: pass at the end of the loop can also be omitted.
  • return None can also be omitted in general, or even just return. If you want to be explicit that's fine though.

So overall there are priorities: make it work, show the user how (not just by test cases, by writing down the exact invocation for them to use), then make it nicer. You're on the right way though!

\$\endgroup\$
1
\$\begingroup\$

First of all, many thanks for your reply. It has brought many interesting information to my attention resulting in an increased quality of the project.


To start with, I must say there were few mistakes in a code resulting in an improper functionality. All the calls without a stated output_currency should have given a different output, the problem was in a wrong indentation of the last line of a code snippet below (it was on the same level as the second-to-last line and therefore always being rewritten instead of added):
service.py, create_json:

..
output_currencies = output_currency.split(",")
    for curr in output_currencies:
        if curr != input_currency:
            if "output" not in dict:
                dict["output"] = {}
            dict["output"].update({curr: convert(input_currency, curr, amount)})
..


To continue, here is an updated version of README.md:

Currency Converter

A practical task for a position of Junior Python Developer. Task entry: https://gist.github.com/MichalCab/c1dce3149d5131d89c5bbddbc602777c

Prerequisites

python 3.4

Requirements

Required libraries can be found in requirements.txt and installed via pip3 install -r requirements.txt

Run application

Run a __main__.py file, either in pyapi or pycli folder depending on a desired form of usage.

Parameters

  • amount - amount which we want to convert - float
  • input_currency - input currency - 3 letters name or currency symbol
  • output_currency - requested/output currency - 3 letters name or currency symbol

Note that a single currency symbol can represent several currencies:
- in case this happens with output_currency, convert to all known currencies with such symbol
- in case this happens with input_currency, conversion is not performer. Rather, an info message with currencies having such symbol is shown, so a user can specify input_currency more precisely

Output Possibilities:

  • json with a following structure:

Single input and output currency: { "input": { "amount": <float>, "currency": <3 letter currency code> } "output": { <3 letter currency code>: <float> } }

Single input and multiple output currencies (in case a currency sign represents more currencies): { "input": { "amount": <float>, "currency": <3 letter currency code> } "output": { <corresponding 3 letter currency code>: <float> <corresponding 3 letter currency code>: <float> . . } }

Single input and no output currency - convert to all known currencies: { "input": { "amount": <float>, "currency": <3 letter currency code> } "output": { <3 letter currency code>: <float> <3 letter currency code>: <float> <3 letter currency code>: <float> . . } }

  • Info message:

Multiple input currencies (in case a currency sign represents more currencies):
"Input currency not clearly defined. Possible currencies with such symbol: <possible currencies>"

Unknown input currency: "Input currency not recognized"

Unknown output currency: "Output currency not recognized"

Examples

CLI

./currency_converter.py --amount 100.0 --input_currency EUR --output_currency CZK { "input": { "amount": 100.0, "currency": "EUR" }, "output": { "CZK": 2561.78 } }

./currency_converter.py --amount 0.9 --input_currency € --output_currency AUD { "input": { "amount": 0.9, "currency": "EUR" }, "output": { "AUD": 1.46 } }

./currency_converter.py --amount 10.92 --input_currency zł { "input": { "amount": 10.92, "currency": "PLN" }, "output": { "HRK": 18.84, "UZS": 24006.34, "RUB": 196.93, "BOB": 20.64, . . . } }

./currency_converter.py --amount 10.92 --input_currency EUR --output_currency £ { "input": { "amount": 10.92, "currency": "EUR" }, "output": { "GBP": 9.79, "FKP": 9.77, "LBP": 19462.11, "SHP": 16.97, "SYP": 6617.36, "EGP": 230.18, "GIP": 9.77 }
}

./currency_converter.py --amount 10.92 --input_currency Nonsense_curr Input currency not recognized

API

Note: When using curl, currencies symbols are not decoded properly and therefore not recognised. A recommended tool is Postman.

GET /currency_converter?amount=4.5&input_currency=₱&output_currency=VEF HTTP/1.1 { "input": { "amount": 4.5, "currency": "PHP" }, "output": { "VEF": 20633.77 } }

GET /currency_converter?amount=10.92&input_currency=£ HTTP/1.1 Input currency not clearly defined. Possible currencies with such symbol: SHP,FKP,EGP,LBP,SYP,GIP,GBP

GET /currency_converter?amount=10.92&input_currency=₦ HTTP/1.1 { "input": { "amount": 10.92, "currency": "NGN" }, "output": { "HRK": 0.19, "UZS": 241.47, "RUB": 1.98, "BOB": 0.21, "TZS": 68.63, "GBP": 0.02, "GIP": 0.02, "GTQ": 0.23, . . . } }


Now, you mentioned "this isn't production quality code: There are no error checks or fallbacks. If an expected argument is missing e.g. the HTTP API crashes". These are the measures I came up with: pyapi/__main__.py:
(simple check if arguments are present)

def get():
    if 'amount' in request.args and 'input_currency' in request.args:
        if 'output_currency' in request.args:
            return service.create_json(service.sign_to_abbreviation(request.args['input_currency']),
                                       service.sign_to_abbreviation(request.args['output_currency']),
                                       request.args['amount'])
        else:
            return service.create_json(service.sign_to_abbreviation(request.args['input_currency']),
                                       "None",
                                       request.args['amount'])
    return "Missing arguments"

service.py, contact_api:
(exception if external site is unreachable (+logging of a time needed for a request))

# external converter service
def contact_api(inp, out):
    logging.info(" FUNC: contact_api parameters: inp:%s out:%s", inp, out)
    api_url_base = 'http://free.currencyconverterapi.com/api/v5/convert'
    conversion = inp + "_" + out
    payload = {"q": conversion, "compact": "ultra"}
    try:
        start_time = time.time()
        response = requests.get(api_url_base, params=payload, timeout=1)  # we have 1 sec to get a response
        logging.info(" FUNC: contact_api request elapsed time: %s", time.time() - start_time)
    except requests.exceptions.ConnectionError as e:
        logging.error(" FUNC: contact_api CONNECTION ERROR: ", e)
        return None

    logging.info(" FUNC: contact_api Loading from CACHE: %s", response.from_cache)
    if response.status_code == 200:
        return json.loads(response.content.decode('utf-8'))
    return None

I consider this to be simple and functional, could it be done better?


And to finish with, tests could get improved too but that is for another day. And for a reply to be complete, here is a link to the project: https://github.com/ciso112/kiwi-currencies

\$\endgroup\$
  • 1
    \$\begingroup\$ You should be creating a new post for any refined code, but yes, much better; the get method can probably be improved though, such that it's a bit more compact; I just searched and one of the first hits was webargs.readthedocs.io/en/latest/quickstart.html to have a more declarative syntax for query arguments. But even with request.args.get("name", "default") you might already be a bit better off too. \$\endgroup\$ – ferada Sep 24 '18 at 21:02
  • 1
    \$\begingroup\$ Just a quick note regarding the log levels - Info (IMHO) should only be used when you really need the user to get the message. The user already knows what they put on the command line, you don't need to tell them immediately when they run the app. Also, logging inside an exception should use logging.exception, not error and without the e. Also, you have inconsistent return values - both JSON and None. Instead of None, return a message stating why it failed with HTTP status of 400 or other Client Error return code. That way you don't need to interpret None higher up in the stack. \$\endgroup\$ – C. Harley Sep 25 '18 at 0:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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