Shrinking and expanding square

Question

I'm a computer science student who's just starting out with algorithms and their implementation. I wrote this code for a dynamically morphing square. I was wondering if I could get a review on it. I apologize if the comments are excessive, this is how I submitted on it at school.

import time  #imported the time module to make the loop stop for a .5 sec before the next iteration

user_input = int(input("Input a number greater than 3: ")) #gets the user input in the form of an integer

def pattern(count): #created a function here called pattern, and gave it a parameter called count
    while user_input > 3: #This loop works as long as the integer entered is > 3
        min = 0 #initialize the variable min as the starting corner of the square i.e zero point.
        max = count - 1 #initialize the variable max as "count -1". This is because this variable will be updated by the loops through each iteration.
        for run in range(0,count): #first loop to control the iterations of the square
            print(run) #This will print the number of the iteration we are in at the time. This is put here as a check to see if the loop works as intended.
            for i in range(0,count): #this loop goes from row 0 to row N, where N is the value entered by the user.
                for j in range(0,count): #this loop goes from column 0 to column N, where N is the value entered by the user. This will be responsible for printing the stars in the square, from column to column.
                    if (min <= max and min <= i and i <= max and min <= j and j <= max) and (i == min or i == max or j == min or j == max): #this branch checks if ceertain cinditions are met. then prints * if they are. The conditions are explained below.
                        print("*",end="") #prints * without a line break after every iteration of its loop
                    else:
                        print(".",end="") #prints . without a line break after every iteration of its loop
                print("") #prints a new line
            print("") #prints a new line
            if run < count / 2: #in this branch, if the iterations of the boxes < half of the user input, the variable min will be incremented by 1
                min += 1        #and the variable max will be reduced by 1
                max -= 1
            if run >= count / 2: #if the iterations of the boxes > half of the user input, min will be reduced by 1 and  max will increase by 1.
                min -= 1
                max += 1
            time.sleep(.5) #pauses for .5 secs between iterations of the square.


if user_input >= 3: #if the user input > 3, the function called pattern will be called 
    pattern(user_input)
else: #if the user input < 3, there will be an error message printed
    print("Enter a number greater than 3 ")




# The first branch in this function checks if the variable min<max, if min < i <= max, if min < j <= max, if i = min, if j = min, if i = max and if j = max.
# These conditions confine the points that are printed to the space between min and max, in both the rows and columns of the square.
# The four final conditions are there to allow the square to dissappear creating a set of dots and display the first square where both rows and columns are equal to the number entered by the user.

The results are as follows:

Answer 1

A few things stand out:

• The many levels of indentation. Consider putting the contents of one of the loops into a function, and calling that instead. For example, all the printing work could be done in a separate function. I'd suggest a maximum number of four levels of indentation

• The amount of comments. Comments are to explain how something works (an algorithm), not what you're doing. The latter should be obvious from the code. And definitely not a comment per line.

  For example:

  def pattern(count): #created a function here called pattern, and gave it a parameter called count
  

  is an unnecessary comment: Python programmers know def means to create a function, and the parameter (I think Pythoneers tend to use the term argument instead) is obvious as well.

• The lengthy lines. The suggested maximum line length is 79 characters, though preferences and local standards may differ. A situation like here provides a good example about a limited line length: you have put code in a code block on a web site, which has set a limited viewing width. And you can immediately see why long lines can be bad, in particular: hard to read. (Hint: other than removing all those comments, consider putting comment lines on the line preceding the code).

• If a user enters something that can't be converted to an int, they'll get presented with a traceback. That is not very user friendly. Consider testing this appropriately (error checking is, for some code at least, > 50% of the actual code).

• You have if run < count / 2:, and below that if run >= count / 2:. Since run and count don't change in between, an else instead of the second if-statement is probably clearer.

Answer 2

I agree with the two answers posted above. There are a few additional points I would add:

1. Your massive control statement can be simplified:

