18
\$\begingroup\$

I have written this code which provides advice based on user's favourite YouTuber. I wonder if there is a better way to do absolutely any of it. Also, if there is any way to optimise it or if there is a problem please also tell me preferably with fixed code as I am still sort of a beginner. The code is below:

import random

def test():
    youtuber = input('Enter your favourite youtuber:  ')
    youtuber = youtuber.lower()
    favouriteYoutuber = ['Dr. Phil', 'Mr. Beast', 'T-Series', 'PewDiePie', '5 Minute Crafts', 'The Ellen Show']

    if youtuber == 'dr. phil':
        print('You are an awesome lad!')
    elif youtuber == 'james charles':
        print('Errmm. Ok...')
    elif youtuber == 'bloamz':
        print('Ok then.')
    elif youtuber == 'ali a':
        print('I mean. Thats old but ok...')
    elif youtuber == 'jacksepticeye':
        print('Thats kinda cool')
    elif youtuber == 'will smith':
        print('Thats different. I rate that')
    elif youtuber == 'jack black':
        print('you have good taste')
    elif youtuber == 'jack white':
        print('I like him as well')
    elif youtuber == 'dr. mike':
        print('so you like learning then')
    elif youtuber == 'morgz':
        print('I mean just leave now')
    else:
        print('I dont know that one. Ill check them out')

    print('my favourite youtuber is ' + random.choice(favouriteYoutuber))

def try_again():
    again = True
    while again:
        test()
        while True:
            try:
                print("")
                print("Would you like to try again?")
                maybe = input("Y/N ")
                maybe = maybe.lower()
            except ValueError:
                print("That is not a valid option")
                print("")
                continue
            if maybe in ('y','n'):
                if maybe == "n":
                    print("")
                    again = False
                elif maybe == "y":
                    print("")
                break

    else:
        print("Thank you for using this app!")
try_again()

\$\endgroup\$
4
  • 1
    \$\begingroup\$ What's the purpose of the code? \$\endgroup\$
    – Mast
    Commented Jan 23, 2020 at 20:55
  • \$\begingroup\$ At least half of these are not YouTubers... \$\endgroup\$ Commented Jan 26, 2020 at 21:26
  • \$\begingroup\$ God, I feel old. \$\endgroup\$ Commented Jan 26, 2020 at 21:26
  • 1
    \$\begingroup\$ I've rolled back your edit. Please do not edit the question with updated code. If you would like further improvements, please ask a new question. \$\endgroup\$
    – Linny
    Commented Jan 27, 2020 at 7:46

3 Answers 3

30
\$\begingroup\$

I've heard of dictionary lookups but I don't really understand what they are so if anyone thinks that they maybe relevant and could show me how to implement it that would be great!

Here you go!

Use Dictionaries!

The entire if/elif/else if yelling, if not screaming, to be put into a dictionary. This is a lot faster than your implementation because it's a simple key lookup, instead of multiple logical checks to test the user input.

Here is your test function (now named favorite_youtuber):

def favorite_youtuber():

    youtube_response = {
        'dr. phil': 'You are an awesome lad!',
        'james charles': 'Errmm. Ok...',
        'bloamz': 'Ok then.',
        'ali a': 'I mean. Thats old but ok...',
        'jacksepticeye': 'Thats kinda cool',
        'will smith': 'Thats diferent. I rate that.',
        'jack black': 'You have good taste.',
        'jack white': 'I like him as well.',
        'dr. mike': 'So you like learning then!',
        'morgz': 'I mean just leave now.'
    }
    # Since some youtubers in the list below are not included in the dictionary, I left the list. #
    my_favorite_youtubers = ['Dr. Phil', 'Mr. Beast', 'T-Series', 'PewDiePie', '5 Minute Crafts', 'The Ellen Show']

    youtuber = input('Enter your favourite youtuber:  ').lower()

    if youtuber in youtube_response:
        print(youtube_response[youtuber])
    else:
        print('I dont know that one. Ill check them out.')

    print(f'My favourite youtuber is {random.choice(my_favorite_youtubers)}!')

A dictionary works by utilizing keys and values, like so:

my_dict = {
    "key": "value of any type",
    ...
}

In this case, the key is the name of the youtuber that the user inputs, and the value is the response. This prevents you from having to have multiple print statements depending on what the user inputs. Now, all you have to do is make sure that the youtuber entered by the user is included in the dictionary's keys, utilizing this line:

if youtuber in youtube_response:

Format Your Strings!

The era of my_string = a + " " + b is over. You can now format your strings to include your variables directly in them! Take a look:

print(f'My favourite youtuber is {random.choice(my_favorite_youtubers)}!')

Essentially, the value of the code within {} is placed in that position in the string.

Another option is to use .format(), which is a method called on a string. Take a look:

print('My favourite youtuber is {}'.format(random.choice(my_favorite_youtubers)))

They both do the same thing. It's up to you which one you want to use.

.lower() utilization

Instead of

maybe = input("Y/N ")
maybe = maybe.lower()

do this

maybe = input("Y/N ").lower()

Since input() returns a string, .lower() applies to that string. This prevents you from having to write that extra line, and it makes your code a little nicer.

\n

Instead of

print("That is not a valid option")
print("")

