10
\$\begingroup\$

I build this simple function, trying to use PEP 8 Style, something that I discovered recently thanks to this previous question.

The function works, but I was wondering if there is a better way to do it.

import datetime

def quarter_values_builder():
    """A function that returns an array
    with all the actual year quarters
    """
    now = datetime.datetime.now()
    year = now.year
    year_str = str(year)
    quarter_values = ["31/03/","30/06/","30/09/","31/12/"] 
    quarter_values_now = [x+year_str for x in quarter_values]

    return quarter_values_now

current_quarters_string = quarter_values_builder()
print(current_quarters_string)  
\$\endgroup\$
0

2 Answers 2

8
\$\begingroup\$

Overall, it looks fine. I probably would've written it a little differently, and I'll explain the differences.

import datetime

def get_current_quarters():
  """A function that returns an array
  with all the actual year quarters
  """
  current_year = datetime.date.today().year
  quarter_values = ["31/03/","30/06/","30/09/","31/12/"] 
  current_quarter_values = [
    "{0}{1}".format(x, current_year)
    for x in quarter_values]

  return current_quarter_values


current_quarters = get_current_quarters()
print(current_quarters)

The function name quarter_values_builder() sounds too complicated to me. Is it really a builder if all it does is take a known list and append the year to it? It's basically a glorified string formatter. A builder reminds me of the Builder pattern, something that doesn't apply here.

Getting the current year can be simplified too. We're not interested in the intermediary variables, so no need to create them. It's still perfectly readable if we do it all in one go.

x+year_str should at least have a little whitespace, x + year_str, to increase readability. While we're at it, we might as well use proper string formatting. This is how we did save us a str() cast earlier too. The formatting takes care of this for us. Because the line got a bit long, I cut it up a bit per function. This isn't required under 79 characters, but I think it looks better.

At the end, we don't need to call the storage variable a string. We don't need to store it at all, the last 2 lines could easily become 1 line. But if we store it, putting the type of the variable in the name is more of a distraction than a help. After all, it's Python. The type might easily change in the next version. If you really want to use type-hints in Python, use actual type hints.

Having a now in a variable while you're actually talking about quarters is somewhat confusing as well, in my opinion. So I've completely eliminated that from the used variables.

Addendum:
We don't need to use datetime.datetime.now() at all. The datetime module has a date, time and datetime type. Considering we're only interested in the year attribute, a datetime is overkill. date is enough to extract the year from.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Mostly sensible, though I would replace datetime.datetime.now with datetime.date.today which captures your intent better \$\endgroup\$
    – Reinderien
    Apr 8, 2021 at 17:32
  • \$\begingroup\$ @Reinderien Great catch, you're absolutely right. \$\endgroup\$
    – Mast
    Apr 8, 2021 at 17:38
9
\$\begingroup\$

Without seeing the rest of your code it's difficult to say how this is used, but I think your function has conflated logic and formatting, and formatting is being done too soon. You should hold onto real date information right up until the edges of your program when that's possible. The following demonstrates one way of keeping dates and moving them to the current year.

from datetime import date, timedelta
from typing import Iterable, Tuple

QUARTER_DATES = (
    date(2000, 3, 31),
    date(2000, 6, 30),
    date(2000, 9, 30),
    date(2000, 12, 31),
)


def get_current_quarters() -> Iterable[date]:
    year = date.today().year
    for quarter in QUARTER_DATES:
        yield quarter.replace(year=year)


print('\n'.join(d.strftime('%d/%m/%Y') for d in get_current_quarters()))

dd-mm-YYYY is also an ambiguous and non-sortable format. Your usage (which you have not shown) will either need this to be "machine-readable" - e.g. for logs, REST payloads, etc. - or human-readable, e.g. for a UI. If you need machine readability, the ISO yyyy-mm-dd format is strongly preferred. If you need human readability, rather than hard-coding dd/mm/YYYY, your code should obey the current locale (not shown above).

\$\endgroup\$
3
  • \$\begingroup\$ Would you mind removing the following part of your answer? "[Date format in other peoples cultures] should be killed with prejudice and replaced with [the date format in my culture]". \$\endgroup\$
    – Peilonrayz
    Apr 8, 2021 at 16:16
  • \$\begingroup\$ @Peilonrayz ISO is not a culture. The more important distinction is one of machine readability vs. human readability, which I've expanded on. \$\endgroup\$
    – Reinderien
    Apr 8, 2021 at 16:20
  • 2
    \$\begingroup\$ Yes ISO and dd/mm/yyy aren't cultures. However cultures use ISO and dd/mm/yyyy. And °C/°F, imperial/metric, etc. Yes. The focus should be readability vs human readability, thank you for changing the focus to that. \$\endgroup\$
    – Peilonrayz
    Apr 8, 2021 at 16:41

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.