5
\$\begingroup\$

For a project I'm working on we need a basic time holder class. This is written in CircuitPython, which is a stripped down version of CPython that's missing a number of libraries, including the whole datetime library.

The class definition is as follows. Note that TimeDelta is very similar, it also holds a second and day count, but is meant to represent a duration rather than a single point in time. Thus a different class for the different semantic meaning.

__hms_to_secs() and __ymd_to_days() do exactly what their names suggest they ought to.

class TimeStamp:
    def __init__(self, year = 2000, month = 1, day = 1, hours = 0, minutes = 0, seconds = 0):
        self.seconds = __hms_to_secs(hours, minutes, seconds)
        self.days = __ymd_to_days(year, month, day)
        self.normalize()

    def normalize(self):
        if self.seconds < 0 or self.seconds >= 86400:
            day_error = self.seconds // 86400
            self.seconds -= day_error * 86400
            self.days += day_error
        return self

    def __add__(self, rhs)
        return (TimeStamp(self.days + rhs.days, 0, 0, self.seconds + rhs.seconds) if isinstance(rhs, TimeDelta) else TimeDelta(self.days, 0, 0, self.seconds + rhs)).normalize()

    def __sub__(self, rhs)
        return (TimeDelta(self.days - rhs.days, 0, 0, self.seconds - rhs.seconds) if isinstance(rhs, TimeDelta) else TimeDelta(self.days, 0, 0, self.seconds - rhs)).normalize()

The one thing I'm curious about is the normalize() method. By both modifying self and returning it as well, it allows both

    time_stamp.normalize()

and

    return temporary_time_stamp.normalize()

to work like you'd expect. But as a relative newcomer to Python I have no idea if this is considered good style. Also observations on __add__() and __sub__() are welcome. Is there a preference for longer "do it all" statements, or for breaking them up into two or more bite sized lines? The intention is to offer two overloads, either adding a TimeDelta or a raw count of seconds.

As a 40 year career C/C++ programmer, overloading based on parameter type is second nature to me. I miss it a lot in Python, which can't do it due to the fact it's duck-typed. So I've taken to doing the overload resolution at run-time.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ You shouldn't edit the code in your question after it has been answered. In this case, I'm going to edit the answer to match, since that's straightforward, but in future please don't do that. \$\endgroup\$ Commented Aug 31, 2022 at 6:36

1 Answer 1

Reset to default
8
\$\begingroup\$

First, I want to point out that leap seconds exist, which means a day is not necessarily guaranteed to be exactly 86400 seconds. If you're aware of this and have determined your use case does not require you to account for it, that is fine, but I'm still pointing it out in case you weren't aware of it

Speaking of 86400, it might be nice to have that broken out into a constant with a descriptive name like SECONDS_PER_DAY

Beyond that, I'd argue that normalize is a method that users will not want to care about at all:

If we do not expect users to manually modify an existing TimeStamp's or TimeDelta's seconds and days values, we are already providing only normalized values -- they get normalized as part of their initialization, and never become un-normalized

If we do expect users to manually modify existing values, I still don't think keeping the value normalized should be their responsibility -- rather, I would suggest that normalization be guaranteed as part of the modification. The built-in property function could help with that, if CircuitPython supports it

Either way, I don't think we should be thinking of normalize as part of our interface, when we can instead make every part of our interface provide only normalized values

On a related note, the normalize calls in __add__ and __sub__ are kind of redundant -- freshly created timestamps and timedeltas are already normalized, and since we don't modify them between creation and return, normalizing them again does nothing

Finally, while in-line ifs have their uses, they can be a bit cumbersome to read, and if/else blocks are usually preferred. I might rewrite __add__ (and __sub__) closer to

def __add__(self, rhs):
    if isinstance(rhs, TimeDelta):
        return TimeStamp(self.days + rhs.days, 0, 0, self.seconds + rhs.seconds)
    else:
        return TimeDelta(self.days, 0, 0, self.seconds + rhs)

Alternatively, I might go with the "ask forgiveness, not permission" approach that's common in the Python community, and end up with something akin to

def __add__(self, rhs):
    try:
        return TimeStamp(self.days + rhs.days, 0, 0, self.seconds + rhs.seconds)
    except AttributeError:
        return TimeDelta(self.days, 0, 0, self.seconds + rhs)

Edit

My intuition is that some_timestamp + 3 should be the same kind of operation as some_timestamp + TimeDelta(seconds=3), but the latter returns a TimeStamp while the former returns a TimeDelta. I suspect that might not be intentional - is it? If it is, I feel like it's not obvious what that operation, and the resulting TimeDelta, represents, and it may be good to clarify that

\$\endgroup\$
9
  • \$\begingroup\$ normalize() should probably be renamed to __normalize() because the external API need not allow direct modification of the members. To change a TimeStamp use a TimeDelta. As an extension, renaming the members to __days and __seconds has merit. As for the calls to normalize() in __add__() and __sub__() the use case is subtracting a TimeDelta holding 23 hours from a TimeStamp representing 2020/6/8T12:23:34. In the absence of the normalize() call, that would leave the TimeStamp with a seconds value that's out of range. \$\endgroup\$ Commented Aug 30, 2022 at 23:53
  • 1
    \$\begingroup\$ And yes, I'm aware of leap seconds, the conscious decision to "ignore" them has been made. The necessity here is something that provides accuracy to within a second or two, not "to the millisecond" accuracy. In the long term, this will ping an NTP server every few hours, which should silently handle the problem. As long as it's only ever a single second change, we will gracefully handle it, either a second simply goes missing in action or more commonly gets duplicated. Neither of those cases will break the system. \$\endgroup\$ Commented Aug 31, 2022 at 0:04
  • 2
    \$\begingroup\$ I'm aware of the "ask forgiveness" principle in Python, I don't agree with it. Just about every other language takes the view that exceptions are not to be used for flow control, since doing so can introduce all sorts of subtle bugs. In this case, use of the exception pattern allows the addition of any class that has days and seconds members. Any class whatsoever, including as it happens TimeStamp itself. The only legal RHS for addition to a TimeStamp is a TimeDelta. The if pattern enforces this, the exception pattern doesn't. \$\endgroup\$ Commented Aug 31, 2022 at 1:48
  • 1
    \$\begingroup\$ Most languages that discourage use of exceptions in the happy(ish) path do so for performance reasons that don't apply to Python, rather than to ensure functional correctness. You can disagree all you like, but it always pays to use the language's established idioms if you want to work with others. If you want to constrain argument types, use annotations (but be aware that duck-typing is the established Python idiom, so you're going against the grain there, too). \$\endgroup\$ Commented Aug 31, 2022 at 6:44
  • 1
    \$\begingroup\$ @TobySpeight In most of the discussions I've seen, performance has very little to do with it. The argument usually revolves around the fact that an exception is a non-local goto. Which in turn makes the code fragile. Consider the following scenario,for __add__() Rather than the simple work in the try clause, assume it does something far more complex, that just happens to throw the very exception we're catching. You've now got a nightmare on your hands: it'll try the catch clause, and that will of course fail because the RHS was actually a valid TimeStamp. ... \$\endgroup\$ Commented Aug 31, 2022 at 8:13

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.