3
\$\begingroup\$

I'm new around here and want to involve myself with fellow developers and hopefully improve my skills here while attempting to help in any way I can with the knowledge I have so far. I'm a new Python programmer, been on and off it for years but have stuck to it recently officially.

I plan on making a career off of developing with it, whether it's data science, cyber security etc.

I plan on implementing some OOP in there instead of just straight functions:

import os

#create shopping list with automatic calculation


def display_help():
    print('''
    =======> Help <=======
    'HELP' to display this menu
    'START' to start creating your shopping list
    'SHOW' to display your current items
    'QUIT' to exit the program
    ''')
#create a list with the items needed
#

def clear_screen():
    os.system("cls" if os.name == "nt" else "clear")



#Create function to change values of current items or update price of items in the cart



def shopping_list():
    clear_screen()
    print('It\'s time to go shopping!')
    #Convert all floating point numbers to max digits of 2 after the decimal    
    clear_screen()
    item_dict = {}
    print('Enter HELP at any moment if you need help or EXIT to return to the main menu')
    cart = 0
    def show_items():
        for key, value in item_dict.items():
            print(key + '-' * 5 + str(value))
        print('Current cart total: ',sum(item_dict.values()))
    while True:
        shop = input('Please enter an item: ')
        if shop.upper() == 'HELP':
            display_help()
            continue
        elif shop.upper() == 'EXIT':
           #Show total with taxes (or without taxes) with all the items in cronological order
           #start without taxes first
           print("Goodbye")
           break
        elif shop.upper() == 'SHOW':
            show_items()
            continue
        print(f'You have added {shop} to your list\n')
        try:
            cost = float(input(f'Price of {shop}: '))
            item_dict[shop] = cost
            cart = sum(item_dict.values())
            print(f'You entered {cost} as the price of {shop}\n')
            print(f'You now have {len(item_dict)} items in your list.\n')
            print(f'The total currently on your cart is {cart}')
            add_more_items = input('Would you like to add more items?(Y/N) ')
            if add_more_items == 'Y'.lower():
                continue
            else:
                print(f'Here\'s your cart and your total: \n')
                raise KeyboardInterrupt('Goodbye')
                continue
        except ValueError:
            print('Please enter a number')



def begin_attempt():
    print('Here\'s a list of possible commands: HELP, SHOP, EXIT')
    print('Welcome! What would you like to do?')
    while True:
        choose = input('> ')
        if choose.upper() == 'SHOP':
            print('Shopping has begun\n')
            shopping_list()
        elif choose.upper() == 'HELP':
            display_help()
            continue
        elif choose.upper() == 'EXIT':
            print("Goodbye")
            break

begin_attempt()

