Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My son (9) is learning Python and wrote this temperature conversion program. He can see that there is a lot of repetition in it and would like to get feedback on ways to make it shorter.

def get_temperature():
    print "What is your temperature?"
    while True:
        temperature = raw_input()
        try:
            return float(temperature)
        except ValueError:
            print "Not a valid temperature. Could you please do this again?"    

print "This program tells you the different temperatures."
temperature = get_temperature()

print "This is what it would be if it was Fahrenheit:"
Celsius = (temperature - 32)/1.8
print Celsius, "degrees Celsius"
Kelvin = Celsius + 273.15
print Kelvin, "Kelvin"

print "This is what it would be if it was Celsius."    
Fahrenheit = temperature * 9 / 5 + 32
print Fahrenheit, "degrees Fahrenheit"
Kelvin = temperature + 273.15
print Kelvin, "Kelvin"

print "This is what it would be if it was Kelvin"    
Celsius = temperature - 273.15
print Celsius, "degrees Celsius"
Fahrenheit = Celsius * 9 / 5 + 32
print Fahrenheit, "degrees Fahrenheit"
share|improve this question
    
Out of curiosity: why didn't your son ask here himself? –  svick Jan 19 '13 at 12:56
5  
    
@vikingosegundo I did not know that. –  svick Jan 19 '13 at 13:12
2  
@svick I think it is quite common to exclude kids younger than 13. So maybe this 9-yo is very clever and poses as his father :) –  vikingosegundo Jan 19 '13 at 13:20
3  
I just realized the delightful detail of not adding 'degrees' in front of 'Kelvin'! I've had professors in college doing it wrong all the time... –  Jaime Jan 20 '13 at 2:09

4 Answers 4

up vote 3 down vote accepted

The easiest way to remove repetition here is with nested for loops, going through all the combinations of units. In the code below I also use the fact that you can convert between any two scales in the following way:

  1. subtract something then divide by something to get back to Celsius, then
  2. multiply by something then add something to get to the desired unit

The "something"s can be stored in a single tuple (list of values), making the conversions simpler.

scales = ('Celsius', 'degrees ', 1, 0), ('Farenheit', 'degrees ', 1.8, 32), ('Kelvin', '', 1, 273.15)
value = get_temperature()
for from_unit, _, slope1, intercept1 in scales:
    print "This is what it would be if it was", from_unit
    celsius = (value - intercept1) / slope1
    for to_unit, prefix, slope2, intercept2 in scales:
        if to_unit != from_unit:
            print '{0} {1}{2}'.format(intercept2 + slope2 * celsius, degrees, to_unit)

Dividing code into functions is of course a good idea in general, but not that useful in this case. If he's interested in performing other operations on temperatures it might be interesting to go a step further and make a simple class like the following.

scales = {
            'Celsius': ('degrees ', 1, 0),
            'Farenheit': ('degrees ', 1.8, 32),
            'Kelvin': ('', 1, 273.15)
        }
class Temperature:    
    def __init__(self, value, unit = 'Celsius'):
        self.original_unit = unit
        _, slope, intercept = scales[unit]
        self.celsius = (value - intercept) / slope
    def to_unit(self, unit):
        _, slope, intercept = scales[unit]
        return slope * self.celsius + intercept
    def print_hypothetical(self):
        print "This is what it would be if it was", self.original_unit
        for unit, (prefix, _, _) in scales.iteritems():
            if unit != self.original_unit:
                print "{0} {1}{2}".format(self.to_unit(unit), prefix, unit)
value = get_temperature()
for unit in scales:
    m = Temperature(value, unit)
    m.print_hypothetical()
share|improve this answer

Some of what I am going to write may be a little too much for a 9 year old, but if he was able to produce such neat, structured code on his own, he should be ready for some further challenge.

So to make things less repetitive, all you have to do is figure out how to make what appears different to share a common framework. For the temperature conversions, they are all of the form:

return_value = (input_value + a) * b + c

Lazy programmers are better programmers, so you don´t want to figure out all six possible transformations by hand, so note that the reverse transformation has the form:

input_value = (return_value - c) / b - a

Furthermore, even specifying three of six transformations is too many, since you can always convert, e.g. from Celsius to Kelvin and then to Fahrenheit. So we just need to choose a BASE_UNIT and encode a transformation to or from it to all other. This will also make it a breeze to include Rankine degrees or any other of the weird scales if you would so desire in the future.

So the code below is probably going to end up being longer than what you posted, but the main program is just four lines of code, there's quite a lot of error checking, and it would be a breeze to update to new units:

UNITS = 'fck'
BASE_UNIT = 'k'
UNIT_NAMES = {'f' : 'Fahrenheit', 'c' : 'Celsius', 'k' : 'Kelvin'}
CONVERSION_FACTORS = {'fk' : (-32., 5. / 9., 273.16),
                      'ck' : (273.16, 1., 0.)}

def get_temperature():
    print "What is your temperature?"
    while True:
        temperature = raw_input()
        try:
            return float(temperature)
        except ValueError:
            print "Not a valid temperature. Could you please do this again?"

