9
\$\begingroup\$

I worked up a quick diamond program that takes only odd numbers while returning an error with even numbers or 0.

The code looks rather ugly to me, especially with the recurring int(). I haven't found out how to clean that up with my ENTER to quit.

while True:
    number = raw_input("Enter an odd number or hit ENTER to quit program: ")
    if number == "":
        print "\nQuitting program...\n"
        break
    elif int(number) == 0:
        print "\nCannot print symbols for a zero value, try again.\n"
    elif int(number) % 2 == 0:
        print "\nThat's an even number, try again.\n"
    else:   
        for i in range((int(number))+1):
            if i % 2 == 0:
                None
            else:
                print ("*" * i).center(int(number))

        for i in range((int(number))-1):
            if i % 2 == 0:
                None
            else: 
                print ("*" * ((int(number)) - (i + 1))).center(int(number))
\$\endgroup\$
  • 2
    \$\begingroup\$ Why do you make a separate condition for 0? Zero IS even. \$\endgroup\$ – BeetDemGuise Oct 30 '14 at 12:31
11
\$\begingroup\$
  • You can test for emptiness of number by just if not number
  • After testing for emptiness, you could go straight to an int by adding number = int(number)
  • Inside the loops, when you test for i % 2 == 0:, and then put None as the loop body (that should really be pass), you could just test for if i % 2 != 0:, and replace None with the else body, and remove the else.
  • 0 is even, so you can remove that test.
  • You can replace int(number) % 2 == 0 with not int(number) % 2.
  • Looking at the for loops, the range could be changed to range(1, number-1, 2), removing the need for an if statement.
  • You can also invert the last range, so the subtracting from number and 1 is not needed.
  • Python automatically adds newlines, so the \ns aren't really needed.

That leaves us with (after removing a few useless brackets):

while True:
    number = raw_input("Enter an odd number or hit ENTER to quit program: ")
    if not number:
        print "Quitting program..."
        break
    number = int(number)
    if not number % 2:
        print "That's an even number, try again."
    else:   
        for i in range(1, number + 1, 2):
            print ("*" * i).center(number)
        for i in range(number - 2, 0, -2):
            print ("*" * i).center(number)

If you want to go functional, you can move your user input checking into a function. In this function you could also check if the user inputs a non-number, such as abc, by catching the exception or using str.isdigit:

def get_input():
    while True:
        number = raw_input("Enter an odd number or hit ENTER to quit program: ")
        if not number:
            return  # return None when the user wants to quit
        if number.isdigit() and int(number) % 2:
            return int(number)  # return the number, as its odd
        print "That's not a valid input (must be an odd integer), try again."

With this function, the rest of your program reduces to:

for number in iter(get_input, None):
    for i in range(1, number + 1, 2):
        print ("*" * i).center(number)
    for i in range(number - 2, 0, -2):
        print ("*" * i).center(number)

print "Quitting program..."

iter calls the function and gives its value until the function returns None (the sentinel value).

On a stylistic note:

  • You should always have spaced around operators, so range(int(number)+1) should be range(int(number) + 1) (that's PEP8).
  • You may want to add a big fat comment at the start of your program describing what it does.

Also, nice use of str.center, it avoided having too many loops!

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

In Python, pass is how you indicate that nothing happens; you shouldn't just put None on a line on its own. Alternatively, note that:

if x:
    pass
else:
    y()

is the same as:

if not x:
    y()

except that the latter is much neater.


You are right that the repeated int is bad form. You might find Asking the user for input until they give a valid response useful in helping you refactor to make number an int as early as possible.

\$\endgroup\$
2
\$\begingroup\$

Well you can just assign a converted value after your check. After that, get rid of all dead code, move the conditions around a bit and you'd end up with something like this. I'd say that continue-ing earlier to get rid of another level of indentation is worth it as well.

In the documentation for range you can see that the longer form has a step argument, which you can use in this case to get rid of the additional test by incrementing by 2.

while True:
    number = raw_input("Enter an odd number or hit ENTER to quit program: ")
    if number == "":
        print "\nQuitting program...\n"
        break

    number = int(number)
    if number % 2 == 0:
        if number == 0:
            print "\nCannot print symbols for a zero value, try again.\n"
        else:
            print "\nThat's an even number, try again.\n"
        continue

    for i in range(0, number + 1, 2):
        print ("*" * i).center(number)

    for i in range(0, number - 1, 2):
        print ("*" * (number - (i + 1))).center(number)

And then factor it into functions (e.g. the drawing part) and it will be much nicer to handle as well.

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

This is mainly for structure.

I would have the quiting operations outside the while loop, and have the condition on the while loop as the quitting condition. This makes the code more readable and you avoid having to use brake as a flow control.

You can also use the string class's isdigit() function to check if the input is a number and then simply convert it once. This avoids having to do many casts and also prevents the user from braking the program with non numerical input.

number = raw_input("Enter an odd number or hit ENTER to quit program: ")
while number != "":
    if number.isdigit():
        number = int(number)
        if number == 0:
            print "\nCannot print symbols for a zero value, try again.\n"
        elif number % 2 == 0:
            print "\nThat's an even number, try again.\n"
        else:   
            # rest of code ...
    else:
        print "\nCannot print symbols for non numerical values, try again.\n"
    number = raw_input("Enter another odd number or hit ENTER to quit program: ")
print "\nQuitting program...\n"
\$\endgroup\$
1
\$\begingroup\$

I don't understand your program, but I've syntactically translated it into this:

def getInput():
    number = raw_input("Enter an odd number or hit ENTER to quit program: ")
    if (not number):
        return -2
    try:
        n = int(number)
    except:
        print "\nPlease input only integer numbers\n"
        return 0
    if not n:
        print "\nCannot print symbols for a zero value, try again.\n"
    elif not n % 2:
        print "\nThat's an even number, try again.\n"
    return n

if __name__ == "__main__":
    n = 1
    while n > 0:
        n = getInput()
        if not n % 2:
            continue    
        for i in range(1, n+1, 2):
            print ("*" * i).center(n)
        for i in range(1, n-1, 2):
            print ("*" * (n - (i + 1))).center(n)
    print "\nQuitting program...\n"

Splitting the code into functions is nearly always a good idea, basically in the function to get the input you focus on these annoying cases where the input is not what you would expect and return something already under control to the upper function, there you use that value accordingly.

\$\endgroup\$

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.