2
\$\begingroup\$

The code below takes an input and based on the input gives out a time, date or both. Please will you review me for this and tell me how I can improve this? By the way, I only started learning python.

import datetime

x = datetime.datetime.now()
s = '/'
c = ':'
print("Welcome to Date and Time!")
input = int(input("Please choose one of these options below and type it in the prompt: \n 1 - To know the date (DD : MM : YY) \n 2 - To know the time (Hours : Minutes : Seconds ) \n 3 - To know both date and time.\n"))

if input == 1 :
    print(str(x.day) + s + str(x.month) + s + str(x.year) )
elif input == 2 :
    print(x.strftime('%H') + c + x.strftime('%M') + c + x.strftime('%S'))
elif input == 3 :
    print(str(x.day) + s + str(x.month) + s + str(x.year) + ' ' + x.strftime('%H') + c + x.strftime('%M') + c + x.strftime('%S'))
else: 
    print("You didn't type in 1, 2 or 3!")
\$\endgroup\$
2
\$\begingroup\$

My first time here providing code review on stackexchange. I randomly clicked a link and arrived here. Anyway, here's how I'd improve your code:

#!/usr/bin/env python

import datetime

DATE_FORMAT = "%d/%m/%y"
DATE_FORMAT_FOR_HUMANS = "DD/MM/YY"
TIME_FORMAT = "%H:%M:%S"
TIME_FORMAT_FOR_HUMANS = "Hour:Minute:Second"


def prompt_for_user_input():
    return int(
        input(
            """Please choose one of these options below and type it in the prompt:
1 - To know the date ({DATE_FORMAT_FOR_HUMANS})
2 - To know the time ({TIME_FORMAT_FOR_HUMANS})
3 - To know both date and time. ({DATE_FORMAT_FOR_HUMANS} {TIME_FORMAT_FOR_HUMANS})
"""
        )
    )


def print_datetime_from_input(user_input, datetime_value):
    formatted_date = get_formatted_date(datetime_value)
    formatted_time = get_formatted_time(datetime_value)

    if user_input == 1:
        print(formatted_date)
    elif user_input == 2:
        print(formatted_time)
    elif user_input == 3:
        print(formatted_date, formatted_time)
    else:
        print("You didn't type in 1, 2 or 3!")


def get_formatted_time(datetime_value):
    return datetime_value.strftime(TIME_FORMAT)


def get_formatted_date(datetime_value):
    return datetime_value.strftime(DATE_FORMAT)


def main():
    now = datetime.datetime.now()

    print("Welcome to Date and Time!")

    user_input = prompt_for_user_input()
    print_datetime_from_input(user_input, now)


if __name__ == "__main__":
    main()

And here's why:

  • It's good practice to separate logic into smaller functions as it makes it easier to run tests against smaller parts of a system. For the above, I could have wrote three separate tests, one that ensures that for a given user input, the right date gets displayed. Another test for the the formatting of the date and time, another one for the main execution.

If you're curious about tests, I recommend you to search online why do we write unit tests and how to write tests in python. This helped me a lot in writing better code.

  • Separating into smaller function helps for reusability. In the given example, I tried to separate the logic from the user input and the display.
  • I used an IDE to auto indent and format. The above follows PEP8.
  • I extracted the formats into DATE_FORMAT and TIME_FORMAT. Again, we are not reusing anything, but imagine you'd like to reuse the formats somewhere else. It also makes it easier to change the formats as you only need to change one line to see a different result.
  • I've split the input into multi line string which makes it easier for readability. I also moved the displayed format for the user beside the format used by the code to display it, this way, if you change one of the formats, you can update the prompt for the user more easily.
  • I've added a main function and a if __name__ == "__main__": condition to call main. In this case, it will execute the main function if the script is invoked directly. Imagine your script is called ask_for_time.py, you could now import ask_for_time.DATE_FORMAT and the main execution won't be executed on import time.
  • The first line #!/usr/bin/env python is called a shebang, it's used to tell the shell what to use to execute the script in case it is invoked directly (example: ./ask_for_time.py instead of python ask_for_time.py.

You should always try to use the DRY principle.

For the datetime formatting, you can refer to the following documentation: http://strftime.org/

As an exercise, I suggest you to try and write unit tests to make sure the code you write does what you intend it to do. Good luck and have fun learning python ✌🏻

