6
\$\begingroup\$

I've written two functions:

  1. is_integer(), which tries to get a positive or a negative integer as input and returns the same value.
  2. square(), which takes this returned value as parameter and returns square of the same.

The code in square() uses a for-loop that squares the integer by 'repetitive addition.' Finally, the script prints out the square of the integer returned by square().

Please review my code, point out errors of any kind and make suggestions.

# Squares an integer by repetitive addition
# Uses a for-loop
def square(integer):
    result = 0

    for element in range(1, abs(integer)+1):
        result += abs(integer)
    return 'Square of ' + str(integer) + ' is ' + str(result)

# Makes sure the input is an integer
def is_integer():
    number = raw_input('Enter an integer: ')

    if number.isdigit():
        number = int(number)
        return number
    elif number[0] == '-' and number[1:].isdigit():
        number = int(number)
        return number       
    else:
        print '%r is not an integer' % number
        print 'To try again, press Enter key'
        print 'Otherwise, press Ctrl+C keys to abort'
        raw_input('>>> ')

        return is_integer()

# Square() takes as parameter the value returned by is_digit()
# Print the result returned by square()
print square(is_integer())
\$\endgroup\$

4 Answers 4

10
\$\begingroup\$

Improving square

There are several bad practices in square:

  • Multiple calls to abs(integer) when it could be calculated just once and reused
  • Instead of range(1, x + 1) you could write range(x)
  • When you don't need to use the loop variable element for anything, a common convention is to name it _ instead
  • Converting integer and result to string is tedious. It would better to use a formatted string expression

Making the appropriate changes the method becomes:

def square(integer):
    result = 0
    abs_integer = abs(integer)

    for _ in range(abs_integer):
        result += abs_integer
    return 'Square of {} is {}'.format(integer, result)

Improving is_integer

In addition to what @Jamal already pointed out, the method has other problems too:

  • The way you check if the user input is numeric is overly tedious
  • Naming the input as "number" is not great, because it might be text

A better way to write it (also fixing the name and making it non-recursive):

def prompt_integer():
    while True:
        text = raw_input('Enter an integer: ')

        try:
            return int(text)
        except ValueError:
            print '%r is not an integer' % text
            print 'To try again, press Enter key'
            print 'Otherwise, press Ctrl+C keys to abort'
            raw_input('>>> ')

Why Python 2.7 ?

You're using Python 2.7. Note that there are no more planned releases for Python 2.7 beyond 2015 June. Consider migrating to Python 3, it's the future.

\$\endgroup\$
4
  • \$\begingroup\$ I'm still a beginner and Python is my first language. Most of the books and online classes I'm using to learn still mention Python 2.x explicitly. Sure I'll consider switching to Python 3 as soon as I finish my introductory classes. I consider your improvements a great lesson. \$\endgroup\$
    – user61142
    Jan 14, 2015 at 22:04
  • \$\begingroup\$ I agree that naming the input as "number" is not great, but are you sure naming it as "text" is? because the input is expected to be a number. Please correct me if I'm wrong. I'm aware that raw_input() converts input to a string and if all characters in the string are digits, we can then convert the string to a number if necessary. \$\endgroup\$
    – user61142
    Jan 14, 2015 at 23:38
  • \$\begingroup\$ Wouldn't it be best to use new style string formatting instead of old style? \$\endgroup\$ Jan 15, 2015 at 6:28
  • \$\begingroup\$ @SethMMorton certainly it would be, well spotted! Updated now. \$\endgroup\$
    – janos
    Jan 15, 2015 at 6:30
7
\$\begingroup\$

It looks like is_integer() should be renamed. A function name with "is" in it could mean that it returns a boolean-type value, but this one returns a number. Perhaps you should have this function only check for a valid number, then have another one return the number to the caller.

Also, I don't think it should have a recursive call. It would be better as a loop that will continue until a valid number has been inputted. The else keyword is also redundant, since it is preceded by return statements.

def prompt_integer():
    number = raw_input('Enter an integer: ')
    while True:
        if number.isdigit():
            return int(number)
        elif number[0] == '-' and number[1:].isdigit():
            return int(number)

        # Failed validation
        print '%r is not an integer' % number
        print 'To try again, press Enter key'
        print 'Otherwise, press Ctrl+C keys to abort'
        number = raw_input('>>> ')
\$\endgroup\$
2
  • \$\begingroup\$ I'll rename it right away and make a note of the redundancy. \$\endgroup\$
    – user61142
    Jan 14, 2015 at 22:17
  • 1
    \$\begingroup\$ This is not entirely ok, the second raw input is not validated. \$\endgroup\$
    – orion
    Jan 15, 2015 at 11:24
3
\$\begingroup\$

Integer validation by manually checking the string is not recommended (actually, one should never parse input manually, always use builtins). Let the python decide if it is an integer or not. Following python's philosophy to "rather ask for forgivenes than permission", I'd write it this way:

#let us choose our own prompt string, but use default if not provided
def prompt_integer(prompt='Enter an integer: '):
    try:
        return int(raw_input(prompt))
    except ValueError:
        #you could also use a multiline string here, but let's not use too much python sugar
        print('Invalid integer')
        print('To try again, press Enter key')
        print('Otherwise, press Ctrl+C keys to abort')
        #recursively call prompt with a different prompt string
        return prompt_integer('>>> ')

I didn't want to remember the input string in a variable simply to write it back in case of an error. But that's your choice. You could also just print the exception string.

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

Focusing soley on square, I agree with janos about extracting the constant abs

def square(integer):
    abs_integer = abs(integer)

    result = 0
    for _ in range(abs_integer):
        result += abs_integer

    return 'Square of {} is {}'.format(integer, result)

This is nicer as a comprehension and sum:

def square(integer):
    abs_integer = abs(integer)

    result = sum(abs_integer for _ in range(abs_integer))

    return 'Square of {} is {}'.format(integer, result)

The sum call adds up everything in the container you pass it and (abs_integer for _ in range(abs_integer)) produces the element abs_integer abs_integer times.

Here's also a trick that comes in occasional - albeit rare - use. Instead of adding one abs_integer every time, you can instead double it with

result += result

If you make a generic times function, this is quite simple to express. First we need the base condition:

def multiply(number, times):
    if times == 0:
        return 0

Then we need to consider times > 0 by moving the negative:

    # Make times nonnegative for loop
    if times < 0:
        number = -number
        times = -times

Then we start the loop:

    added = 1
    total = number

and double the number as much as possible:

    # Double while possible
    while added + added < times:
        added += added
        total += total

There will be more left to add, so put that on top:

    # Add the rest
    total += multiply(number, times-added)

That's it!

    return total

So the square is just

def square(number):
    return multiply(number, number)

Note that even this is suboptimal since we recompute the same stuff on every call to multiply, but fixing it is less simple.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.