3
\$\begingroup\$

I was wondering if my code is good or maybe if it could be written better or simpler.

The task was to implement a program that prompts the user for a fraction, formatted as X/Y, wherein each of X and Y is an integer, and then outputs, as a percentage rounded to the nearest integer, how much fuel is in the tank. If, though, 1% or less remains, output E instead to indicate that the tank is essentially empty. And if 99% or more remains, output F instead to indicate that the tank is essentially full. If, though, X or Y is not an integer, X is greater than Y, or Y is 0, instead prompt the user again. (It is not necessary for Y to be 4.) Be sure to catch any exceptions like ValueError or ZeroDivisionError.

Here's my code:

def main():
    z = get_fraction("Fraction: ")
    print(z)

def get_fraction(prompt):
    while True:
        try:
            x, y = input(prompt).split("/")
            x = int(x)
            y = int(y)
            if x <= y:
                z = x/y
            else:
                continue
        except (ValueError, ZeroDivisionError):
            pass
        else:
            z *= 100
            if z >= 99:
                z = "F"
            elif z <= 1:
                z = "E"
            else:
                z = f"{z:.0f}%"
            return z


main()

It works but I wonder if it could be done better :)

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

This seems to mostly compute the right thing. Good job!

The code is much too complex. In particular, the flow of control wanders around more than necessary, due to interaction of while True:, exceptions, and the try / else clause. Also we have one big blob instead of a few small self-contained helper functions.

We need to accomplish a pair of high-level goals, in order.

  1. Keep prompting till we get a valid X/Y input fraction.
  2. Format that fraction.

I would love to see a z = prompt_for_fraction() call to a helper function. That would make it clear that step (1) has finished and we're moving on to step (2). As written, the code keeps jumping in and out of the try block (with continue and return), and step (2) is bound up in the try's else clause.

Here is one possible implementation, which happens to use map:

def prompt_for_fraction(prompt: str = "Fraction: ") -> float:
    """Keep prompting until we get a valid fuel gauge reading."""
    while True:
        line = input(prompt)
        try:
            x, y = map(int, line.split("/"))
            if x <= y:
                return x / y
        except (ValueError, ZeroDivisionError):
            pass

type stability

                z = x / y
            ...
            z *= 100
            ...
                z = "F"

Python is a dynamic language, so all three of these assignments work. But that doesn't mean you have to go crazy with it. The machine doesn't care what value an object (z) has, but it matters to humans, who will want to understand what it means.

Here's a simple rule: Once having assigned a certain type to a variable, let it stick with that type. If you want to switch to a new type like str, invent a new variable name for that new meaning.

That deals with the third assignment. The disconnect on the first two assignments is a more subtle issue of "meaning stability". First we have a fraction on the unit interval 0 .. 1, then the second assignment gives us a percentage 0 .. 100. Better to keep a single meaning for that identifier, like z = 100 * x / y. Then you'll spend less time worrying about bugs.


requirements

There are code defects, and requirements defects. This one is definitely not your fault. And I'm sure the problem author was just trying to keep things simple.

The "tank fullness" fraction can only take on values in the unit interval 0 .. 1. User input is constrained to be no more than one. However, we do accept negative values, which seems unfortunate.


This code achieves its design goals.

I would be willing to accept maintenance tasks on it.

\$\endgroup\$

Your Answer

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

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