\$\endgroup\$
  • \$\begingroup\$ These are good recommendations in general. The DRY principle is a good practice to follow however it can be easy to get too crazy and do complicated stuff in the name of DRY. (complicated = hard to read/maintain). The other recommendation I would suggest is a code linter for python. PEP8 and PEP256 are good coding conventions to follow as best as possible, although the 80character limit is slowly falling out of favor but keeping your line length short helps with readability without scrolling - especially when you have to share code on a big screen/projector. \$\endgroup\$ – cgseller Oct 27 '18 at 21:09
0
\$\begingroup\$

Since you are allowing console input, you may want to either validate the input format or catch exceptions or both. Also, I made some adjustments to handle prompt better and to be more PEP8 compliant for 80 chars. These modifications are nit picks aside from the docstring which is helpful when having documentation autogenerated (useful for larger projects).

#!/usr/bin/env python
"""
My first python script.

This is a script to demonstrate how to ask for input, follow PEP python style,
and to learn the good methods of python.  I also learned how to leverage
Stack Exchange to get code review feedback.
"""

import datetime

# global constants usually are in all CAPS
DATE_FORMAT = "%d/%m/%y"
DATE_FORMAT_FOR_HUMANS = "DD/MM/YY"
TIME_FORMAT = "%H:%M:%S"
TIME_FORMAT_FOR_HUMANS = "Hour:Minute:Second"


def prompt_for_user_input():
    """Choose the date or time format to display."""
    prompt = (
        """
Please choose one of the options below and enter at the prompt:

1 - Show the date ({0})
2 - Snow the time ({1})
3 - Snow both date & time ({0} {1})

Selection): """.format(DATE_FORMAT_FOR_HUMANS, TIME_FORMAT_FOR_HUMANS)
    )
    try:
        selection = int(input(prompt))
    except ValueError as nonIntErr:
        print("\nYou selected a non-integer, please follow\n {}\n ERR={}"
              .format(prompt, nonIntErr))
        exit(1)
    return selection


def print_datetime_from_input(user_input, datetime_value):
    """Output the datestring that was requested."""
    formatted_date = get_formatted_date(datetime_value)
    formatted_time = get_formatted_time(datetime_value)

    if user_input == 1:
        print(formatted_date)
    elif user_input == 2:
        print(formatted_time)
    elif user_input == 3:
        print(formatted_date, formatted_time)
    else:
        print("You didn't type in 1, 2 or 3!")


def get_formatted_time(datetime_value):
    """Return string format of time based on datetime object."""
    return datetime_value.strftime(TIME_FORMAT)


def get_formatted_date(datetime_value):
    """Return string format of date based on datetime object."""
    return datetime_value.strftime(DATE_FORMAT)


def main():
    """Run main for invocation when run via the terminal."""
    now = datetime.datetime.now()

    print("Welcome to Date and Time!")

    user_input = prompt_for_user_input()
    print_datetime_from_input(user_input, now)


if __name__ == "__main__":
    main()
\$\endgroup\$
0
\$\begingroup\$

Definitely agree about validating input. You can do this yourself, or use common libraries to do so. The Click library is a great resource for creating command line interfaces (a substitute for the built-in argparse). Take some time getting to know these as they save you a lot of typing down the line!!

Below is a simple example for Python 3.

import datetime
import click


DATE_FORMAT = "%d/%m/%y"
DATE_FORMAT_FOR_HUMANS = "DD/MM/YY"
TIME_FORMAT = "%H:%M:%S"
TIME_FORMAT_FOR_HUMANS = "Hour:Minute:Second"

now = datetime.datetime.now()
date = now.strftime(DATE_FORMAT)
time = now.strftime(TIME_FORMAT)

results = [
    date, 
    time, 
    ' '.join([date, time])
]

result_dict = {idx: x for idx, x in enumerate(results, 1)}

prompt = f"""\nPlease choose one of these options below and type it in the prompt:
1 - To know the date ({DATE_FORMAT_FOR_HUMANS})
2 - To know the time ({TIME_FORMAT_FOR_HUMANS})
3 - To know both date and time. ({DATE_FORMAT_FOR_HUMANS} {TIME_FORMAT_FOR_HUMANS})
"""
res = click.prompt(prompt, type=click.IntRange(1, len(result_dict)))
print(result_dict.get(res))
\$\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.