5
\$\begingroup\$

I am fairly new to Python. I wrote a program to take the user inputs which are service, the top-up time, the top-up amount, and exit or stay on the program.

Please review my code. And, can my code be organized? I think it looks a bit messy, maybe it can be shorter?

  • First, I wrote everything under function, so I can restart the program later on.
  • Second, I want to take user input about the service that they want to top up by limiting the choice of service to only 3 which are Green, Blue, or Red. If none of it, display "Try Again!, Wrong Service" otherwise, "Welcome to Top-up Service".
  • Third, after passed service versification, the user can top up but limit to only 50,100,300,500,1000. Also, the user can only choose "How many times they want to top-up".
  • Forth, after top-up, the list of top-up and sum of top-up is presented.
  • Last, the user can choose to stay or exit the program. If stay, the program is restarted, if exit, the program is terminated.

Here is the code that I wrote

  def main():
# Letting user choose a service
    service = None
    while service not in {"Green", "Blue", "Red"}:
        service = input("Please enter Green, Blue or Red: ")
    # Now service is either Green, Blue or Red 
        if service in {"Green", "Blue", "Red"}:
            print("Welcome to Top-up Service")
        else:
            print("Try Again!, Wrong Service")

# Doing Top up(s)
    top_list = []
    n=int(input("How many times do you want to top up? "))
    for i in range(1,n+1):
        top_up = None 
        while top_up not in {50,100,300,500,1000}:
            top_up = int(input("Please enter 50,100,300,500 or 1000: "))
    # Now top_up is either 50,100,300,500 or 1000"
            if top_up in {50,100,300,500,1000}: 
                top_list.append(top_up)
            else:
                print("Try Again!, Wrong Top-up")

# Show all top up(s) information & Sum all top up(s)
    print("Top up(s) information : ", top_list) 
    print("Total top up(s): ", sum(top_list))
    
# Letting user choose to stay or exit
    restart = input("Do you want to start again?,Y or else ").upper()
    if restart == "Y":
        main()
    else:
        print("Thank you for using our service")
        exit()
main()

This is the example of output

Please enter Green, Blue or Red: o
Try Again!, Wrong Service
Please enter Green, Blue or Red: Blue
Welcome to Top-up Service
How many times do you want to top up? 2
Please enter 50,100,300,500 or 1000: 20
Try Again!, Wrong Top-up
Please enter 50,100,300,500 or 1000: 50
Please enter 50,100,300,500 or 1000: 500
Top up(s) information :  [50, 500]
Total top up(s):  550
Do you want to start again?,Y or else n
Thank you for using our service
\$\endgroup\$
4
\$\begingroup\$

You can divide your whole program into parts, this way it becomes easy while debugging and increases modularity in the code.

You can divide into multiple functions like

  • Selecting service.
  • Add top-up
  • Restart logic

Refractored code

AVAILABLE_SERVICES = {"Green", "Blue", "Red"} 
AVAILABLE_TOPUPS = {50, 100, 300, 500, 1000}


def select_service():
    while True:
        service = input("Please enter Green, Blue or Red: ").title()
        if service in AVAILABLE_SERVICES:
            print("Welcome to Top-up Service")
            break
    return service


def top_up(service):
    times_topup = int(input("How many times do you want to top up? "))
    topups_selected = []
    for _ in range(times_topup):
        while True:
            topup = int(input("Please enter 50,100,300,500 or 1000: "))
            if topup in AVAILABLE_TOPUPS:
                topups_selected.append(topup)
                break
            else:
                print("Try Again!, Wrong Top-up")

    print("Top up(s) information : ", topups_selected)
    print("Total top up(s): ", sum(topups_selected))


def process():
    restart = "Y"
    while restart == "Y":
        service = select_service()
        top_up(service)
        restart = input("Do you want to start again?,Y or else: ").upper()
    print("Thank you for using our service")


if __name__ == "__main__":
    process()

Output:

Please enter Green, Blue or Red: Blue
Welcome to Top-up Service
How many times do you want to top up? 2
Please enter 50,100,300,500 or 1000: 7
Try Again!, Wrong Top-up
Please enter 50,100,300,500 or 1000: 50
Please enter 50,100,300,500 or 1000: 10
Try Again!, Wrong Top-up
Please enter 50,100,300,500 or 1000: 100
Top up(s) information :  [50, 100]
Total top up(s):  150
Do you want to start again?,Y or else: Y
Please enter Green, Blue or Red: Red
...
...
...
Do you want to start again?,Y or else: n
Thank you for using our service

Code Review

  • PEP-8 violations:

    • E231 - missing whitespace after ,
      • range(1,n+1)range(1, n+1)
      • {50,100,300,500,1000}{50, 100, 300, 500, 1000}
  • Declare constants for services and top-ups instead of writing {"Blue", ...} and {50, ...} everywhere.
    • {"Blue", ...}AVAILABLE_SERVICES = {"Green", "Blue", "Red"} Now you can use AVAILABLE_SERVICES instead of writing {"Blue", ...}
    • {50, ...}AVAILABLE_TOPUPS = {50, 100, 300, 500, 1000}
  • Use str.title when taking input for service, so "BLUE" would become "Blue".
  • Use _ here for i in range(1, n+1) when you are not using i in the loop.
  • You can make AVAILABLE_SERVICES as frozenset to make immutable.
    • AVAILABLE_SERVICES = frozenset({"Green", "Blue", "Red"})
    • AVAILABLE_TOPUPS = frozenset({50, 100, 300, 500, 1000})

You can use pep8online* to check if your code is in accordance with PEP-8 standard.

You can use black* (code formatting tool)

black --line-length 79 your_file.py

* I'm not affiliated to pep8online and black.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ service isn't global \$\endgroup\$ – user253751 Dec 9 '20 at 15:50
  • \$\begingroup\$ @user253751 We can add return statement to select_service and pass it as arg to top_up. \$\endgroup\$ – Ch3steR Dec 9 '20 at 16:08
3
\$\begingroup\$

Make use of docstring

A docstring explains what the function does, the input it expects and what output it gives. It's pythonic to include docstrings in functions and classes just below the function or class definition. For example

def f():
     """This is a function"""

Irrelevant checks

In your while loop, you check if service is in your set and also do a similar check in the while loop's body, the latter is irrelevant, you should take advantage of the first check and refactor the code like this

service = input("Please enter Green, Blue or Red: ")
while service not in {"Green", "Blue", "Red"}:
        print("Try Again!, Wrong Service")
        service = input("Please enter Green, Blue or Red: ")
# service is valid because the while loop breaks
# continue the code here....

A similar approach can be done for top_up

Side note

This program does nor necessarily need a recursive approach, recursion tends to slower and takes more memory due to keeping temporaries in the stack frame, consider using a while loop instead.

\$\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.