Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I would like the program to ask the user if they want to add a color. If they say yes, then ask what color to add. Then after that happens, once the program would ask if you want to, run it again. If they say no to add a color, the program ask if you want to remove a color from the list. If they say yes, it would then ask what color to remove, with error handling built in if the color wasn't in that list. Then after that happens once the program would ask if you want to run it again.

Just wanted to point out I do not know a lot about Python, so please don't be harsh, but I can take constructive criticism.

import socket # imports the socket module 
import sys # imports the sys module
import console # imports the console module 

usr = raw_input('What is your name? \n').title() # asks for users name for the hostname information
sname = socket.gethostname() # sets sname to gethostname
print '\nThis program is being run by', usr, 'on machine name', sname # displays host information

mylist = ['Red', 'White', 'Blue', 'Yellow', 'Green', 'Orange'] # sets mylist to colors
templist = list(mylist) # sets templist as mylist
runagain = 'y' # sets runagain to a y values

while runagain == 'y': # for looping
print '\nThe current colors are:', templist # print out list so user knows what colors are already there

clradd = raw_input('\nWould you like to add any colors to the list? y or n\n')

while clradd == 'y':
    colors = raw_input('\nWhat colors would you like to add to the list? Please type one color. \n').title()
    templist.append(colors) # appends color to the list

    print templist # prints out new list
    print '\nAscending and Descending'  # print out ascending and descending lists
    print '-' * 30
    print '\nAscending (high to low): ', sorted(templist, reverse=True)
    print '\nDescending (low to high): ', sorted(templist, reverse=False)

    clrdel = raw_input('\nWould you like to remove any colors from the list? y or n\n')

while clradd == 'n':
    clrdel = raw_input('\nWould you like to remove any colors from the list? y or n\n')

    while clrdel == 'y':
            try:
                removecolor = raw_input('\nWhat colors would you like to remove? ').title() # asks user color to remove
                templist.remove(removecolor) # removes color from the list
            except ValueError:
                print 'Looks like that color was not in the list, try again.'
                continue    

            print 'Updated list\n', templist # prints out updated list

            runagain = raw_input('\nWould you like to run this program again again? y or n')

            if runagain == 'n': # if runagain is n
                console.hide_output() # hide the output 

    while clrdel == 'n':
        runagain = raw_input('\nWould you like to run this program again again? y or n') # asks user to run again

        if runagain == 'n': # if runagain is n
            console.hide_output() # hide the output 
            sys.exit() # stops the program
share|improve this question
1  
This post is now unlocked but is being monitored by moderators. Please do not make edits that invalidate current answers. See this meta post for instructions on how to edit your post after it has been reviewed –  rolfl Mar 29 at 18:31

2 Answers 2

up vote 7 down vote accepted
import socket # imports the socket module 

The comment is completely pointless. But I cannot help it, my eyes naturally read it, my brain processes it, only to realize it says nothing new. Let your code speak for itself, avoid comments as much possible.

print '\nThis program is being run by', usr, 'on machine name', sname

Maybe it's somewhat more readable if you use standard Python formatting like this instead:

print '\nThis program is being run by %s on machine name %s' % (usr, sname)
runagain = 'y'

Instead of setting this to a string, and later using == 'y' to evaluate, it would be better to use proper booleans for this kind of thing. For example:

Similarly, instead of:

clradd = raw_input('\nWould you like to add any colors to the list? y or n\n')

while clradd == 'y':

You could write like this:

clradd = raw_input('\nWould you like to add any colors to the list? y or n\n') == 'y'

while clradd == 'y':

Try-catch block writing style:

try:
    removecolor = raw_input('\nWhat colors would you like to remove? ').title()
    templist.remove(removecolor)
except ValueError:
    print 'Looks like that color was not in the list, try again.'
    continue    

