0
\$\begingroup\$

I'm a beginner and it is my first project in Python 2.7.

My goal is writing a script that I can run with two or three arguments. If run with two arguments like python currency.py RUB USD then the output will be current RUB rate by USD base: .015169. If run with three arguments like python currency.py 1000 RUB USD then it will convert 1000 RUB to dollar.

#!/usr/bin/env python
import ast, requests
import sys, argparse
def checker(base, symbol):
    url = "http://api.fixer.io/latest?base=%s" % base.upper()
    r = requests.get(url)
    content = ast.literal_eval(r.content)
    rate = content["rates"][symbol.upper()]
    return rate
parser = argparse.ArgumentParser(description = "Currency converter. For instance : 'Convert 100 USD to RUB' Will be: 100 USD RUB. If You need only rate: USD RUB")
if len(sys.argv) > 3:
    parser.add_argument("n", type =int, help = "Amount of currency_for_convert", default=1)
parser.add_argument('currency_for_convert', type=str, help="Currency for convert")
parser.add_argument("currency_converted_to", type=str, help = "Currency that converted to")
args = parser.parse_args()
rate = checker(args.currency_for_convert, args.currency_converted_to)
print rate if len(sys.argv) <= 3 else rate*args.n

But I don't like the way I'm using if statements because the running script help ignore third argument and give information only about first two arguments.

$ python currency.py --help usage: currency.py [-h] currency_for_convert currency_converted_to

Currency converter. For instance : 'Convert 100 USD to RUB' Will be: 100 USD RUB. If You need only rate: USD RUB

positional arguments: currency_for_convert Currency for convert
currency_converted_to Currency that converted to

optional arguments: -h, --help show this help message and exit

Is there another way to write two/tree arguments script?

\$\endgroup\$
  • 1
    \$\begingroup\$ Your spelling of "currency" is inconsistent (i.e. wrong) in several places. \$\endgroup\$ – BCdotWEB Apr 25 '16 at 9:17
  • \$\begingroup\$ Reading your question, it sounds like your Python script isn't working properly (since help ignore third argument). If so, it is off-topic for Code Review since this site expects working code. \$\endgroup\$ – Tunaki Apr 25 '16 at 9:37
  • 1
    \$\begingroup\$ @Tunaki I believe they mean that the command line help isn't very clear because of the way their parameters work, not that it's nonfunctional. \$\endgroup\$ – SuperBiasedMan Apr 25 '16 at 9:49
  • \$\begingroup\$ @SuperBiasedMan It is functional , but the --help option does not give whole information about arguments. I could use "n" argument as optional. In this case, I should type "-n" every time I give the third argument that makes input more complicated. \$\endgroup\$ – IvanSp Apr 26 '16 at 4:05
1
\$\begingroup\$

Never assume that your user doesn't make mistakes. I ran your program with no arguments ... error message, fine. I ran it with a single argument of 4 ... error message, fine. I ran it with two arguments, 4 and 5 ...:

Traceback (most recent call last):
  File "test.py", line 17, in <module>
    rate = checker(args.curency_for_convert, args.curency_converted_to)
  File "test.py", line 9, in checker
    rate = content["rates"][symbol.upper()]
KeyError: 'rates'

For debugging purposes, I put print content above that line and ran again:

{'error': 'Invalid base'}

If I run the program with a valid base, but an invalid symbol:

Traceback (most recent call last):
  File "test.py", line 16, in <module>
    rate = checker(args.curency_for_convert, args.curency_converted_to)
  File "test.py", line 8, in checker
    rate = content["rates"][symbol.upper()]
KeyError: '8'

You should catch these errors and display something that is a little more user-friendly.


I would recommend that you read PEP 8, the Python style guide. There are a couple things you aren't following:

Imports should usually be on separate lines, e.g.:

Yes:

import os
import sys

No:

import sys, os

It's okay to say this though:

from subprocess import Popen, PIPE

Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value.


Your whitespace is a little strange. I would have at least one blank line above and below the function definition. I would also put one after the if block and maybe even before it too. You have " =", " = ", and "=" used in just three lines. PEP 8 says that A Foolish Consistency is the Hobgoblin of Little Minds, but that doesn't mean never be consistent. It is only the foolish consitency which is a hobgoblin. In this case, you have nothing to lose.

