14
\$\begingroup\$

I have been teaching myself Python (as well as Tkinter) and today one of the sites I was reading, asked us to write a program to covert one value from Fahrenheit to Celsius, once - an easy three line program. As you can read in the comments for the program below, I wanted to expand on this.

This program is one of the most complex and best organized for me thus far. I have feed it through a PEP8 program and it returned no errors, and the program itself runs as I intended and I haven't crashed it when passing errors.

I would be grateful for any comments on the structure of the program, as well as areas I can improve and/or streamline the code - any and all constructive feedback is welcomed!

#!/usr/bin/python
"""
Program: Temperature Coversion (C to F, or F to C)
Date:    02 May 2019
Author:  Jason P. Karle
Remark:  This program was inspired by a Python exercise that
asks you to create a program that will convert one Celsius value to Fahrenheit;
so a program that can be executed with three lines of code.
However, I wanted to make something that would allow the user to
convert to and from either C of F, and do so multiple times, until the user
decides to end the program. This was also an exercise for me to
advance not only my code skills, but how I structure a program.
"""


def quitContinue():
    print("\nDo you want to:\n")
    print("     1. Make another conversion; or")
    print("     2. Exit the program?\n")
    answer = input("Make you selection: ")
    try:
        if answer == "1":
            mainProg()
        else:
            return
    except:
        print("That is not a valid choice.")
        quitContinue()


def CtoF_Calc():
    print("\nThank you, please enter the")
    print("value you want to convert.")
    print("Enter a value between -273.5°C to")
    print("+5.5 dectillion °C")
    value = float(input(": "))
    try:
        if value < -273.5 or value > 5.5**30:
            print("That is not a valid range.")
            celciusCalc()
        else:
            answer = (value*(9/5))+32
            print(f"{value}°C equals: {answer}°F")
            quitContinue()
    except:
        print("Please entet a number!")
        CtoF_Calc()


def FtoC_Calc():
    print("\nThank you, please enter the")
    print("value you want to convert.")
    print("Enter a value between -273.5°C to")
    print("+5.5 dectillion °C")
    value = float(input(": "))
    try:
        if value < -459.5 or value > 42**30:
            print("That is not a valid entry.")
            celciusCalc()
        else:
            answer = (5/9)*(value-32)
            print(f"{value}°F equals: {answer}°C")
            quitContinue()
    except:
        print("That is not a number!\n")
        FtoC_Calc


def makeSelection(selection):
    try:
        if selection == "1":
            CtoF_Calc()
        elif selection == "2":
            FtoC_Calc()
        else:
            return
    except:
        print("That is not a valid selection")
        makeSelection(selection)


def mainProg():
    print("Please enter the number")
    print("corresponding to what you")
    print("want to convert:")
    print("     1. Celcius to Farenheit")
    print("     2. Farenheit to Celcius")
    print("     3. Exit\n")
    selection = input("Enter 1, 2 or 3: ")
    makeSelection(selection)


if __name__ == "__main__":
    print("Welcome to the temperature")
    print("conversion program!\n")
    mainProg()
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Mast
    May 3, 2019 at 15:00
  • \$\begingroup\$ Understood, because that one was a save/version error, and never should have made it to this post, I mistakenly thought that my action was okay provided that I made note of it in the edit comments. All other comments and suggestions have not and will not be actioned in this post. I have now read (and bookmarked!) the link you provided, thank you! \$\endgroup\$
    – J Karle
    May 3, 2019 at 15:31
  • \$\begingroup\$ No problem, it's confusing for plenty of new users. You get used to it. Feel free to ask a new question once your code has been improved. \$\endgroup\$
    – Mast
    May 3, 2019 at 17:39
  • 3
    \$\begingroup\$ Thank you, everyone, for the quick and fulsome answers - awesome insight and very helpful information. I am digesting all the information I received and will begin a revised version that i look forward to posting as a new question this week for your feedback. Some of the suggested changes flew right over my head (I am researching them today!) and some elicited questions, which I will post to you specifically over the next few days. My heartfelt thanks, what a great welcome to this cleary supportive community! \$\endgroup\$
    – J Karle
    May 5, 2019 at 15:43
  • \$\begingroup\$ value < -273.5 or value > 5.5**30 can be replaced with -273.5 < value < 5.5**30 which to my eyes is a little more readable. \$\endgroup\$
    – bfris
    Sep 16, 2020 at 16:30