It's better to keep the try block as small as possible. For example, the removecolor assignment should be moved out from it. Also, since you simply want to check if the given color is in the list of colors, it would be cleaner to write that exactly:

removecolor = raw_input('\nWhat colors would you like to remove? ').title()
if removecolor in templist:
    templist.remove(removecolor)
else:
    print 'Looks like that color was not in the list, try again.'
    continue

There are couple of other things too, globally:

  • Incorrectly indented code
  • Incorrectly using while instead of a simple if
  • Duplicated code blocks
  • sys.exit is a bit harsh way to exit, try to avoid if possible

Putting it all together:

import socket

usr = raw_input('What is your name?\n').title()
sname = socket.gethostname()
print '\nThis program is being run by %s on machine name %s' % (usr, sname)

mylist = ['Red', 'White', 'Blue', 'Yellow', 'Green', 'Orange']
templist = list(mylist)
runagain = True

while runagain:
    print '\nThe current colors are:', templist

    clradd = raw_input('\nWould you like to add any colors to the list? y or n\n') == 'y'

    if clradd:
        colors = raw_input('\nWhat colors would you like to add to the list? Please type one color. \n').title()
        templist.append(colors)

        print templist
        print '\nAscending and Descending'
        print '-' * 30
        print '\nAscending (high to low): ', sorted(templist, reverse=True)
        print '\nDescending (low to high): ', sorted(templist, reverse=False)

    clrdel = raw_input('\nWould you like to remove any colors from the list? y or n\n') == 'y'

    while clrdel:
        removecolor = raw_input('\nWhat colors would you like to remove? ').title()
        if removecolor in templist:
            templist.remove(removecolor)
        else:
            print 'Looks like that color was not in the list, try again.'
            continue

        print 'Updated list\n', templist
        break

    runagain = raw_input('\nWould you like to run this program again again? y or n\n') == 'y'
share|improve this answer
    
thanks for your help, since I'm only a beginner. My teacher makes me comment almost every line, thats why import socket was commented. –  techteej Mar 28 at 0:50
    
@TjF You only need comments when it's hard to understand code from a quick glance. If you're not sure when to comment - you can try having one comment per logical block of code - briefly describing what the following block is doing: # Repeatedly asking a user for color until he/she gets it right. –  Ruslan Osipov Mar 28 at 0:54
    
@RuslanOsipov I actually knew that myself, but my teacher asks as part of every assignment that each line of code to be commented. –  techteej Mar 28 at 0:57

Your code is a bit hard to read due to you not following PEP8. Also, your comments are explicitly verbose - they don't have to duplicate the code. And in your case you need very few comments explaining what is happening overall. You can get a more thorough functional code review if you fix the style and edit the question - right now it is quite hard to edit and understand what is going on due to module not having descriptive variable naming and overall descriptive comments.

import socket # imports the socket module 
import sys # imports the sys module
import console # imports the console module 

You don't need comments there. Also, it's good to organize imports alphabetically. See PEP8 section: http://legacy.python.org/dev/peps/pep-0008/#id16.

usr = raw_input('What is your name? \n').title() # asks for users name for the hostname information

Limit line lengths to 80 characters (applies to all the code below as well). No need for a comment here, it also adds ambiguity. Is it a hostname or a name input? Rename usr to user - cropping one letter out does not help readability.

sname = socket.gethostname() # sets sname to gethostname

Again, no need for a comment is needed here. Everywhere below you do not need comments since you are duplicating your code in English. More descriptive variable name as host_name would be good as well.

runagain = 'y' # sets runagain to a y values

No comment is needed. Variable naming convention is lowercase_with_underscores. So run_again is a better name. Also, True is a better value here.

while runagain == 'y': # for looping
print '\nThe current colors are:', templist # print out list so user knows what colors are already there

Indent second line. Otherwise it'll throw an error.

clradd = raw_input('\nWould you like to add any colors to the list? y or n\n')

Better variable naming. Try color_add or add_color.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.