7
\$\begingroup\$

I am a beginner in python and I was wondering if the code for this program is nice and clean. The program works I was wondering how could i improve it. So this program asks for your 'username' and checks to see if your 'username' is in the 'banned users' list. If it is then it will 'log out' and won't let you comment. If your 'username' isn't in the 'banned uses' list it will let you comment and ask if you would like to let someone else to comment. If you enter 'yes' the 'while loop' will run again if there isn't then the program will print the username and comments. This is basically what the program does. How can I improve it?

# We define a function here.
def display_comments():
    """Displays User Input (comments)"""
    if user_comments:  # If the dictionary 'user_comments' has at least one key value pair then execute this block of code.
        print("\nTop Comments:")
        for username, comment in user_comments.items(): # Define a for loop for each key value pair in the dictionary 'user_comments'.
            print(f"\n\t{username}: {comment}.")  # Print each key value pair.

username_prompt = "\nUsername: "
comment_prompt = "Comment: "
continue_prompt = "\nWould you like to let another user comment (yes/no)? "

banned_users = ['pete', 'jack', 'ali', 'henry', 'jason', 'emily'] # List of banned users.
user_comments = {} # Empty dictionary that we will store user input later on in the program.

account_active = True  # Set up a flag

while account_active: # Run as long as the 'account_active' flag remains 'True'.
    username = input(username_prompt) # Ask for a username and store it in the variable 'username'.

    if username.lower() in banned_users: # Cross reference the username with the banned_users list. If the username is in the banned_users list then execute the following code.
        print(f"\nI'm sorry {username} but you are banned from using this site.") # Tell the user that they are banned.
        print("Deactivating...") # Simulate logging out.

        account_active = False # Set the flag to 'False'

    else: # Run this code only if the user is not in the 'banned_users' list.
        comment = input(comment_prompt) # Ask the user for their comment and store it in the variable 'comment'.

        user_comments[username] = comment # Add the username and comment (key-value pair) to the 'user_comments' dictionary.

        repeat = input(continue_prompt) # Ask the user if they want to repeat and store it in the variable 'repeat'.

        if repeat == 'no': # If the variable 'repeat' has the value 'no' then execute the following code, otherwise run the while loop again.
            account_active = False # Set the flag to 'False'

display_comments() # Call the 'display_comments()' function to print the key-value pairs in the dictionary.
\$\endgroup\$
  • 6
    \$\begingroup\$ Usually you would put a summary of the problem or what the code does in the title. The desire to improve the code is implied by posting to this site. \$\endgroup\$ – spyr03 Aug 14 at 14:01
  • \$\begingroup\$ spyr03 Thanks bro, I changed it. \$\endgroup\$ – snow_razer Aug 14 at 14:15
7
\$\begingroup\$

The code is pretty clear in what it does, good work.


# We define a function here.
def ...
...
... = {} # Empty dictionary
...
... = False # Set the flag to 'False'

Many of these comments are not all that useful. I can see by looking at the code that the flag is set to false, or that a function has been defined. Usually you would comment with the reason why a piece of code exists. If you removed a comment from beside some code, could you still work out what the code does? If so the comment is probably not useful.


def display_comments():
    """Displays User Input (comments)"""
    if user_comments:  # If the dictionary 'user_comments' has at least one key value pair then execute this block of code.
        print("\nTop Comments:")
        for username, comment in user_comments.items(): # Define a for loop for each key value pair in the dictionary 'user_comments'.
            print(f"\n\t{username}: {comment}.")  # Print each key value pair.

This looks pretty good. It does what it says it does. Remove the inline comments and it is golden.


banned_users = ['pete', 'jack', ...
...
if username.lower() in banned_users:

Extracting banned users to a list is good. An issue might arise here if a with an uppercase letter is is added to the banned list. He won't be prevented from commenting! In general there are two ways to avoid this problem

  1. Do a case-insenstive compare. In a Java this would be done with username.equalsIgnoreCase(banned_user)
  2. Normalize both strings and then compare. This is the recommended method in Python.

So I would suggest making sure every banned user has been lowercased

banned_users = ['pete', 'jack', ...
banned_users = [banned_user.lower() for banned_user in banned_users]

If you believe you'll ever need to deal with international usernames casefold may be of interest. It will normalize strings more aggressively than lower will.

One result of lowercasing the names is that it cuts down on the number of available usernames. If Ben Dor and B. Endor both try to sign up as BenDor and BEndor respectively, only one can get the name. Is that OK? Will this ever be a problem?


repeat = input(continue_prompt)
if repeat == 'no':

This is one place I would lowercase as an answer of "NO" or "No" clearly indicate the user is done.


user_comments = {}
...    
user_comments[username] = comment

As the comments are stored in a dictionary, every username will only have one comment, the latest one they've made. For instance if the chat was meant to be

A: where is the coal?
B: walk west for 2 mins then
B: north for 1 min

The comments appear as

A: where is the coal?
B: north for 1 min

Is that intended? If not you can change to use a list containing the username and comment instead

comments = []
...
comments.append((username, comment))

and display_comments looks nearly identical

for username, comment in comments:
    ...
\$\endgroup\$
  • \$\begingroup\$ You're explanation is very clear and understandable. I just didn't get one point that you made, and it's probably because I am a beginner. You said that I can change to using a list containing usernames and comments instead, you wrote: 'comments.append((username, comment))'.Could you please explain how this works since I thought that the elements in the list had no correlation with each other unlike key-value pairs in dictionaries. \$\endgroup\$ – snow_razer Aug 14 at 15:25
  • 1
    \$\begingroup\$ You are right that elements in a list are separate things. However if you look at the elements each one is itself a tuple of the username and comment. So in the same way you can have a list of points [(1, 2), (-3, 5), (x1, y1)] you have a list of username/comment pairs [('pete', 'hello'), ('jack', 'hi there'), ('pete', 'are you nearby?')] \$\endgroup\$ – spyr03 Aug 14 at 15:29
  • 1
    \$\begingroup\$ notice the extra pair of brackets in comments.append(). Append takes one object to add to the list, so I want to add the tuple (username, comment) to the list \$\endgroup\$ – spyr03 Aug 14 at 15:30
  • \$\begingroup\$ I fully understood, thank you very much. One last question though, how would the for loop work to extract the tuples from the list and print it. \$\endgroup\$ – snow_razer Aug 14 at 15:38
  • 1
    \$\begingroup\$ The keyword to search is tuple unpacking. If first_pair = comments[0] you can unpack the pair with username, comment = first_pair. Since for x in y assigns to x each object in y, you can unpack in the loop header. Thus for pair in comments becomes for username, comment in comments. \$\endgroup\$ – spyr03 Aug 14 at 16:22

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.