do this

print("That is not a valid option.\n")

It adds a newline character at the end of the string, doing exactly what you're doing but in a nicer way.

Repetitive User Input

Now let's talk about your try_again function.

There's a lot to break down here. I find it easier to show you my improved version of your code, and walking you through what I did. Have a look:

def run_app():
    while True:
        favorite_youtuber()
        again = input("Play again? (Y/N)").lower()
        while again not in "yn":
            print("Please enter Y/N!")
            again = input("Play again? (Y/N)").lower()
        if again == "n":
            break
    print("Thank you for using this app!")

It's fairly self explanatory. The one thing I want to talk about is the nested while loop.

Instead of checking if something is within a tuple ("y", "n"), you can check if something is within a string "yn". It's easier to understand this way. The while loop keeps asking for input until the user enters a "y" or a "n". This is easier than having nested while True: loops, as those can get very messy very fast.

Since you only want to see if they don't want to keep playing, you only need to check for the existence of an "n". Then, it's a simple break statement to print out the final goodbye.

Main Guard

Last thing I'm commenting on.

You should use a main guard when running this program. Why?

Let's say you want to import this module into another program, because you don't want to rewrite all this code in a different file. When you import the module, that spare try_again is going to run. That is not what you want. Containing this extra code in a main guard will prevent this from happening. It's a simple if statement:

if __name__ == "__main__":
    run_app()

I renamed your try_again to run_app(), since that name is more fitting of what the program is doing.

\$\endgroup\$
5
  • \$\begingroup\$ For if youtuber in youtube_response.keys(), don't you mean the simpler if youtuber in youtube_response? Also, by not following your own advice of applying .lower() after input() in your example, if again is "N" then the if again == "n" fails... \$\endgroup\$ Commented Jan 24, 2020 at 12:56
  • \$\begingroup\$ @MatthieuM. Those are both great points, some things I overlooked while I was writing this review. I'll edit my answer accordingly. \$\endgroup\$
    – Linny
    Commented Jan 24, 2020 at 18:38
  • \$\begingroup\$ In run_app it could be again = "" while again != "n":. \$\endgroup\$
    – S.S. Anne
    Commented Jan 24, 2020 at 19:21
  • \$\begingroup\$ @S.S.Anne True, but that doesn't provide the opportunity for the program to skip the while loop completely. \$\endgroup\$
    – Linny
    Commented Jan 25, 2020 at 19:05
  • \$\begingroup\$ @Linny I don't see how it would make a difference except for code clarity. \$\endgroup\$
    – S.S. Anne
    Commented Jan 25, 2020 at 19:06
2
\$\begingroup\$

Your code has bad scaling, since with more youtubers, more code and more ifs are needed. This can be solved by using a dict with each youtuber name as key and each message as value:

youtubers = {}
youtubers['dr. phil'] = 'You are an awesome lad!'
youtubers['james charles'] = 'Errmm. Ok...'
# others entries here

# querying
print(youtubers[youtuber])

This will reduce querying time since getting an item in a python dict uses constant time in average case.

You can also create the dictionary using less code with comprehension dict, assuming you have a youtuber's names list and a message's list

youtuber_names = ['youtuber1', 'youtuber2']
messages = ['message1', 'message2']

# this will create an equivalent dict 
youtubers = {youtuber_names[i]: messages[i] for i in range(len(youtuber_names)}
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Better to define the constant all at once rather than build it up over one line per element. \$\endgroup\$ Commented Jan 23, 2020 at 20:54
0
\$\begingroup\$

@Linny. First of all thanks for your solution. It's a real improvement and very good explained. This push me to try to enhance it even more.

This new proposal is based on your solution, and it includes two additional changes. Both of them according to Jeff Bay's Object Calisthenics (some basic rules to write better Object Oriented code):

Rule 2: "Don´t use the else keyword".

There's only one else. See below:

    if youtuber in youtube_response:
        print(youtube_response[youtuber])
    else:
        print('I dont know that one. Ill check them out.')

Luckily, the whole 4 lines (if/else) can be replaced by one line:

    print(youtube_response.get(youtuber, 'I dont know that one. Ill check them out.'))

Get behaviour is the following: get(key[, default]) return the value for key if key is in the dictionary, else default.

Rule 1: "Only One Level Of Indentation Per Method."

Run_app function has two additional level of indentation:

  • First line is considered as level 0 ´While True´
  • Level 1 starts below first while
  • And level 2 starts below second while
def run_app():
    while True:
        # Level 1
        favorite_youtuber()
        again = input("Play again? (Y/N)").lower()
        while again not in "yn":
            # Level 2
            print("Please enter Y/N!")
            again = input("Play again? (Y/N)").lower()
        if again == "n":
            # Level 2
            break
    print("Thank you for using this app!")

This function in fact has two responsibilities, run_app and check if play_again. My proposal is to extract this second responsibility to another function. This should improve code readability and lower its complexity:

def run_app():
    while True:
        favorite_youtuber()
        if not play_again():
            break
    print("Thank you for using this app!")


def play_again():
    while (again := input("Play again? (Y/N)").lower()) not in "yn":
        print("Please enter Y/N!")
    return again == "y"

I hope this can be helpful

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