6
\$\begingroup\$

I'm a student currently studying computing/programming at school and am a beginner and would like some help understanding how to shorten down my code. I know some basic Python and can program relatively simple programs such as the one I need help with.My task/homework was to create a metric-imperial and vice-versa conversion calculator. I haven't quite finished it yet but how would I shorten down this long and repetitive code?

 #Start
 print("Welcome to the Conversion-Calculator")

In this program, "_" means "to" eg. "km_mi" is kilometres to miles

 #created the menus for the length 
 metric_length=["'km', 'm' , 'cm'"]
 imperial_length=["'mi', 'ft', 'in'"]

 #This defines variables from metric to imperial measurements of distance 
 def km_mi():
     print(leng1/1.6, "miles")
 def m_ft():
     print(leng1*3.26, "feet")
 def cm_in():
     print(leng1/2.54, "inches")

 #This list defines variables from imperial to metric measurements of distance
 def mi_km():
     print(leng2*1.6, "kilometers")
 def ft_m():
     print(leng2/3.26, "metres")
 def in_cm():
     print(leng2*2.54, "centimetres")

 #menu for weights
 metric_mass=["'t','kg', 'g'"]
 imperial_mass=["'stone', 'lb', 'oz'"]

 #this defines variables from metric to imperial in mass
 def t_st():
     print(mass1*157.47, "stone")
 def kg_lb():
     print(mass1*2.2, "pounds")
 def g_oz():
     print(mass1*0.035, "ounces")
 #this defines variables from imperial to metric in mass
 def st_t():
     print(mass2*0.00635, "tones")
 def lb_kg():
     print(mass2*0.4536, "kilograms")
 def oz_g():
     print(mass2*28.35, "grams")


 #creates startup menu
 menu_type=["'Length-1', 'Weight-2', 'Cacpacity-3'"]

 #choose which what to convert
 print(menu_type)
 measure=int(input("Please select what you would like to convert by typing a number:"))

 #starting if statement chain
 if measure==1:
     #menu to chose from either metric to imperial or vice-versa
     print("Metric-Imperial - 1\n")
     print("Imperial-Metric - 2\n")
     choice=int(input("Choose a conversion by typing a number: "))
     #If 1 is chosen follows by asking what metric distance to convert
     if choice== 1:
         print("Please choose the measurement you want to use.\n")
         #prints the metric length menu
         print(metric_length)
         distance =input("Type an option from the menu: ")

         #if statements that takes user input to assign a value for leng1
         #then runs the function
         if distance=="km":
             leng1=int(input("Please enter a number to convert: "))
             print=(km_mi())
         elif distance=="m":
             leng1=int(input("Please enter a number to convert: "))
             print=(m_ft())
         elif distance=="cm":
             leng1=int(input("Please enter a number to convert: "))
             print=(cm_in())
         else:
             print("Invalid Entry")

     #This is the code for the second choice in the program
     #If 2 is chosen follows by asking what imperial distance to convert
     if choice== 2:
         print("Please choose the measurement you want to use.\n")
         #prints the imperial length menu
         print(imperial_length)
         distance2 =input("Type an option from the menu: ")

         #if statements that takes user input to assign a value for leng2
         #then runs the function
         if distance2=="mi":
             leng2=int(input("Please enter a number to convert: "))
             print=(mi_km())
         elif distance2=="ft":
             leng2=int(input("Please enter a number to convert: "))
             print=(ft_m())
         elif distance2=="in":
             leng2=int(input("Please enter a number to convert: "))
             print=(in_cm())
         else:
             print("Invalid Entry")

 if measure==2:
     #menu to chose from either metric to imperial or vice-versa
     print("Metric-Imperial - 1\n")
     print("Imperial-Metric - 2\n")
     choice2=int(input("Choose a conversion by typing a number: "))
     #If 1 is chosen follows by asking what metric distance to convert
     if choice2== 1:
         print("Please choose the measurement you want to use.\n")
         #prints the metric length menu
         print(metric_mass)
         weight =input("Type an option from the menu: ")

         #if statements that takes user input to assign a value for mass1
         #then runs the function
         if weight=="t":
             mass1=int(input("Please enter a number to convert: "))
             print=(t_st())
         elif weight=="kg":
             mass1=int(input("Please enter a number to convert: "))
             print=(kg_lb())
         elif weight=="g":
             mass1=int(input("Please enter a number to convert: "))
             print=(g_oz())
         else:
             print("Invalid Entry")

     #This is the code for the second choice in the program
     #If 2 is chosen follows by asking what imperial mass to convert
     if choice2== 2:
         print("Please choose the measurement you want to use.\n")
         #prints the imperial length menu
         print(imperial_mass)
         weight2 =input("Type an option from the menu: ")

         #if statements that takes user input to assign a value for mass2
         #then runs the function
         if weight2=="stone":
             mass2=int(input("Please enter a number to convert: "))
             print=(st_t())
         elif weight2=="lb":
             mass2=int(input("Please enter a number to convert: "))
             print=(lb_kg())
         elif weight2=="oz":
             mass2=int(input("Please enter a number to convert: "))
             print=(oz_g())
         else:
             print("Invalid Entry")