def convert_temp(temp, unit_from, unit_to) :
    if unit_from != BASE_UNIT and unit_to != BASE_UNIT :
        return convert_temp(convert_temp(temp, unit_from, BASE_UNIT),
                            BASE_UNIT, unit_to)
    else :
        if unit_from + unit_to in CONVERSION_FACTORS :
            a, b, c = CONVERSION_FACTORS[unit_from + unit_to]
            return (temp + a) * b + c
        elif unit_to + unit_from in CONVERSION_FACTORS :
            a, b, c = CONVERSION_FACTORS[unit_to + unit_from]
            return (temp - c) / b - a         
        else :
            msg = 'Unknown conversion key \'{0}\''.format(unit_from + unit_to)
            raise KeyError(msg)

def pretty_print_temp(temp, unit_from) :
    if unit_from not in UNITS :
        msg = 'Unknown unit key \'{0}\''.format(unit_from)
        raise KeyError(msg)
    txt = 'This is what it would be if it was {0}'
    print txt.format(UNIT_NAMES[unit_from])
    for unit_to in UNITS.replace(unit_from, '') :
        print '{0:.1f} degrees {1}'.format(convert_temp(temp, unit_from,
                                                        unit_to),
                                           UNIT_NAMES[unit_to])

if __name__ == '__main__' :

    print "This program tells you the different temperatures."
    temperature = get_temperature()

    for unit_from in UNITS :
        pretty_print_temp(temperature, unit_from)

EDIT As an aside, for minimal length without modifying the logic of the original code, the following would work:

print "This program tells you the different temperatures."
temperature = get_temperature()
text = ('This is what it would be if it was {0}:\n'
        '{1:.1f} {2}\n{3:.1f} {4}')
print 
Celsius = (temperature - 32) / 1.8
Kelvin = Celsius + 273.15
print text.format('Fahrenheit', Celsius, 'degrees Celsius', Kelvin, 'Kelvin')
Fahrenheit = temperature * 9 / 5 + 32
Kelvin = temperature + 273.15
print text.format('Celsius', Fahrenheit, 'degrees Fahrenheit', Kelvin,
                  'Kelvin')
Celsius = temperature - 273.15
Fahrenheit = Celsius * 9 / 5 + 32
print text.format('Kelvin', Celsius, 'degrees Celsius', Fahrenheit,
                  'Fahrenheit')
share|improve this answer
2  
A solution like this might make sense if you wanted to convert between many different units. But if you expect that you won't need any unusual units, this code is way too overengineered. Besides the question is how to make the program shorter, you made it longer and much more complicated. –  svick Jan 19 '13 at 20:17
    
Your temperature conversions could get lazier still, since they are actually all of the form return_value = input_value * a + c –  Stuart Jan 19 '13 at 23:10
    
@svick I'd argue that the program is actually much, much shorter: 4 lines of code. It's the temperature conversion library functions that take up more space... In any real production environment in which you had to do all six possible conversions, I really don't see coding six different functions as a good design practice. –  Jaime Jan 19 '13 at 23:31
1  
@Jaime For me “a program” is all code I have to develop and maintain for it. And that's not 4 lines in your case. And I really do think that 6 one-line functions are better than one big, that is hard to understand. –  svick Jan 20 '13 at 0:19
1  
I would say the problem is more that convert_temp is needlessly complicated and could be reduced to a few lines and made easier to read at the same time pastebin.com/jQkmR8Rz –  Stuart Jan 20 '13 at 1:47

He should define functions that he can re-use:

def fahrenheit_from_celsius(temprature):
    return temperature * 9 / 5 + 32


print fahrenheit_from_celsius(get_temperature())
share|improve this answer

The most repetition in that program comes from printing the converted temperatures. When a program contains repeated code, the most common solution is to refactor the code by extracting a function. The new function would contain the common parts, and use its parameters for the differing parts.

In your case, the function could look like this:

def print_temperatures(original_unit, first_value, first_unit, second_value, second_unit):
    print "This is what it would be if it was", original_unit
    print first_value, first_unit
    print second_value, second_unit

The code to print the temperatures (assuming the differences in punctuation are not intentional):

print "This program tells you the different temperatures."
temperature = get_temperature()

Celsius = (temperature - 32)/1.8
Kelvin = Celsius + 273.15
print_temperatures("Fahrenheit", Celsius, "degrees Celsius", Kelvin, "Kelvin")

Fahrenheit = temperature * 9 / 5 + 32
Kelvin = temperature + 273.15
print_temperatures("Celsius", Fahrenheit, "degrees Fahrenheit", Kelvin, "Kelvin")

Celsius = temperature - 273.15
Fahrenheit = Celsius * 9 / 5 + 32
print_temperatures("Kelvin", Celsius, "degrees Celsius", Fahrenheit, "degrees Fahrenheit")

This still leaves some repetition in the temperature conversions themselves. This could be again solved by extracting a function for each conversion. Another solution would be to encapsulate all the conversions into a class. That way, your code could look something like:

temperature_value = get_temperature()

temperature = Temperature.from_fahrenheit(temperature_value)
print_temperatures("Fahrenheit", temperature.Celsius, "degrees Celsius", temperature.Kelvin, "Kelvin")

But creating a class would make more sense if conversions were used more, it probably isn't worth it for this short program.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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