Dr Hydralisk
Dr Hydralisk

Reputation: 1181

Learning Python, is there a better way to write this?

I am learning Python (2.7) and to test what I have learned so far I wrote a temperature converter that converts Celsius to Fahrenheit and I wanted to know if my code could be written better to be faster or something more Pythonic. And could someone tell me if there is an actual name for the if __name__ == '__main__': main() (out of curiosity)?

from sys import argv, exit # import argv and exit functions

def to_f(c): # Convert celsius to ferinheight
    temp = (c * 9/5) + 32
    return temp

def to_c(f): # Convert ferinheight to celsius
    temp = (f - 32) * 5/9
    return temp

def main():
    args = argv[1:] # Creates an argument list omitting the omitting the [0] element
    if len(argv) < 2: exit(1) # If less than two arguments
    if args[0] == '-f': # If the first argument is -f
        print args[1], 'ferinheight is', str(to_c(int(args[1]))), 'celsius'
    elif args[0] == '-c': # If the first argument is -c
        print args[1], 'celsius is', str(to_f(int(args[1]))), 'ferinheight'
    else: exit(1)

if __name__ == '__main__':
    main()

http://pastebin.com/rjeNikDt

Upvotes: 4

Views: 1476

Answers (4)

Jesse Dhillon
Jesse Dhillon

Reputation: 7997

import sys
from getopt import getopt, GetoptError

def to_f(c):
  return (c*9/5) + 32

def to_c(f):
  return (f-32) * 5/9

def usage():
  print "usage:\n\tconvert [-f|-c] temp"

def convert(args):
  opts = None
  try:
    opts, args = getopt(args, "f:c:")
  except GetoptError as e:
    print e

  if not opts or len(opts) != 1:
    usage()
    return 1

  converters = {
    '-f': (to_c, '{0} Fahrenheit is {1} Celsius'),
    '-c': (to_f, '{0} Celsius is {1} Fahrenheit')
  }

  # opts will be [('-f', '123')] or [('-c', '123')]
  scale, temp = opts[0][0], int(opts[0][1])
  converter = converters[scale][0]
  output = converters[scale][1]

  print output.format(temp, converter(temp))
  return 0

if __name__ == '__main__':
    sys.exit(convert(sys.argv[1:]))

I used getopt to clean up your argument and error handling. I also consolidated the logic that acts upon the given option, once that option has been determined. Getopt is a very powerful option parser and I think it's worth learning if you are going to be writing these sorts of programs often.

Upvotes: 1

John La Rooy
John La Rooy

Reputation: 304463

A couple of improvements on Ned's answer. In Python2.7 / still truncates the result by default, so you need to import division from __future__ otherwise (c * 9/5) + 32 always rounds down which leads to reduced accuracy.
eg if 36C is 96.8F it's better to return 97 than 96

You don't need a return statement in convert. By default None is returned. If there is a problem you can raise an exception

Also using "".format() is preferred nowdays

A further improvement would be to use optparse or similar to process the command arguments, but may be overkill for such a simple program

from __future__ import division
import sys

def to_f(c): # Convert celsius to fahrenheit
    return (c * 9/5) + 32

def to_c(f): # Convert fahrenheit to celsius
    return (f - 32) * 5/9

def convert(args):
    if len(args) != 2:
        raise RuntimeError("List of two elememts required")
    t = int(args[1])
    if args[0] == '-f': # If the first argument is -f
        print "{0} Fahrenheit is {1} Celsius".format(t, round(to_c(t)))
    elif args[0] == '-c': # If the first argument is -c
        print "{0} Celsius is {1} Fahrenheit".format(t, round(to_f(t)))
    else:
        raise RuntimeError("First element should be -c or -f")

if __name__ == '__main__':
    sys.exit(convert(sys.argv[1:]))

Upvotes: 3

Ned Batchelder
Ned Batchelder

Reputation: 376022

import sys

def to_f(c): # Convert celsius to fahrenheit
    return (c * 9/5) + 32

def to_c(f): # Convert fahrenheit to celsius
    return (f - 32) * 5/9

def convert(args):
    if len(args) < 2:
        return 1 # If less than two arguments
    t = args[1]
    if args[0] == '-f': # If the first argument is -f
        print "%s Fahrenheit is %s Celsius" % (t, to_c(int(t)))
        return 0
    elif args[0] == '-c': # If the first argument is -c
        print "%s Celsius is %s Fahrenheit" % (t, to_f(int(t)))
        return 0
    else:
        return 1

if __name__ == '__main__':
    sys.exit(convert(sys.argv[1:]))

What I did:

  1. Changed the name of main() to convert()
  2. Pass the arguments to convert() explicitly
  3. Change calls to exit() to returns, and call exit() in the main clause.
  4. You were checking argv for length 2, when you should have been checking args.
  5. The to_c and to_f functions don't need a temp variable, just return the expression.
  6. Although others are right that you can just put the main() function at the top level, it is good form to use the if __name__ style, so that you could import this module and use the functions in other code.
  7. String formatting is nicer than intermixing strings and values in the print statement.
  8. args[1] appears enough that I assigned it to t for brevity.
  9. I prefer importing sys, and using sys.argv, for example.
  10. I always put dependent clauses on new lines, never if blah: doit()
  11. Fix the spelling of Fahrenheit

Upvotes: 11

Forrest Voight
Forrest Voight

Reputation: 2257

The if __name__ == '__main__': pattern is for when you're writing a module intended to be used by other code, but you want some testing code in the module.

If you run the module directly, it runs the stuff in that if block. If it's imported from somewhere else, it doesn't.

So, I would recommend keeping that if __name__ == '__main__': block because you can do something like:

from temp_conv import c_from_f
print c_from_f(73)

later in another piece of code if you named this temp_conv.py.

Upvotes: 4

Related Questions