7
\$\begingroup\$

Below is an example of a simple converter. In the first example temperature I have used a function within a function. In the second example milli I have used a more direct approach. Which would you consider is the better method to handling calculations in functions and why? Also feedback on my approach and examples of other methods is appreciated.

def temperature():
  while True:
    try:
      user_in = float(input('Number to convert:> '))
      t = user_in
      def cel_to_far(x):
        return 9 * x / 5 + 32
      result = 'is: {} degrees Fahrenheit'.format(cel_to_far(t))
      print('Input: {} degrees Celcius'.format(t), result)
      break
    except ValueError:
      print('Enter numbers only!')


def milli():
  while True:
    try:
      user_in = float(input('Number to convert:> '))
      n = 1 * user_in / 25.4
      print('Input: {} millimeters is: {} inches'.format(user_in, n))
      break
    except ValueError:
      print('Enter numbers only!')
\$\endgroup\$
10
\$\begingroup\$

I think it depends on how self-evident the code is. If it's clear from context what the intent of a calculation or function call is, then I agree with @Sam that the code will be more concise without giving it a name.

On the other end though, depending on the context, 9 * x / 5 + 32 might not be obvious in what it's doing. You may find that giving that equation a name helps readability in general, which may trump conciseness.

In this exact case though, I don't think there's a ton of benefit to giving it a name unless you strongly suspected that you'd need it elsewhere in the future, or you personally like the way having a name reads. I think that that formula is well-known enough that most people would be able to tell what it's doing; especially when it's inside of a function called temperature.


In this case though, there's no good reason that the function should be inside of temperature. Defining one function inside of another suggests that the inner function has no meaning outside of enclosing function, which may be the case if you're forming a closure over a local variable or something similar. In this case though, cel_to_far is just simply taking a parameter and returning an output. It isn't using anything local to temperature. If you wanted to have a standalone function, I would definitely move cel_to_far outside of temperature. If you want it to be "private" to the module, prefix the name with an _:

def _cel_to_far(x):
    return 9 * x / 5 + 32

def temperature():
    . . .

You're only using two spaces of indentation. Please use four-spaces, as per PEP8.


t = user_in isn't useful. You're taking a line to rename an already short variable name to something obscure. If you need to reduce line lengths, there are other more readable ways of achieving that. Here though, the longest line is only 76 characters long after fixing the indentation and getting rid of t. That's not very long, and user_in is a much more descriptive name.


In both your main functions, you're getting float input from the user manually with a while True and try. In an effort to reduce duplication and centralize how you're handling input, I'd make a helper function:

def _ask_for_float():
    while True:
        try:
            return float(input('Number to convert:> '))

        except ValueError:
            print('Enter numbers only!')

Instead of repeating the while True: try . . . part, make that a function, then call the function. Notice how much neater this makes each of the functions:

def _ask_for_float():
    while True:
        try:
            return float(input('Number to convert:> '))

        except ValueError:
            print('Enter numbers only!')


def _cel_to_far(x):
    return 9 * x / 5 + 32


def temperature():
    user_in = _ask_for_float()
    result = 'is: {} degrees Fahrenheit'.format(_cel_to_far(user_in))

    print('Input: {} degrees Celcius'.format(user_in), result)


def milli():
    user_in = _ask_for_float()
    n = 1 * user_in / 25.4

    print('Input: {} millimeters is: {} inches'.format(user_in, n))

That immediately fixes any line-length issues by removing two levels of indentation, and makes each function trivial.


Finally, lines like:

print('Input: {} degrees Celcius'.format(user_in), result)

Can be written inline using f-strings. I'd collapse result back into the literal for readability as well:

def temperature():
    user_in = _ask_for_float()

    print(f'Input: {user_in} degrees Celcius is: {_cel_to_far(user_in)} degrees Fahrenheit')
\$\endgroup\$
  • \$\begingroup\$ To make in more user friendly, _ask_for_float() should take a str argument to be passed to input(). It can default to 'Number to convert:> '. That way the prompt can be something more useful, like "Enter degrees Celcius:> " \$\endgroup\$ – RootTwo Dec 17 '19 at 7:01
  • \$\begingroup\$ @RootTwo When I've suggested this function previously, I had just that. I intentionally wanted to keep it simple here though. \$\endgroup\$ – Carcigenicate Dec 17 '19 at 13:34
6
\$\begingroup\$

My general rule is that if I'm going to need to refer to something more than once, I give it a name, and if not, then I skip the syntactic sugar and just use the expression in place. This usually makes the code more concise and reduces the burden on the reader of having to keep track of a bunch of different variable names.

Applying that principle to your first example, I'd get rid of not only cel_to_far but also t and result, which would give me:

def temperature() -> None:
    while True:
        try:
            user_in = float(input('Number to convert:> '))
            print(
                'Input: {} degrees Celsius'.format(user_in),
                'is: {} degrees Fahrenheit'.format(9 * user_in / 5 + 32)
            )
            break
        except ValueError:
            print('Enter numbers only!')

IMO this is a lot more clear because you can see at a glance what's happening to the input without having to trace it through multiple assignments and function calls (maintaining multiple pieces of mental state to be able to execute the code in your head and figure out what it's doing).

The new version of the print call also shows a preview within the code of exactly what the output will look like, rather than assembling it in reverse order (again, making the reader mentally execute the code in order to visualize the final result).

If you think you might have any other functions that would need to do the same conversion, then of course it makes sense to define a function for it, but you'd do it in the outer scope so that it's actually reusable.

\$\endgroup\$

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.