5
\$\begingroup\$

I just wrote this small inch <-> cm conversion program. It works just fine, although I know that I've done some things in a rather stupid manner and that it could be improved.

def conversion():
    try: amount = int(raw_input("Please enter the value: "))
    except ValueError:
        print "Please specify a valid amount."
        return conversion()
    answer = raw_input("Please choose between converting FROM kilograms/pounds: ")
    if answer == "kilograms":
        return amount * 2.2
    elif answer == "pounds":
        return amount / 1.45
    else:
        print "Please choose between kilograms and pounds."
        restart = raw_input("Try again? ")
        if restart == "yes":
            return conversion()
        elif restart == "y":
            return conversion()
        else:
            print "Okay, bye."
            return

print conversion()
\$\endgroup\$
  • 1
    \$\begingroup\$ where are cm and inch? \$\endgroup\$ – Emanuele Paolini Aug 24 '14 at 16:49
6
\$\begingroup\$

The first point would be the separation of concerns: every entity (function, class …) should only be responsible for one task (except, ironically, printing the result).

In your code, one function (conversion) is responsible for everything. Try separating the different issues:

  • user input
  • conversion
  • output

Granted, for such a very small program the result will almost certainly be larger. But it will also be much clearer and easier to extend.

Next up, your use of recursion. I love recursion as much (more!) as the next programmer but in your case a few loops wouldn’t hurt readability.

A third point, magic constants should be avoided. Your use of the conversion factors 2.2 and 1.45 is innocuous enough but it soon becomes a problem in larger programs. Use properly-named constants instead.

\$\endgroup\$
4
\$\begingroup\$

Don't use recursion in this case. Python doesn't optimize tail recursion and after long session script can show your RuntimeError: maximum recursion depth exceeded.

Don't use floating point arithmetic in this case. Use decimal arithmetic with decimal module. For example:

>>> 100 * 2.2
220.00000000000003
>>> from decimal import Decimal
>>> Decimal("100") * Decimal("2.2")
Decimal('220.0')

Split logic and presentation. For now there is no way to use this converter with Web UI or as a library for a bigger program. For example you can start with low level functions like this:

def kilograms_to_pounds(amount):
    return amount * Decimal("2.2")

def pounds_to_kilograms(amount):
    return amount / Decimal("1.45")

Then you can create front-end function like this:

converters = {
    ("kilograms", "pounds"): kilograms_to_pounds,
    ("pounds", "kilograms"): pounds_to_kilograms,
}

def convert(amount, from_, to):
    c = converters.get((from_, to))
    if c is None:
        raise ValueError("converter not found")
    return c(amount)

>>> convert(Decimal("100"), "pounds", "kilograms")
Decimal('68.96551724137931034482758621')

Later you can add aliases like this (or as function decorator):

aliases = {
    "kg": "kilograms",
    "lb": "pounds",
    ...
}

def convert(amount, from_, to):
    from_ = aliases.get(from_, from_)
    to = aliases.get(to, to)
    c = converters.get((from_, to))
    if c is None:
        raise ValueError("converter not found")
return c(amount)

>>> convert(Decimal("100"), "lb", "kg")
Decimal('68.96551724137931034482758621')

With this architecture you can later add other converters to your library and you need to call only convert() function from your UI loop.

Also I don't like your UI. For example if I need to convert 10 values from kilograms to pounds I need to enter value then I need to enter "kilograms". By adding modes to your converter you can save me 10 lines. For example user first enter "kilograms to pounds" and this conversion mode stored and displayed in the prompt. Then user can enter values which will be converted from kilograms to pounds. Later user can change conversion mode by entering "pounds to kilograms".

\$\endgroup\$
2
\$\begingroup\$

I would not recommend you to structure the app this way. A cleaner solution (IMHO) would be to have the values being inputted by arguments to the script so that you call it using ./convert --inch 5.2 or ./convert --cm 5.2. Also to have all functionality mashed together in one function isn't good practice, you should divide input/output and function at least.

I did however make my own version that should be pretty equal as far as functionality goes, although i suspect some things differ. And it is not really a better solution... just different.

#!/usr/bin/python2

def convert(value, frm):
    try:
        amount = int(value)
    except:
        throw Exception("Value is not an numeric")
    return amount * 2.2 if frm == "kilograms" else (1/1.45) if frm == "pounds" else throw Exception("Can only convert from kilograms or pounds")

def ask_user():
    while(True):
        amount = raw_input("Please enter the value: ");
        frm = raw_input("Please choose between converting FROM kilograms/pounds: ")
        try:
            result = convert(amount, frm)
            print "The result is " + result + " " + "pounds" if frm == "kilograms" else "kilograms"
        except Exception as e:
            print e
            print "Please try again"
            continue
        restart = raw_input("Try again? ");
        if restart[0] == 'y':
            continue
        else:
            print "Okey, bye."
            break

if __name__ = '__main__': # This row is to be able to import the file without the question being asked on import.
    ask_user()
\$\endgroup\$

Your Answer

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