HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

10 green bottles assignment

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
greenassignmentbottles

Problem

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_t

Solution

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.

Code Snippets

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)

Context

StackExchange Code Review Q#93809, answer score: 8

Revisions (0)

No revisions yet.