2
\$\begingroup\$

Any thoughts?

print('*'*44+'\n****How Many Seconds Have You Been Alive****\n'+'*'*44)

while True:
    try:
        print('\n--------------------------------------------')
        Age = int(input('\nHow old are you?: '))

        if Age<1 or Age>90:
            print('(Input Error)*Enter Valid Age*')
            continue

    except(TypeError,ValueError):
        print('(Input Error)*Enter Valid Age*')
        continue

    seconds = str(Age*31536000)

    def splitthousands(x, sep=','):  #Inserts a comma after every three digits from the right.
        if len(x) <= 3: return x  
        return splitthousands(x[:-3], sep) + sep + x[-3:]

    print('\nYou have been alive for '+splitthousands(seconds, sep=',')+' seconds.')
    print('\n--------------------------------------------')

    while True:
        try:
            reply = str(input('\nDo you want to play again(Y/N)?: ').title())

        except(TypeError,ValueError):
            print('(Input Error)*Enter Valid Option*')
            continue

        if reply.startswith('Y'):
            break

        else:
            raise SystemExit
\$\endgroup\$
  • \$\begingroup\$ Watch your indentation: your code fails to compile with the line if reply.startswith('Y'):. Since this is very minor, I won't close the question right away. Just be sure to check the code next time. \$\endgroup\$ – Jamal Dec 26 '16 at 23:56
  • \$\begingroup\$ Why do you exclude people younger than 1 year or older than 90 years from using your program? \$\endgroup\$ – mkrieger1 Dec 27 '16 at 0:27
4
\$\begingroup\$

Crashing bug

If I copy and paste your code straight into my editor and run it, I get an indentation error. Looks like something got screwed up in the post formatting:

$ python3 myage.py
  File "myage.py", line 33
    if reply.startswith('Y'):
                            ^
IndentationError: unindent does not match any outer indentation level

Aside from that, I was unable to find a malicious input that would trigger an unhandled exception in your code. 👍

No accounting for leap years

You compute the number of seconds somebody’s been alive as follows:

seconds = str(Age*31536000)

There’s no comment to explain where 31536000 came from. The program would be improved if you explained where this magic number came from, so that it can be verified independently (for example, when I first typed my previous sentence, I made a typo and missed a 0). Or even better, compute it on the fly. For example:

seconds = age * (60 * 60 * 24 * 365)

Now it’s much easier to see (1) where the quantity comes from, and (2) that it’s correct.

ish.

Your program doesn’t account for leap years correctly. For example, somebody who’s 20 years old has lived through the leap years 2016, 2012, 2008, 2004, (not 2000!) and maybe some of 1996. That’s up to 5 * 24 * 3600 = 432,000 seconds you’re missing.

In this case, that’s fairly small and perhaps insignificant – but it’s worth remembering leap years whenever you’re dealing with dates and times.

Read PEP 8

PEP 8 is the Python style guide, and you should read it and apply it to your code. Lots of weird stuff around spacing, line length, variable names, and so on, that make it harder to read your code. Standards make it easy for other experienced Python programmers to read your code.

If you need help, there are automated tools like flake8 which can analyse a Python file and give you suggestions for making it PEP 8 compliant.

Break up your code into functions

Nothing in this program is reusable, it’s just one long top-level execution. Breaking it up into smaller pieces will make those pieces easier to test, and easier to reuse. Some candidates for code that could be broken off into their own chunk:

  • Get input from the user
  • Given a validated input, how many seconds has somebody been alive
  • Print the input

You get the idea. This tends to pay off in code quality and readability as well – small, self-contained functions are easier to work on than a monolith.

Use the standard library for pretty number printing

Your splitthousands() function is fine in places where commas are used to format numbers, but in other places people may used periods, or spaces, or not group digits at all. A more flexible solution is to use the locale module from the standard library, and let it absorb that messiness for you:

import locale
print(locale.format('%d', seconds_alive, grouping=True))

Again, in this case that might not matter – but it’s something worth thinking about for bigger projects in the future.

\$\endgroup\$
  • \$\begingroup\$ You could remove the crashing bug part of your answer since another mod fixed it in the question (we don't normally do that, but it was just one space). \$\endgroup\$ – Jamal Dec 27 '16 at 17:25
4
\$\begingroup\$

I am fully agree with what @Jamal has said. Apart from that I would like to add some more points.

Use consistency in your code

Although PEP8 says there is no strict rule for naming convention, if you stick to one particular convention it'll be easier for you and the reviewers to follow.

In your code you have used reply, seconds which starts with lowercase letter but Age starts with uppercase letter.

I use the following convention

  • normal variables: lowercase_with_underscore
  • class names: CamelCase.
  • constants: UPPERCASE

And about the function which you defined inside the while loop

Only in certain cases(like in decorators) you should define your function inside an enclosing scope. Defining a function inside while loop means, whenever the while condition become true it will encounter that function block which is time consuming.

Better define your function outside the while block and it improves readability.

Hard coding

You have hard coded the numbers like 1, 90, 31536000. Giving names to these number will help you a lot in the future.

Creating function

Creating a function for the calculation of age-in-seconds will allow you for the re-usability of your work.

After changing all the stuff, my function is looking like the following

SECONDS_IN_A_YEAR = 31536000
LOWEST_AGE = 1
HIGHEST_AGE = 90

def splitthousands(x, sep=','):
    '''
    Inserts a comma after every three digits from the right.
    '''
    if len(x) <= 3:
        return x

    return splitthousands(x[:-3], sep) + sep + x[-3:]

def calc_age_in_seconds(age):
    '''
    calculates the given age in seconds

    :raises: ValueError
    '''

    if age < LOWEST_AGE or age > HIGHEST_AGE:
        raise ValueError

    return age * SECONDS_IN_A_YEAR

def ask_user_and_calc_age_in_seconds():

    print('*'*44+'\n****How Many Seconds Have You Been Alive****\n'+'*'*44)

    while True:
        try:
            print('\n--------------------------------------------')
            age = int(input('\nHow old are you?: '))

            seconds = str(calc_age_in_seconds(age))

        except(TypeError,ValueError):
            print('(Input Error)*Enter Valid Age*')
            continue

        print('\nYou have been alive for '+splitthousands(seconds, sep=',')+' seconds.')
        print('\n--------------------------------------------')

        while True:
            try:
                reply = str(input('\nDo you want to play again(Y/N)?: ').title())

            except(TypeError,ValueError):
                print('(Input Error)*Enter Valid Option*')
                continue

            if reply.startswith('Y'):
                break

            else:
                raise SystemExit

if __name__ == '__main__':
    ask_user_and_calc_age_in_seconds()
\$\endgroup\$
2
\$\begingroup\$

This is a little hard to read at a glance:

def splitthousands(x, sep=','):  #Inserts a comma after every three digits from the right.
    if len(x) <= 3: return x
    return splitthousands(x[:-3], sep) + sep + x[-3:]

Instead, indent the first return and separate the second one. I'd also recommend putting the comment on top to make it clear that it's covering the entire function. Doing so could help reduce horizontal length, too.

#Inserts a comma after every three digits from the right.
def splitthousands(x, sep=','):
    if len(x) <= 3:
        return x

    return splitthousands(x[:-3], sep) + sep + x[-3:]
\$\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.