3
\$\begingroup\$

I implemented a convenience class to time code execution and print it to the user. This is part of a bigger project that will be distributed to external users. The timer however is only called by other procedures, not by the user directly. I was interested in implementing this myself, so I'm not looking for an external module or similiar. Docstrings are omitted here, the functions are rather straightforward.

from time import perf_counter


class Timer:
    def __init__(self, default_round_digits: int = 5):
        self.default_digits = default_round_digits
        self.start_timer()

    def start_timer(self) -> None:
        self.start_time = perf_counter()
        self.pause_start_time = None
        self.pause_length = 0

    def pause_timer(self) -> None:
        self.pause_start_time = perf_counter()

    def resume_timer(self) -> None:
        if self.pause_start_time is not None:
            self.pause_length += perf_counter() - self.pause_start_time
            self.pause_start_time = None

    @property
    def elapsed_seconds(self) -> float:
        # If timer is paused only consider time up to self.pause_start_time instead of now
        if self.pause_start_time is not None:
            return self.pause_start_time - self.start_time - self.pause_length
        else:
            return perf_counter() - self.start_time - self.pause_length

    def get_timer(self, round_digits: int = None, print_text: str = None, restart: bool = False) -> float:

        if round_digits is None:
            round_digits = self.default_digits

        elapsed_seconds = round(self.elapsed_seconds, ndigits=round_digits)

        if print_text is not None:
            print(f"{print_text}{elapsed_seconds} seconds")

        if restart:
            self.start_timer()

        return elapsed_seconds

    def __str__(self) -> str:
        state = "Running" if self.pause_start_time is None else "Paused"
        return f"{state} Timer at {self.get_timer()} seconds"

I'm still unsure about initializing attributes outside of __init__. I know it's not recommended, but copying start_timer into __init__ seemed like the worse option to me.

\$\endgroup\$
4
\$\begingroup\$
  1. Your Timer class contains two timers.

    1. The entire duration timer.
    2. The timer for tracking pauses.

    I'd move the core timer code out into a separate class. And we can see you're starting to repeat yourself.

  2. Initializing a timer shouldn't start the timer. If I open my timer app should the timer start running straight away?

    If I want to create some timers but I don't want the timer to be started your class gives me no options. However just not calling start_timer in __init__ will allow everyone to use your class however is desired.

    If you really really want to start the timer when building the object, you can write a class method.

    class Timer:
        @classmethod
        def run(cls, *args, **kwargs):
            self = cls(*args, **kwargs)
            self.start_timer()
            return self
    
    # option 1
    timer = Timer()
    timer.start_timer()
    
    # option 2
    timer = Timer.run()
    
  3. start_timer restarts the timers. You should rename the method or make the method do what is expected.

  4. If you call pause_timer twice without calling resume_timer your code acts as if the timer was never paused the first time.

  5. I don't really see the point of making elapsed_seconds a property.

  6. get_timer is doing way too much and violates the SRP principle.

    1. Rounds elapsed timings to a specified amount of decimal places.
    2. Prints the elapsed timings.
    3. Restarts the timer.
    4. Returns the elapsed timings.

    You should make a function to round the elapsed timings. Then leave printing and restarting to the user.

  7. I'd a couple more methods from functionality I'd expect from a timer.

from time import perf_counter


class CoreTimer:
    def __init__(self):
        self.stop()

    def __bool__(self):
        return bool(self.start_time)

    def start(self):
        self.start_time = perf_counter()

    def stop(self):
        self.start_time = 0

    def time_taken(self, now):
        return now - self.start_time

    def elapsed(self):
        return self.time_taken(perf_counter())


class Timer:
    def __init__(self, decimal_places=5):
        self._decimal_places = decimal_places
        self._timer = CoreTimer()
        self._pause = CoreTimer()
        self._pause_length = 0

    def __str__(self):
        state = "Paused" if self._pause else "Running"
        return f"{state} Timer at {self.elapsed_rounded()} seconds"

    @classmethod
    def run(cls, *args, **kwargs):
        self = cls(*args, **kwargs)
        self.start()
        return self

    def start(self):
        if not self._timer:
            self._timer.start()

    def stop(self):
        self._timer.stop()
        self._pause.stop()
        self._pause_length = 0

    def restart(self):
        self.stop()
        self.start()

    def pause(self):
        if not self._pause:
            self._pause.start()

    def resume(self):
        if self._pause:
            self._pause_length += self._pause.elapsed()
            self._pause.stop()

    def elapsed(self):
        if self._pause:
            duration = self._timer.time_taken(self._pause.start_time)
        else:
            duration = self._timer.elapsed()
        return duration - self._pause_length

    def elapsed_rounded(self, decimal_places=None):
        if decimal_places is None:
            decimal_places = self.decimal_places
        return round(self.elapsed(), ndigits=decimal_places)
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Thank you for your perspective, this is really helpful (a lot of improvements I hadn't considered yet)! I should've specified, that the timer will not be used directly by the user, only the print-outs by get_timer are seen by the user. That's why I included as much convenience for myself as possible. Considering that, would you still advise against the existence of convenience methods for printing and restarting? Something like print_elapsed_and_restart? \$\endgroup\$ Apr 2 at 14:46
  • 1
    \$\begingroup\$ @riskypenguin By "the user" I mean "anyone who uses Timer" not "anyone who uses your library". I can understand the desire for convenience. But many times what you think is convenient at the start of the project is an inconvenience by the end of the project. \$\endgroup\$
    – Peilonrayz
    Apr 2 at 15:00
  • 1
    \$\begingroup\$ I see your point, thanks again for the comprehensive review! \$\endgroup\$ Apr 2 at 15:24
3
\$\begingroup\$

Regarding your question about attribute initialization:

It's not necessarily better to put all attributes into __init__. It's generally recommended, since we expect to find all class attributes inside of __init__, but I would argue in this case, it's just as readable. In fact I would say initializing these values with None inside __init__ (just to suppress the warnings or follow the recommendation) hurts readability in your case:

def __init__(self, default_round_digits: int = 5):
    self.default_digits = default_round_digits

    self.start_time = None
    self.pause_start_time = None
    self.pause_length = None

    self.start_timer()

Here are some further discussions on StackOverflow regarding the topic:

\$\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.