3
\$\begingroup\$

This code was for a university assignment. It's well passed now so don't be scared of giving me false grades, for example. I was wondering what could be done better, or bits of code that are horribly inefficient or are bad practices.

def main():
    import time
    while True:
        try:
            num1 = int(input("Pick a number between 10 and 30: "))
        except ValueError:
            print("That's not a number!")
            continue
        if num1 > 30 or num1 < 10:
            print("Not Valid!")
            continue
        break           
    while True:
        try:
            hue = str(input("Pick a colour; Red, Green, Blue: ")) 
        except ValueError:
            print("Letters not numbers!") 
            continue                        
        if  (hue == "Red") or (hue == "red") or (hue == "Green") or (hue == "green") or (hue == "Blue") or (hue == "blue"):

            break
    print("Generating File")
    time.sleep(3)
    numbers ='no', 'One', 'Two', 'Three', 'Four', 'Five', 'Six', 'Seven', 'Eight', 'Nine', 'Ten', 'Eleven', 'Twelve', 'Thirteen', 'Fourteen', 'Fifteen', 'Sixteen', 'Seventeen', 'Eighteen', 'Nineteen', 'Twenty', 'Twentyone', 'Twentytwo', 'Twentythree', 'Twentyfour', 'Twentyfive', 'Twentysix', 'Twentyseven', 'Twentyeight', 'Twentynine', 'Thirty' 
    text_one = hue +' bottle%s\nHanging on the wall'
    text_two = "And if one " + hue + " bottle\nShould accidentally fall\nThere'll be"
    text_three =' \n'

    with open(numbers[num1] + ' ' + hue + ' Bottles.txt', 'w') as a:
        for l in range(num1, 0, -1):                                              
            a.write(numbers[l]+ ' ')
            if l == 1:
                a.write(text_one % '' +'\n')
            else:
                a.write(text_one % 's' +'\n')

            a.write(numbers[l] + ' ')
            if l == 1:
                a.write(text_one %  '' + '\n') 
            else:
                a.write(text_one % 's' +  '\n')
            a.write(text_two + ' ')
            a.write(numbers[l-1] + ' ')
            if (l - 1) ==1 :
                a.write(text_one % ''+'\n')
            else:
                a.write(text_one % 's'+'\n')
            a.write('\n' + '\n')


if __name__ == '__main__':
    main()

This should be saved to a folder and ran from the Python file it's saved as, not ran in an IDE.

\$\endgroup\$
  • \$\begingroup\$ One thing to work on is not repeating yourself. For example you have a lot of a.write calls with some string interpolation. You could put that in a small function. \$\endgroup\$ – roby Jun 17 '15 at 5:21
  • \$\begingroup\$ if (hue == "Red") or (hue == "red") or (hue == "Green") or (hue == "green") or (hue == "Blue") or (hue == "blue"): break looks awful. You could shorten it by using if (hue.lower() == "red") or (hue.lower() == "green") or (hue.lower() == "blue"): break or simply add "red", "green", "blue" to a list called colors and then check if hue.lower() is present in colors. \$\endgroup\$ – Spikatrix Jun 17 '15 at 5:45
  • 1
    \$\begingroup\$ @CoolGuy or a set called colors for greater efficiency - hue.lower() in {'red', 'green', 'blue'} \$\endgroup\$ – jonrsharpe Jun 17 '15 at 7:05
  • \$\begingroup\$ Case insensitive string comparison using lower() is broken. See this answer for the correct approach to case insensitive comparison in Python. \$\endgroup\$ – Johnbot Jun 17 '15 at 8:49
8
\$\begingroup\$

A good place to start is to break your big main method into smaller chunks of work. Then you can focus on improving the code in each section.

Currently your main method does many things, it reads user input, builds up a song and writes it to a file. Here's how you might rewrite it:

def main():
  num_bottles = get_num_bottles_from_user()
  hue = get_hue_from_user()
  song = build_song(num_bottles, hue)
  out_filename = create_out_filename(num_bottles, hue) 
  write_song_to_file(song, out_filename)

The function is easier to read if all the calls it makes are at a similar level of abstraction. The main method doesn't do low level operations like input reading or file writing, it tells a story.

\$\endgroup\$
6
\$\begingroup\$

It's considered a bad practice to import packages in the middle of code, with rare exceptions. Import at the top of the file.

Another bad practice is using numbers in variable names, like num1.

I would find this a slightly simpler and more natural way of validating and exiting the input loop:

    if 10 <= num1 <= 30:
        break
    print("Not Valid!")

A simpler way to check if hue is one of the supported colors: hue.lower() in ('red', 'green', 'blue')

In this code, and the lines that follow after that, I cannot tell if that's a small L or a big i. It would be better to avoid such letters as variable names. It's also good to avoid single-letter variable names in general.

for l in range(num1, 0, -1):   
\$\endgroup\$
4
\$\begingroup\$

Taking input while making some validation is very common in beginner programs but you wrote two similar while True loops. I would go a bit more general at the cost of the code being a bit longer and write:

from general_input import general_input

def main():
    num1 = general_input("Pick a number between 10 and 30: ",
                         type_=int, set_=range(10,30+1))
    hue = general_input("Pick a colour; Red, Green, Blue: ",
                        set_=("red", "blue", "green"),
                        cap_sensitive=False)

Where general_input is a function that handles all the cases of possibly malformed input by printing meaningful messages.


You also have the exact same code two times, so use a loop:

for _ in range(2):
    a.write(numbers[l]+ ' ')
    if l == 1:
        a.write(text_one % '' +'\n')
    else:
        a.write(text_one % 's' +'\n')
\$\endgroup\$
2
\$\begingroup\$

Maintainable code has proper naming. Single letter variables are only acceptable for indexes in loops.

numbers contains a bunch of strings. Although I understand why it's called numbers, it's a counter-intuitive name.

print("Generating File") is in a place where no actual generating takes place. The appropriate place would be a function which actually generates something, not right before a time.sleep statement.

with open(numbers[num1] + ' ' + hue + ' Bottles.txt', 'w') as a:

That's num1 and a having non-descriptive names. num1 is input from the user. userInput isn't a great name, but better than num1. Perhaps even inputNumber or userNumber. a could be named file, fileStream, numberFile or something similar.

for l in range(num1, 0, -1):

@janos already stated this, but it's very important. Never use l for a variable name. i is acceptable in many languages, l is not. Depending on your font it may easily be confused for something else.

Do note I'm using mixedCase while Python officially prefers underscores. It's a matter of preference. Reading the naming conventions in PEP8 may be a good idea anyway.

\$\endgroup\$
1
\$\begingroup\$

while True/continue/break structures are generally difficult to read, I personally prefer something like:

valid_input = False
while not valid_input:
    # user input and validation here
    # when the input is valid, set valid_input to True

This way you know what the loop is about simply looking at the exit condition.

Then you can make it a function:

# don't be afraid of long names
def prompt_number_between_10_and_30():
    valid_input = False
    while not valid_input:
        # User input and validation here
        # When the input is valid, set valid_input to True
    return user_input

With these two modifications, it's interesting to note you don't have to comment on what the code does, the naming does it for you.

\$\endgroup\$
  • \$\begingroup\$ prompt_number_between(start,end) would be a better way to define for the second function. \$\endgroup\$ – Pranav Raj Jun 17 '15 at 20:30
  • 2
    \$\begingroup\$ Moving the prompt into a function is a good idea. Introducing a valid_input flag variable is a bad one. The same thing could be accomplished with while True: user_input = input(…); if …: return user_input. \$\endgroup\$ – 200_success Jul 17 '15 at 21:14

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.