3 Answers 3

14
\$\begingroup\$

At first glance your code looks nice and clean. And even though the naming goes against PEP8 would be near perfect style.

I then entered your code into PyCharm, and then and there I knew this was unfortunately a facade.

Firstly I don't think you've set your linter up correctly. As within seconds I knew your code wasn't PEP8 compliant due to using camel case function names.

I have feed it through a PEP8 program and it returned no errors

  1. PEP 8 convention is to use snake_case rather than quitContinue or CtoF_Calc.
  2. You have a couple of miss-spelt words, Prog and "entet".
  3. It's advised against having bare except statements as they catch too many errors and lead to hard to debug errors. This can actually be seen in your program, it's hidden a bug that you probably don't know about.
  4. celciusCalc is undefined, and so is a bug in your program.
  5. PEP8 and linters are quite finicky when it comes to whitespace. value*(9/5) should have spaces either side of the operators.
  6. You have a 'pointless statement', FtoC_Calc. This is as you forgot to call the function. And so causes a bug in your program.

And so I recommend you look into configuring your linter to get the maximum amount of warnings possible. I personally use Prospector and Flake 8 with a butt tone of plugins.


  1. In FtoC_Calc you state the range in celsius, which is confusing. If I'm using your program I likely won't know what the equivalent in Fahrenheit is.
  2. Keep the code in the try statement to be as small as possible. Looking at the except it looks like the purpose is to handle when you don't enter floating point integer values.

    1. You haven't put the call to float in the try and so you have another bug.
    2. You should use except ValueError.
    3. You can put all the code that is currently in the try in an else statement.
    4. You should replace celciusCalc() with FtoC_Calc and CtoF_Calc.
  3. Currently your design is sub-optimal, FtoC_Calc interacts with the user and performs the mathematical calculations.

    It also uses recursion rather than loops to cycle through the function calls, leading not only to spaghetti code, but toward getting a RuntimeError where you exceed the recursion depth.

    This is all rather easy to deal with. Split the function into three distinct functions:

    1. One that gets a floating point integer from the user. And handles interactions with the user if they enter an incorrect value.

      If a user enters an incorrect value you'll want to have your try statement in a while loop to continuously ask for input.

      By giving a range or an 'is_invalid' function to this function you can reduce the amount of duplicated code.

    2. The function to convert C to F.

    3. The function that calls both of these functions.
  4. The majority of your code in make_selection is not needed. No exceptions should be raised from these functions, and it isn't the correct place to handle them.

  5. quit_continue should be changed to a function that returns a boolean. This can be used in mainProg to determine if the user will continue using the program or exit.

    This means mainProg should contain a while loop to continuously allow the user to enter values they want to convert.

    It should be noted that quit_continue shouldn't need the try and should never reach the except. However adding more code the way you did would make this assumption to be less safe as the program becomes more and more problematic.

  6. I changed your string delimiters as one of my tools errors on ", as I commonly use '. Using " is perfectly acceptable.

#!/usr/bin/python
"""
Program: Temperature Coversion (C to F, or F to C)
Date:    02 May 2019
Author:  Jason P. Karle
Remark:  This program was inspired by a Python exercise that
asks you to create a program that will convert one Celsius value to Fahrenheit;
so a program that can be executed with three lines of code.
However, I wanted to make something that would allow the user to
convert to and from either C of F, and do so multiple times, until the user
decides to end the program. This was also an exercise for me to
advance not only my code skills, but how I structure a program.
"""


def input_float(prompt, is_invalid):
    while True:
        try:
            value = float(input(prompt))
        except ValueError:
            print('That is not a number!')
        else:
            if is_invalid(value):
                print('That is not a valid number.')
                continue
            return value


def c_to_f(value):
    return (value * (9 / 5)) + 32


def f_to_c(value):
    return (5 / 9) * (value - 32)