\$\endgroup\$
4
  • \$\begingroup\$ Have you considered simply taking the input as "15km"? In which case, you don't need all that menu stuff. If "15km" is given, convert it to miles. If "20kg" is given, convert it to lb. If "100oz"is given, convert it to grams. etc. \$\endgroup\$
    – wookie919
    Commented Oct 16, 2014 at 20:05
  • 1
    \$\begingroup\$ For a first step, you may want to look at using a "function table": a dict that maps from a key to a function. In this case, for example, the key ('g', 'oz') would map to the function g_oz. Then, instead of all that if/elif stuff, you just do functions[source, target](). \$\endgroup\$
    – abarnert
    Commented Oct 16, 2014 at 20:11
  • \$\begingroup\$ For a second step, instead of using global variables for leng1 and the like, pass them as parameters to the functions. This won't make things that much shorter, but it will make them a whole lot simpler to understand, and to refactor. \$\endgroup\$
    – abarnert
    Commented Oct 16, 2014 at 20:12
  • 2
    \$\begingroup\$ For a third step, do you actually need functions at all? Really, they all do the same thing: multiply an argument by a constant and print it out. So, instead of a function table, just have a conversion constant table. Then, instead of functions[source, target](value) it's just print(conversions[source, target] * value, target). \$\endgroup\$
    – abarnert
    Commented Oct 16, 2014 at 20:14

4 Answers 4

5
\$\begingroup\$

There's too much code for me to do a line-by-line review, so here are a few general comments:

  • Read PEP 8, the Python style guide. Spaces around your binary operators will make reading your code easier.

  • Your <system>_<quantity> variables seem odd to me. For example:

    metric_length=["'km', 'm' , 'cm'"]
    

    This is creating a list with a single element, and that element is the string 'km', 'm' , 'cm'. I think what you actually mean is to have a list of three elements like this:

    metric_length_units = ['km', 'm', 'cm']
    

    (Note also the change in variable name: this list contains unit names, not lengths.)

  • The functions km_mi(), m_ft(), and so on, don’t take any arguments, but the body includes a lengX variable. What if that isn’t set in the global scope? Instead, this should be passed in as an argument to the function.

  • Cacpacity is misspelt.

  • Every conversion factor appears at least twice. For example, 1.6 occurs in both km_mi() and mi_km(). One way to pull this out is to define a general purpose conversion function:

    def convert_a_to_b(value, unit_a, unit_b):
        return value * conversion_rate(unit_a, unit_b)
    

    where conversion_rate looks up the conversion rate (e.g. from a dict) that’s stored in one place. You could then define wrappers around this if you want, or just use it as-is.

    Then take the value from this function, and drop that into a print string. Don’t print it directly: it makes it harder to pass values between different converters, and harder to test at a later stage.

  • I think I know what you meant to do here, but it’s wrong:

    print=(cm_in())
    

    I assume that stray equals sign snuck in by mistake. If not, you’re trying to assign to the variable print, which will fail because print is a reserved keyword.

  • You have the same code to ask the user to choose Imp->Met or Met->Imp at least twice. As you add more units, this will get duplicated again. Consider wrapping it in a function.

  • What happens if measure == 3?

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

One idea: Combine all your similar functions into one function and use parameters in your calls ...

def convert_multiply(mass, factor, label): 
    print (mass*factor, label)

Then, you can have a dictionary with the factors and labels ...

   factor_labels = {'t': [157.47, "stone"], 
                    'kg': [2.2, "pounds"],
                     ...
                    }
\$\endgroup\$
4
\$\begingroup\$

A few things you can do:

  1. Whenever you have to execute a few things after a loop (a conditional branch execution), this is the prime candidate to use a dictionary. In Python, functions are just names of code blocks; so you can use them anywhere you can use any other variable.

  2. You only need to ask for the unit and the amount once after the correct selection has been made.

Here is a version of your code that does this; I have left your comments in place:

metric_length=['km', 'm' , 'cm']
imperial_length=['mi', 'ft', 'in']
menu_type=['Length-1', 'Weight-2', 'Cacpacity-3']
metric_mass=['t','kg', 'g']
imperial_mass=['stone', 'lb', 'oz']

