3
\$\begingroup\$

I was working through another question here and have hacked up an answer:

def get_rows():
""" Get height of triangle from user"""

    while True:
        rows = input('Enter the number of rows:  ')
        if 3 <= rows <= 33:
            return rows

def triangle(rows, invert=True):
""" Print the outline of a triangle, of variable size. 
    Print out a regular or inverted version of the triangle as necessary """

    if invert:
        height = -1 * rows
    else:
        height = 0

    # inner padding for min height (3)
    inner_buffer = [0, 1, 3]
    while len(inner_buffer) <= rows:
        inner_buffer.append(inner_buffer[-1]+2)

    level = 0        
    while level <= rows:
        outer_padding = ' '*(rows - abs(height))

        if height == 0:
            print(outer_padding + '*')
        else:
            inner_padding = ' '*( inner_buffer[ abs(height) ] )
            print(outer_padding + '*' + inner_padding + '*')

        height += 1
        level += 1

At this stage I am working to re-implement this two ways using: recursion and itertools.

But I am looking at this and wonder if it would be a good place to use decorators. If someone is happy to implement one triangle (either regular or inverted, I don't mind) I would love to see the difference in the 3 implementations.

\$\endgroup\$
4
\$\begingroup\$

Decorators are a tool that you use when you have many functions that share a behaviour, for example a particular calling convention or treatment of results. See the Python Decorator Library on the Python wiki for some examples, and this answer on Stack Overflow for a tutorial.

Here you only have two functions, so it doesn't seem likely that decorators will be helpful. Use the right tool for the job!

Anyway, comments on your code.

  1. No docstrings! What do these functions do and how do you call them?

  2. The else: continue in get_rows() is unnecessary.

  3. The regular keyword argument to triangle is ignored.

  4. Your function triangle seems very long for what it's doing. Why not use Python's format language to do the padding? Like this:

    def triangle(n, invert=True):
        """Print inverted triangle of asterisks (with `n`+1 rows) to standard output.
        Print it the right way up if keyword argument `invert=False` is given.
        """
        for i in reversed(xrange(n + 1)) if invert else xrange(n + 1):
            print('{:>{}}{:>{}}'.format('*', n - i + 1, '*' if i else '', 2 * i))
    
\$\endgroup\$
2
  • 1
    \$\begingroup\$ +1 + two comments: 1) I'd put some parens on the thing being iterated (or separate it in a variable). 2) That string being formatted is really complicated: I'd probably write a simple string concatenation. \$\endgroup\$ – tokland Mar 26 '13 at 14:20
  • \$\begingroup\$ Hi Gareth, thank you for your answer. Will re-edit according to first 3 points. Lol, I will have to wrap my head around your formatting - thank you very much for that! Appreciate your thoughts on decorators and using the right tools. Many thanks! \$\endgroup\$ – Nick Burns Mar 26 '13 at 19:07
0
\$\begingroup\$

I would suggest that you read PEP8. Also, you can reduce code size by using certain coding styles:

height = -1 * rows if rows else 0

It should look more readable.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the reference to PEP8 - have certainly learned and tidied up many things through reading that. After a cursory look, it seems PEP8 discourages putting multi-line stmts on the same line, BUT... in simple cases (and I agree that the case you've given above is straight forward) it may be ok. Seems there is some lee-way on this. nb: the stmt above, does not test if invert so does not quite accomplish the intended task. Will edit above with a more tidy solution (using recursion, string formatting (as suggested by Gareth) and tidier logic. Thanks for your comments people! \$\endgroup\$ – Nick Burns Mar 28 '13 at 7:13
  • \$\begingroup\$ hmm, I think there is a typo error, there will be if invert. Also, you can have any number of condition in a if statement. \$\endgroup\$ – vivek Mar 28 '13 at 8:26
  • \$\begingroup\$ I do see your point here, the statement could be written height = -1 * rows if invert else 0 and still achieve the desired end. PEP8 includes, "While sometimes it's okay to put an if/for/while with a small body on the same line, never do this for multi-clause statements." This is not definitive, what is the general concensus re. compacting code in this sense? \$\endgroup\$ – Nick Burns Mar 28 '13 at 9:54
  • \$\begingroup\$ Hmm, but you see its not a typical if/else compound statement where you have two clauses (header and suite). Its a simple expression, and its perfectly fine to use it like this. \$\endgroup\$ – vivek Mar 28 '13 at 10:59
  • \$\begingroup\$ Oh right! That makes sense to me now. thanks! \$\endgroup\$ – Nick Burns Mar 28 '13 at 11:06

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.