def convert_c_to_f():
    print('\nThank you, please enter the')
    print('value you want to convert.')
    print('Enter a value between -273.5°C to')
    print('+5.5 dectillion °C')
    celsius = input_float(': ', lambda v: v < -273.5 or 5.5**30 < v)
    fahrenheit = c_to_f(celsius)
    print(f'{celsius}°C equals: {fahrenheit}°F')


def convert_f_to_c():
    print('\nThank you, please enter the')
    print('value you want to convert.')
    print('Enter a value between -459.5°F to')
    print('+42 dectillion °F')
    celsius = input_float(': ', lambda v: v < -459.5 or 42**30 < v)
    celsius = f_to_c(fahrenheit)
    print(f'{fahrenheit}°F equals: {celsius}°C')


def quit_continue():
    print('\nDo you want to:\n')
    print('     1. Make another conversion; or')
    print('     2. Exit the program?\n')
    answer = input('Make you selection: ')
    return answer == '1'


def main():
    while True:
        print('Please enter the number')
        print('corresponding to what you')
        print('want to convert:')
        print('     1. Celsius to Fahrenheit')
        print('     2. Fahrenheit to Celsius')
        print('     3. Exit\n')
        selection = input('Enter 1, 2 or 3: ')
        if selection == '1':
            convert_c_to_f()
        elif selection == '2':
            convert_f_to_c()
        else:
            return
        if not quit_continue():
            return


if __name__ == '__main__':
    print('Welcome to the temperature')
    print('conversion program!\n')
    main()

\$\endgroup\$
14
  • \$\begingroup\$ It would probably be better to have a input_float(prompt, range=(-float("inf"), float("inf"))) function that can handle both Celsius and Fahrenheit input. Also in convert_f_to_c() you give the allowed range in Celsius instead of Fahrenheit. \$\endgroup\$
    – Graipher
    May 3, 2019 at 12:25
  • \$\begingroup\$ @Graipher Yeah that's a good suggestion, I thought there was a way to do that, but I was too tired when answering. As for the second thing, this is a problem in the original code too, do you want to write an answer for it? \$\endgroup\$
    – Peilonrayz
    May 3, 2019 at 15:43
  • \$\begingroup\$ Ah didn't even see that in the OP. Nah, I think the current answers cover everything I really care about. However it seems like this is not the right way around or the naming is confusing: if is_valid(value): print('That is not a valid number.') \$\endgroup\$
    – Graipher
    May 3, 2019 at 16:17
  • 1
    \$\begingroup\$ @Graipher That's cool, I'll add it to mine. Oh shoot, yeah that's meant to be the inverse D: \$\endgroup\$
    – Peilonrayz
    May 3, 2019 at 16:23
  • \$\begingroup\$ Yeah, feel free to do so :) \$\endgroup\$
    – Graipher
    May 3, 2019 at 16:23
10
\$\begingroup\$

While there are a lot of things that could be fixed or improved in your program, the biggest problem with it is your use of functions and control flow.