converters = dict()
converters['km'] = lambda x: '{} {}'.format(x/1.6, 'miles')
converters['m'] = lambda x: '{} {}'.format(x*3.26, 'feet')
converters['cm'] = lambda x: '{} {}'.format(x/2.54, 'inches')
converters['mi'] = lambda x: '{} {}'.format(x*1.6, 'kilometers')
converters['ft'] = lambda x: '{} {}'.format(x/3.26, 'meters')
converters['in'] = lambda x: '{} {}'.format(x*2.54, 'centimeters')
converters['t'] = lambda x: '{} {}'.format(x*157.47, 'stone')
converters['kg'] = lambda x: '{} {}'.format(x*2.2, 'pounds')
converters['g'] = lambda x: '{} {}'.format(x*0.035, 'ounces')
converters['stone'] = lambda x: '{} {}'.format(x*0.00635, 'tonnes')
converters['lb'] = lambda x: '{} {}'.format(x*0.4536, 'kilograms')
converters['oz'] = lambda x: '{} {}'.format(x*28.35, 'grams')

print("Welcome to the Conversion-Calculator")
print('\n'.join(menu_type))
measure=int(input("Please select what you would like to convert by typing a number:"))

#starting if statement chain
if measure==1:
 #menu to chose from either metric to imperial or vice-versa
 print("Metric-Imperial - 1")
 print("Imperial-Metric - 2")
 choice=int(input("Choose a conversion by typing a number: "))

 #If 1 is chosen follows by asking what metric distance to convert
 if choice== 1:
     print('\n'.join(metric_length))    
 #This is the code for the second choice in the program
 #If 2 is chosen follows by asking what imperial distance to convert
 if choice== 2:
     print('\n'.join(imperial_length))
     #prints the imperial length menu


if measure==2:
 #menu to chose from either metric to imperial or vice-versa
 print("Metric-Imperial - 1\n")
 print("Imperial-Metric - 2\n")
 choice2=int(input("Choose a conversion by typing a number: "))
 #If 1 is chosen follows by asking what metric distance to convert
 if choice2== 1:
     print('\n'.join(metric_mass))
     #prints the metric length menu
 #This is the code for the second choice in the program
 #If 2 is chosen follows by asking what imperial mass to convert
 if choice2== 2:
     print('\n'.join(imperial_mass))

option = input("Please choose the measurement you want to use: ") 
if option not in converters.keys():
    print('Invalid Entry')
else:
    leng1=int(input("Please enter a number to convert: "))
    print(converters[option](leng1))

I also made the method names the same as the inputs, this way if the user enters something invalid, the key in the converters dictionary will not exist - an "cheap" way to get the validation done.

The main line that does the execution is here:

print(converters[option](leng1))

It is fetching the value from converters, which is a lamdba (a function you can write as an expression), and then calls it with the value entered by the user.

Here is a simplified example, because this is the main part that did the "shrinking" of your code.

Lets say you have two methods, a and b, and depending on what the user selects, you have to call either a or b. You might write some code like this:

def a():
  return 'Hello'

def b():
  return 'Goodbye'

user_request = input('Please enter either a or b: ')
if user_request == 'a':
   print(a())
if user_request == 'b':
   print(b())

This is fine if its two options, but what if there are 3, 4 or 9 options? Now you'll end up with a large chain of if/else statements. To prevent this, what you can do is use a call map - which is a dictionary that has the values as the method names:

call_map = dict()
call_map['a'] = a
call_map['b'] = b

As methods are just like normal values, you can assign and pass them around. Now what you want to do is call the method that corresponds to the input:

option = 'a'
function_to_call = call_map[option]
result = function_to_call()

You can shorten the above with:

result = call_map[option]()

As the dictionary only has keys for those values that map to valid functions, it is easy to filter out an invalid entry - it simply won't be in the list of keys for that dictionary.

if option not in call_map.keys():
    print('{} is not a valid option'.format(option))
else:
    print('The result is: {}'.format(call_map[option]())
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for this it was really helpful, but I dont uderstand how you used the curly braces and the ".format" function with the lamda function and added a string to it. How does this work and what does the .format do? \$\endgroup\$
    – Random Guy
    Commented Oct 23, 2014 at 18:22
  • \$\begingroup\$ Have a look at string formatting in the Python manual. \$\endgroup\$ Commented Oct 25, 2014 at 14:39
2
\$\begingroup\$

I see two obvious things you can do but I don't have the time to look into it in detail:

1: You try to assign a value to print. That causes an error.

2: You repeat equal lines of code in each condition of your if statements. Don't do that. Use one line that precedes all these if-statements. For example:

    mass1=int(input("Please enter a number to convert: "))
    if weight=="t":
        print=(t_st())
    elif weight=="kg":
        print=(kg_lb())
    elif weight=="g":
        print=(g_oz())
    else:
        print("Invalid Entry")

If you don't want to read input in case the entry was wrong, you can use:

 if weight not in {'t','kg','g'}:
     print("Invalid Entry")
     return
 else:
     mass1=int(input("Please enter a number to convert: "))
     if weight=="t":
         print=(t_st())
     elif weight=="kg":
         print=(kg_lb())
     elif weight=="g":
         print=(g_oz())
\$\endgroup\$

Your Answer

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

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