6
\$\begingroup\$

Thank you to everyone for the feedback provided to the initial version of this program posted here.

Please find below the newest version, revised based on the comments provided, for review and comment on how I can further improve my coding.

I have removed the portion of the program that checked if the provided value was a legitimate temperature, as this was useless code only showing that I had recently learned what the opposite of absolute zero was....who cares, and who cares what value the user wants to convert!

I look forward to your feedback.

#!/usr/bin/python
"""

Program: Temperature Conversion (C to F, or F to C)
Date:    05 May 2019
Version: 1.2
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.

version history:
    1.0 Initial draft
    1.1 Correction of runtime; posted to StackExchange for feedback
    1.2 Re-coded based on feedback; trying to improve flow control
"""


def read_selection():
    selection = input('''Welcome to the temperature conversion program!

Please make a selection:

    c to convert from Celcius to Fahrenheit;
    f to convert from Fahrenheit to Celsius; or
    q to quit the program.

Enter your selection: ''').lower()
    return selection


def value_input(selection):
    value = input('''\nPlease enter the temperature you
want to convert: ''')
    try:
        value = float(value)
    except ValueError:
        print('That is not a number!\n')
    else:
        if selection == 'c':
            convert_c2f(value)
        else:
            convert_f2c(value)
            # return value


def convert_c2f(value):
    print(f'The answer is: {(value * (9/5)) + 32}°F\n')
    return


def convert_f2c(value):
    print(f'The answer is: {(value-32) * (5/9)}°C\n')
    return


def main():
    while True:
        selection = read_selection()
        if selection == 'q':
            return
        elif selection == 'c' or selection == 'f':
            value_input(selection)
            '''convert_c2f()
        elif selection == 'f':
            convert_f2C()'''
        else:
            print('Invalid selction. Try again.\n')


if __name__ == '__main__':
    main()
\$\endgroup\$
8
\$\begingroup\$

You're not following PEP8 and you still have a spaghetti mind-set. Each function should have a single responsibility.

value_input however is in charge of:

  1. Asking and validating user input.
  2. Handling how to convert the input.
  3. Convert and display the input.


This should instead only perform the first task I've said above. After this you should have the calling code perform 2 and 3.

convert_c2f also is responsible for two things converting and displaying the input.

It can be seen in main that you were originally doing something better than you have now, so it's unclear why you changed this.

def read_selection():
    return input('''Welcome to the temperature conversion program!

Please make a selection:

    c to convert from Celcius to Fahrenheit;
    f to convert from Fahrenheit to Celsius; or
    q to quit the program.

Enter your selection: ''')


def float_input():
    value = input('''\nPlease enter the temperature you
want to convert: ''')
    try:
        return float(value)
    except ValueError:
        print('That is not a number!\n')


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


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


def main():
    while True:
        selection = read_selection().lower()
        if selection == 'q':
            return
        elif selection == 'c' or selection == 'f':
            value = float_input(selection)
            if selection == 'c':
                converted = convert_c2f(value)
                print(f'The answer is: {converted}°F\n')
            else:
                converted = convert_f2c(value)
                print(f'The answer is: {converted}°C\n')
        else:
            print('Invalid selction. Try again.\n')
\$\endgroup\$
  • \$\begingroup\$ @Peilonrazy Thank you for the feedback; excellent as always. I am clearly not fully understanding what recursion and iteration is. However, I am researching that now, and once I have done so I will come back with a few questions, or a new version of the program. One question now however - WRT converting the input to a float in the input definition, on top of asking for the input, why is this "bad?" It has to be converted somewhere, so whether is done in that def when it is entered, or back in the main() def, what are the advantages or disadvantages? \$\endgroup\$ – J Karle May 6 at 16:27
  • 1
    \$\begingroup\$ @JKarle 1. If you are only allowed to look at the main, it's hard to tell what the code is doing in your method. However when it's written the way I suggested it's immediately apparent what it does. And since input_float has a good name you can reasonably guess what the code is doing just from the main method. 2. If you need to later get a floating point number from the user, but you don't want either of the convert functions to run you either; a. have to make a new function b. hack at the existing function. And so it's better to keep SRP. \$\endgroup\$ – Peilonrayz May 6 at 17:43
7
\$\begingroup\$

@Peilonrayz and @Carcigenicate have great feedback you should definitely follow.

Since you are using Python 3 features like f-string, #!/usr/bin/env python3 should be used as shebang line.

\$\endgroup\$
  • \$\begingroup\$ Thank you, I was thinking about that, but forgot to follow-up and look it up. I will take that for action! \$\endgroup\$ – J Karle May 6 at 16:32
5
\$\begingroup\$

Your conversion functions have two notable things:

  • You're printing the result directly. Don't do this. For toy programs like this doing so doesn't create problems. In the real world though, you don't just print data to the screen, you use data. With how you have it now, the caller can't actually use the converted value for anything. What if you wanted to send the raw data over the internet or save it to a file?

  • You're putting an empty return at the end. This is redundant though and unnecessary. None is automatically returned at the end anyways if no return is met, which is equivalent to what you're doing.

I would have the functions return the converted value, and print it at the call site:

def value_input(selection):
    value = input('''\nPlease enter the temperature you
want to convert: ''')
    try:
        value = float(value)

    except ValueError:
        print('That is not a number!\n')

    else:
        # Save if we're converting to Celsius or not
        is_celsius = selection == 'c' 

        new_value = convert_c2f(value) if is_celsius else convert_f2c(value)
        unit_str = "C" if is_celsius else "F"

        print(f'The answer is: {new_value}°{unit_str}')


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

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

I decided to reduce the printing down to a single call to print and just decide what data will be printed ahead of time. This is personal style though. I get an odd thrill out of reducing duplication. Do whatever you feel is more readable.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback. Copy on the empty return (that makes sense), and that I should not print the result directly (that also makes sense after thinking on it for the night.) Rather, and fixing both issues raised, I should calculate the value and return that from the def, to then be display as required. \$\endgroup\$ – J Karle May 6 at 16:33
  • 1
    \$\begingroup\$ @JKarle Yes, let the caller decide how they want to use the data. If they want to print it out, they can do that themselves. Don't force data to be used in a certain way we possible unless you have good reason. You'll limit how easily that function can be used by other code in the future. \$\endgroup\$ – Carcigenicate May 6 at 16:38

Your Answer

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

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