Your naming is a little strange. n even shows up in the help messages. One-letter variable names are generally not recommended because they give very little idea of what the variable is for. curency_for_convert and curency_converted_to, besides mis-spelling "currency", are ungrammatical. You could say "for conversion" or "to convert", but "for convert"? I would suggest "convert_from" and "convert_to".

if len(sys.argv) > 3:
    parser.add_argument("n", type =int, help = "Amount of curency_for_convert", default=1)

Instead of manually parsing an argument, make it optional with nargs='?'

print rate if len(sys.argv) <= 3 else rate*args.n

Since our argument will always be present (either with the default of 1 or with a user-supplied number), we can now do print rate * args.n.

Full Program

#!/usr/bin/env python
import argparse
import ast
import requests
import sys

def checker(base, symbol):
    url = "http://api.fixer.io/latest?base=%s" % base.upper()
    r = requests.get(url)
    content = ast.literal_eval(r.content)
    rate = content["rates"][symbol.upper()]
    return rate

parser = argparse.ArgumentParser(description=
    "Currency converter. For instance: to convert 100 USD to RUB, it would be "
    "100 USD RUB. If you need only the rate: USD RUB"
)

parser.add_argument("amount", type=int, help="Amount of currency to convert", default=1, nargs='?')
parser.add_argument('convert_from', type=str, help="Currency to convert")
parser.add_argument('convert_to', type=str, help = "Currency of converted")

args = parser.parse_args()

try:
    rate = checker(args.convert_from, args.convert_to)
except KeyError:
    print "Invalid argument(s)"
else:
    print rate*args.amount
\$\endgroup\$
0
\$\begingroup\$

Style

Give your program some love. As it stand it is just a big pile of instructions. Add a bit of vertical spacing to separate logical sections, limit your line length so we can get most things at a glance.

You should also read and follow PEP8, the official Python's style guide:

  • each import statement on its own line;
  • consistent use of spaces around the = sign;
  • meaningful variable names (r is not).

The url variable isn't really one. You should extract it out of the function and give it a proper capitalized name as the constant it is. Also you should use str.format instead of % as it is the new standard.

Top-level code

Even if it is not that important for such "short" code, it is always a good practice to wrap your top-level code under an if __name__ == '__main__' statement. At the very least, it lets you import your code into an interactive session to test your function without triggering all its logic.

Requests

The responses from api.fixer.io are json strings, not Python dictionaries. You can convert back and forth between the two using the json module. However, requests provide a builtin hook for that so you don't even have to import the module:

content = r.json()

Argparse

You can totally get the help usage you’re looking for if you provide more than two parameters to your program. For instance:

$ python currency.py a b --help

However it is less than ideal. Argparse provide a way to have optional required arguments (sounds weird anyway):

  • set the nargs parameter to '?' to indicate it is optional;
  • set a default value.

Since there is only one optional parameter, argparse will figure out things pretty well base on the length of sys.argv. Now how do you figure it out? Easily enough, if default=1 then you perform rate * args.n blindlessly.

Oh, and type=str is useless as it is the default.

Proposed improvements

#!/usr/bin/env python

import requests
import argparse


URL = "http://api.fixer.io/latest?base={}"


def get_rate(base, symbol):
    url = URL.format(base.upper())
    content = requests.get(url).json()
    return content['rates'][symbol.upper()]


if __name__ == '__main__':
    parser = argparse.ArgumentParser(
            description="Currency converter. For instance : 'Convert 100"
            " USD to RUB' Will be: 100 USD RUB. If You need only rate: "
            "USD RUB")
    parser.add_argument("n", type=int, nargs="?", default=1,
                        help="Amount of currency_for_convert")
    parser.add_argument("base_currency", help="Currency for convert")
    parser.add_argument("converted_currency", help="Currency that converted to")

    args = parser.parse_args()

    converted = args.n * get_rate(args.base_currency, args.converted_currency)
    print converted

You may also be interested into handling networking issues, conversions issues or bad currencies.

\$\endgroup\$
  • \$\begingroup\$ Thank you a lot! I found your answer very useful and informative! \$\endgroup\$ – IvanSp Apr 26 '16 at 5:48

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.