I still would like to add a lot more things, such as entering a specific state (or city in this case since I don't want to look up the taxes of every state) to calculate taxes, if there are discounts anywhere (I don't think I'll be adding that anytime soon or at all, but just an idea).

\$\endgroup\$
  • \$\begingroup\$ I was just having a quick look through this and was wondering whether at the current stage you want suggestions on how to implement some OOP in there or just things like formatting \$\endgroup\$ – 13ros27 Jun 9 '18 at 11:54
  • \$\begingroup\$ Hi there. Yes I am currently trying to implement some OOP in there. I started working on it while also improving my old code. Everything is looking better than my old version. \$\endgroup\$ – nudood Jun 10 '18 at 17:18
6
\$\begingroup\$

Welcome to Code Review SE!

I'll list some things I found while skimming over your code. This is unordered, and by no means exhaustive.

  • Most programming languages have a certain style guide associated with them. In the case of Python, it's PEP-8. It is in your best interest to comply with the points outlined there, as doing so will significantly improve readability.

    • Keep vertical whitespace to a minimum. Blank lines can aid in grouping related pieces of code together1, but in my opinion, you've gone a bit overboard. Having just two blank lines in between top-level function definitions is recommended.

    • Your function and variable names follow the PEP-8 naming conventions2. Good job, lots of people get this wrong at first.

    • Whenever you find yourself using an escaped quote character (\' or \"), you can simply surround the string with the other type of quotes.3 This makes the code more readable and reduces the chances of getting syntax errors. To take an example from the piece of code you wrote:

      print('It\'s time to go shopping!')
      

      ... becomes:

      print("It's time to go shopping!")
      
    • It's good practice to separate arguments to functions by a single space.4 An example from shopping_list():

      print('Current cart total: ',sum(item_dict.values()))
      

      ... becomes:

      print('Current cart total: ', sum(item_dict.values()))
      
    • Block comments should start with a space.5

  • It's nice that you added comments, but most of them are plain wrong. Never add comments that are simply a rewording of the code, and always assure your comments are up to date and correct. Let's go over your comments one by one.

    • Lines 3 through 6:

      #create shopping list with automatic calculation
      
      
      def display_help():
      

      ... not even close. I guess you reordered your functions at some point and forgot to update this comment.

    • Lines 14 through 17:

      #create a list with the items needed
      #
      
      def clear_screen():
      

      ... the same goes for this comment. You don't create a list here. You don't do anything here.

    • Lines 22 through 26:

      #Create function to change values of current items or update price of items in the cart
      
      
      
      def shopping_list():
      

      ... finally, a comment that's in the right place. Unfortunately, this one falls under the 'documenting self-documenting code' category.6 Nitpick: you normally don't create a function, but define it.

    • ...

  • I'm happy you found a way to avoid global variables, but nesting functions is rarely the right solution. show_items() should really be a top-level function, taking one argument.

  • By using f-strings, you're greatly limiting portability. I'd make a safe bet and use str.format() for now.

  • Let's see what happens when I exit the program:

    Welcome! What would you like to do?
    > EXIT
    Goodbye
    

    So far, so good. How about quitting from the 'SHOP' submenu?

    Please enter an item: QUIT
    You have added QUIT to your list
    
    Price of QUIT:
    

    Whoops, that's not what I was hoping to see. The help message clearly says:

    'QUIT' to exit the program
    

    Peculiar. Now that we're stuck with having to add a new item, let's go through with it and then exit.

    Price of QUIT: 50
    You entered 50.0 as the price of QUIT
    
    You now have 1 items in your list.
    
    The total currently on your cart is 50.0
    Would you like to add more items?(Y/N) n
    Here's your cart and your total:
    
    Traceback (most recent call last):
    File "ttt.py", line 86, in <module>
        begin_attempt()
    File "ttt.py", line 78, in begin_attempt
        shopping_list()
    File "ttt.py", line 64, in shopping_list
        raise KeyboardInterrupt('Goodbye')
    KeyboardInterrupt: Goodbye
    

    Oh my, that's ugly.

    On a more serious note, there should only be one way to exit the program, and it should not be by means of an uncaught exception. You could do this a number of ways:

    • Put all of the prompts in a huge while-loop, then break from the loop when the user wants to exit. This gets messy very quickly. Avoid it.

    • Put all of the prompts in a huge while-loop, then call a function to exit. This is a much better alternative.

    As an aside, it's almost never a good idea to raise a KeyboardInterrupt manually. Even a RuntimeError would be a better fit, but as I explained, cleanly exiting the script is ideal. The sys module provides sys.exit(), which is the best way of quitting Python scripts. Don't use exit(). It is a special function and should only be used in the Python REPL.

  • The main menu is broken:

        =======> Help <=======
        'HELP' to display this menu
        'START' to start creating your shopping list
        'SHOW' to display your current items
        'QUIT' to exit the program
    
    > start
    > show
    > SHOW
    > :(
    
  • try / except-constructs should capture the least amount of code possible. A clear violation of this principle, starting on line 52:

    try:
        ... [12 lines skipped]
    except ValueError:
        print('Please enter a number')
    

    ... the try-block only needs to encompass line 53, because that's the only line that could raise a ValueError.

  • On line 60, the input validation is off. The prompt suggests 'Y' and 'N' are valid inputs, but:

    if add_more_items == 'Y'.lower():
    

    ... indicates it is not. If you want to do case-insensitive comparisons, call str.lower(), like you did in other instances. Alternatively, you could call str.casefold(), which also handles special characters like 'ß':

    >>> "Straße".lower()
    'straße'
    >>> "Straße".casefold()
    'strasse'
    
  • shopping_list is a bad function name. Function names are typically verbs. Maybe play_shopping_game?

  • String concatenation is in many ways inferior to string formatting. It's slower, harder to read, and less versatile: each argument needs to be casted to str if it is not already a string. Always prefer string formatting over concatenation.

References

1 PEP-8: Blank Lines

2 PEP-8: Naming Conventions

3 PEP-8: String Quotes

4 PEP-8: Whitespace in Expressions and Statements

5 PEP-8: Comments: Block Comments

6 Don't get me wrong, documenting functions is very important. The 'create a function' part is too verbose nevertheless. If you want to add formal documentation to functions, add docstrings! Docstrings are multiline strings placed below a function or class signature. For example:

def add(a, b):
    """Return `a` + `b`. Both arguments must be numbers."""
    return a + b

You can access the docstring as the object's __doc__ attribute:

>>> add.__doc__
'Return `a` + `b`. Both arguments must be numbers.'

... and in the Python REPL, you can use the built-in help():

add(a, b)
    Return `a` + `b`. Both arguments must be numbers.

There's also an official style guide for docstrings, called PEP-257.

\$\endgroup\$
  • \$\begingroup\$ Wow thanks a lot! I really appreciate this and will continue to work on the app. I try to follow the guidelines as much as I can, but practice is what makes perfect. I wanted a good review and you went in depth which I really appreciate. I left some things out in blank since I forgot to add or remove things. The comments I haven't edited due to what you mentioned, I forgot and was busy editing things. May I send you anything updated or post updates here? Not constantly but if I feel I made huge edits. \$\endgroup\$ – nudood May 25 '18 at 17:36
  • \$\begingroup\$ @nudood Of course, if you made improvements and want to put the new code up for review, simply add a new question. Make sure to link back to the previous iteration every time you add a newer version, to avoid having the question flagged as a duplicate. :) \$\endgroup\$ – Daniel May 25 '18 at 17:54

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.