if (min <= max and min <= i and i <= max and min <= j and j <= max) and (i == min or i == max or j == min or j == max)

In particular you can delete min <= max

You can also rewrite the final condition, (i == min or i == max or j == min or j == max), more simply as (i in (min, max) or j in (min, max))

Finally you can rewrite min <= i and i <= max as min <= i <= max which is both shorter and easier to understand.

2. Why not use a list comprehension to replace your inner for loop?

Seems like you can rewrite

        for j in range(0,count): #this loop goes from column 0 to column N, where N is the value entered by the user. This will be responsible for printing the stars in the square, from column to column.
            if (min <= max and min <= i and i <= max and min <= j and j <= max) and (i == min or i == max or j == min or j == max): #this branch checks if ceertain cinditions are met. then prints * if they are. The conditions are explained below.
                print("*",end="") #prints * without a line break after every iteration of its loop
            else:
                print(".",end="") #prints . without a line break after every iteration of its loop

as something like (I haven't tested this):

def should_print_asterisk(i, j, min, max): return (min <= i <= max and min <= j <= max) and (i in (min, max) or j in (min, max)))

and then inside the i based for-loop:

print(["*" if should_print_asterisk(i,j,min,max) for i in range(0, count) else "."])

3. Check the logic of your control statements to keep them efficient

You have two if statements that are mutually exclusive? Why not refactor this into an if...elif combo?

4. Refactoring logic to eliminate variables

The information contained in your pattern function variables min and max is actually identical. They both represent an offset from the outer edge of the shape. Consider whether you should simply have one variable, something like border_width. In this case, your control statement if run < count/2 ... could be reduced to:

            if run < count / 2: #in this branch, if the iterations of the boxes < half of the user input, the variable min will be incremented by 1
            border_width += 1
        else:
            border_width -= 1

This renaming also shouldn't impact your for loop variables much. min -> border_width and max -> count - 1 - border_width

*Note I typed out the code above without running it so please let me know if there is a syntax error I can fix if you can't figure it out.

Answer 3

I notice that Jamal edited out your mention of the comments, but as Evert stated, please don't do it like that. Despite all the comments, the actual intent of the code isn't specified anywhere in the question!

Without the required output specified exactly, it's hard to suggest algorithmic improvements. However, the code doesn't seem to behave correctly for odd numbers (it skips the last ring).

---

main()

Put all the "main" code together below the function, rather than putting the user input at the top. user_input is passed to pattern() as the variable count so you don't need to access user_input at all. (See also later...)

A common python idiom is to put the program entry point in a main() function, and add a protected call to that function like so:

def main():
    # ... program code starts here!

# this prevents main() being called when loaded as a module
if __name__ == "__main__":
    main()

Your prompt message is technically incorrect, since you do accept 3 as an answer. But, the program doesn't actually do anything in this case. It would be more consistent to produce an error for 3 as well. Note also that numbers greater than 3 includes floating point values which will still fail to convert to an integer. The int() function will throw a ValueError in such cases. So you need to use try and except clauses like so.

try:
    i = int(input("Enter an integer greater than 3: "))

except ValueError:
    print("Invalid input!")

---

pattern()

You want an infinite loop here, so instead of checking user_input > 3 (or count > 3), you should just explicitly make the loop infinite:

while True:
    #...

Notice that for even numbers, you don't actually have enough runs to complete the pattern. The ring with the radius before the max is only printed on the way down, but not the way up. The simplest way to fix this is to test for odd numbers, and increase the number of runs by one in those cases:

    runs = count

    if count % 2 != 0:
        runs += 1

    for run in range(0, runs):
        #...

With this the code appears to work correctly.

The min max solution does seem a bit complicated though. There may well be a simpler way to approach this (e.g. just stepping a radius from zero to count and back, and drawing a circle with that radius), but it's hard to suggest one without knowing the exact problem requirements.