One thing that was not at all obvious to me at first glance is that your program runs in a loop, requesting input and giving conversions until the user is done. Consider what another developer needs to read to determine that control flow in your program:

  1. Top-level module code that calls mainProg()
  2. mainProg() which calls makeSelection()
  3. makeSelection(), where it appears (though not in an obvious way) as if it is supposed to loop (via a recursive call) until a valid selection is entered. (This does not work; more on this later.)
  4. One of CtoF_Calc() or FtoC_Calc() (well, both really, if you want to make sure of what's going on in both cases) where you read through some moderately complex control flow to see that the exit is eventually via quitContinue().
  5. At this point your reader may have the idea from the name, or he may read through quitContinue() to see that it could either exit (unwinding the long stack of functions you've called to get to this point) or call mainProg() again, which causes the whole program to loop.

That's a pretty complex procedure, involving reading most of the code of the program, to get the overall control flow!

The idea behind dividing a program into functions is to let developer look at things at a higher level of abstraction, that is, to be able ignore smaller details and look at just the main points. To do this, you need to have those main points together, with only the less important details (for that level) pushed away, which this program does not do. So let's look at how we could do that here.

First, you can divide up the code in any Python script or module into two basic parts: the stuff executed "now" as the interpreter reads through the code, and the stuff stored to be executed later. Code at the "top level" outside of functions is executed immediately:

print("Hello")

will immediately print "Hello" to the output. Anything in a function is stored to be executed later:

def printHello():
    print("Hello.")

does not immediately print "Hello," but waits until the function is called.

The only code in your program that's immediately executed is the if __name__ == "__main__": ... paragraph.

For reasons I won't get into here (but related to importing modules), you want as much code as possible to be stored to execute later so I would change that to just:

if __name__ == '__main__':
    main()

leaving out the print statements from that part; move those to be inside your main() function. Now all you have to do is write functions from this point out.

The main() function should show the flow of control of the program at the highest (most abstract) level. With appropriate function naming, this can be read alone to give the overall idea of how the program works:

def main():
    while True:
        selection = read_selection()
        if selection == 'q':
            return
        elif selection == 'c':
            convert_C_to_F()
        elif selection == 'f':
            convert_F_to_C()
        else:
            print('Invalid selection')

You'll note that here already, though we don't know the details of how any of the conversions work, it's clear that: 1. This program loops until it a decision is made to quit (while True:). 2. It exits (via return from main()) on the user selecting q. 3. On the user selecting c or f it does the conversion and (because there's no return for either of these) loops to read another selection. 4. An invalid selection prints and error and, again with no return, loops.

In other words, we have in this one space the full high-level operation of the program. (There are actually some slightly cleaner ways of handling this, but I think that this code best gets the point across to beginners.)

Now all that remains is to write the read_selection(), convert_C_to_F() and convert_F_to_C() functions, which I will leave as an exercise for you. However, one thing I would strongly suggest you do in your initial version of this is to keep all looping control out of these functions. That is, regardless of whether the convert_C_to_F() gets valid input or not, always have it just take input, print something, and return. If it gets an invalid input temperature you can simply print an error message and return, letting the top level take care of letting the user try again. (She'll have to enter c or f again, but that's hardly a huge inconvenience.)

Once you've got that working, you can consider extending those functions to request another temperature if the given one is invalid, but before you do that I'd encourage you to look at the two functions convert_C_to_F() and convert_F_to_C() and see if there's common code in the two that you can factor out into their own functions. As a hint, one of the first things you'll probably see is that getting the number, converting it with float() and handling the potential exception there is common to both and can be extracted to a separate function.

This was long, but I hope that this gives some sense of the need to look at overall program structure. There are lots of small errors, you've made as well, but these both have less effect on overall program readability and are much more easily fixed than problems with the overall program structure.


EDIT: Regarding the comments below about having one function get the number to convert and then call another function to do the conversion, here's an abbreviated code sample to explain what I mean by passing one function to another function for the latter to call. I've trimmed this down quite a bit to express just the core idea and give an example of its use; you can work from this idea to add it to the more sophisticated program (which includes user input to select the type of conversion, etc.) in the original question.

def c2f(t):
    ' Convert Celsius temperature `t` to Fahrenheit '
    return t*1.8 + 32

def f2c(t):
    ' Convert Fahrenheit temperature `t` to Celsius '
    return (t-32)/1.8

def convert(f):
    ' Read a value and convert it using function `f`. '
    n = float(input('  Enter value: '))
    print('  Converts to: ', f(n))

def main():
    print('Converting C to F:')
    #   Notice here how we give just the function name, `c2f`,
    #   without calling it by adding parens () to the end.
    #   Convert will bind this function to its parameter `f`
    #   and then can later call it with `f(n)`.
    convert(c2f)
    print('Converting F to C:')
    convert(f2c)

The ability to pass functions to other functions, return them from functions and assign them to variables is referred to as having "first class functions", and is part of a powerful suite of techniques known as functional programming. Languages vary in their support for this; some languages (such as Haskell) are built around these techiques, other provide almost no support at all for them.

Python falls in the middle; it wouldn't be considered a full-fledged functional programming language, but it does provide a fair amount of support for functional programming and some techniques, such as this one, are very commonly used. See, for example, the built-in map() function that can replace some types of for loops.

Python decorators, which you probably not heard of yet, are a classic example of something that looks like a special language feature but is in fact just pure functional programming (passing around functions) with only a tiny bit of syntax added. The realpython.com decorator tutorial goes into a lot more detail about first-class functions and the rest of this, and is well worth reading when you feel ready.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You have correctly identified the most important problem with the code: functions are being incorrectly treated as goto labels. This is therefore spaghetti code, and this bad practice must be fixed. \$\endgroup\$ May 3, 2019 at 16:08
  • \$\begingroup\$ @Curt J. Sampson Thank you for the response; long it good! I believe I understand all of your points. However for clarity, and building on your statement of trying to group common process in one vice multiple functions. I feel like in the main() function I should have it run 'selection' and the if selection return 'c' or 'f' then run another function value() that takes selection as an argument. Then prompts the user for their value, converts it to a float, and then either goes to convert_f2c(value) or convert_c2f(value) if the value is 'f' or 'c'. Good...bad? \$\endgroup\$
    – J Karle
    May 5, 2019 at 17:16
  • 1
    \$\begingroup\$ Yes, what you've described is one reasonable way of factoring out the common "get value, call conversion function" code from the details of actually doing the conversion. You can actually just directly pass in the function that does the conversion to be called by the "read and call convert" function; this passing of functions to be called by something else is a style known as "functional programming." I'll edit above to add an example. \$\endgroup\$
    – cjs
    May 6, 2019 at 1:25
4
\$\begingroup\$

I'm pretty happy that you've gone the extra mile to take a simple exercise and make it your own. That truly shows a lot of programming potential and initiative. While python code isn't restricted to any type of casing, let's use the more conventional snake_case for this review.

There are a few times where you print a menu with multiple options. One idea to improve your code would be to write a reusable method that handles the redundancies of a numbered menu:

def numbered_menu(options):
    print("Do you want to:\n")
    for i, option in enumerate(options):
        print(f"\t{i + 1}. {option}")
    input("> ")

# usage example
numbered_menu(["Celsius to Fahrenheit", "Fahrenheit to Celsius", "Exit"])

This method makes it easy to make a numbered_menu any time you need to print a bunch of options a user can choose from. There are a few fancy python things that may seem new to a reader, so let's break them down. The method enumerate is a convenient method that allows us to iterate with both the index and item of a list; i is the index, and option is the item in the list. Here we want to print the number of the option and the option itself, so enumerate is exactly what we want.

Another tricky doodad is the f"" string. The f is short for formatted string. Assuming you are using at least python 3.6, a formatted string allows you to write python code directly in a string. That braced {i+1} executes like python code embedded when the string is formatted. This line is equivalent to:

print(str(i + 1) + ". " + option)

However, many would argue that the f-string syntax is more pythonic.

While we're at it...

def menu(options):
    user_choice = None
    while user_choice not in options:
        print("Do you want to:\n")
        for key, (option, action) in options.items():
            print(f"{key}. {option}")
            user_choice = input("> ")
    return options[user_choice][1]

# usage example
menu({
    "1": ("Celsius to Fahrenheit", CtoF_Calc),
    "2": ("Fahrenheit to Celsius", FtoC_Calc),
    "q": ("Exit", lambda *args: None) # because q is the first letter in exit
})()

We have achieved python zen. With a fully declarative menu, we can make user menus with minimalist code. This is a dictionary of of tuples. The .items() method is similar to enumerate, but this one gives us the dict keys on the left and the dict values on the right. Since our dict values are tuples the (option, action) syntax destructures the tuples.

\$\endgroup\$
3
  • \$\begingroup\$ you forgot parenthesis in return options[user_choice][1]() \$\endgroup\$ May 3, 2019 at 8:12
  • \$\begingroup\$ The line user_choice = input("> ") is indented one level too far, asking the user after every option instead of once after all options have been printed. \$\endgroup\$
    – Graipher
    May 3, 2019 at 12:28
  • \$\begingroup\$ Also I don't see how # because q is the first letter in exit can ever be true. \$\endgroup\$
    – Graipher
    May 3, 2019 at 12:30

Your Answer

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

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