5
\$\begingroup\$

Report time at whole hours

#!/usr/bin/python3
import time
from datetime import datetime
from pydub import AudioSegment
from pydub.playback import play

bell_sound = AudioSegment.from_wav("/home/me/Music/audio/bell.wav")

while True:
    now = datetime.now()
    # repeats = 1 if now.hour % 3 == 0 else now.hour % 3
    repeats = now.hour % 3 + 1

    if now.hour < 6:
        break
    # report on whole hour
    if now.minute == 0:
        for _ in range(repeats):  # bell the hours
            play(bell_sound)
        time.sleep(60 * 58)  # save the computation
    if now.minute != 0:
        time.sleep(10)
    # quit 23 later
    if now.hour == 23:
        break

Any ideas to improve it?

\$\endgroup\$
2
\$\begingroup\$
while True:
    now = datetime.now()
    # repeats = 1 if now.hour % 3 == 0 else now.hour % 3
    repeats = now.hour % 3 + 1

    if now.hour < 6:
        break

The repeats is not used in the condition of the break. Therefore you should reorder your code like this, to avoid computing things you don't need later:

    now = datetime.now()
    if now.hour < 6:
        break

    repeats = now.hour % 3 + 1

I was confused by the last line of code. I thought it would mean to ring the bell 3 times at 2 o'clock. What confused me was that now.hour does not correspond to my intuitive understanding of now anymore, since by the time the bell will ring, some time has passed.

To avoid this confusion, you should define a variable next_hour = now.hour + 1. When you then define repeats = (next_hour - 1) % 3 + 1, that may look even more confusing because of the - 1 at the beginning and the + 1 at the end. That's probably why you considered repeats = 3 if next_hour % 3 == 0 else next_hour % 3, and it's good that you did that.

Continuing with your code:

    # report on whole hour
    if now.minute == 0:
        for _ in range(repeats):  # bell the hours
            play(bell_sound)
        time.sleep(60 * 58)  # save the computation

I hope that each bell takes only a few seconds. As soon as they become longer, you might miss the next hour completely. The "save the computation" part feels dangerous to me. This is because on practically all operating systems you can put a process to sleep and later resume it. I don't know how exactly this affects the clock and the measurement of datetime.now(), but that's definitely something you should experiment with.

It's better to define a high-level overview first:

def ring_until_midnight():
    current_hour = datetime.now().hour
    if current_hour < 6:
        break

    while current_hour < 23:
        wait_until_hour(current_hour + 1)
        ring_the_bell(current_hour + 1)
        current_hour += 1

This raises the question what to do if the hour steps forward fast, such as when the daylight saving time changes. Since you probably don't want the clock to ring 5 times in a row, it's better to replace the last line above with:

        current_hour = datetime.now().hour  # instead of current_hour + 1

Next, it's time to fill the missing details, which are wait_until_hour and ring_the_bell. For wait_until I propose this simple solution:

def wait_until_hour(hour):
    while datetime.now().hour < hour:
        time.sleep(1)

Note that I intentionally wrote < hour instead of != hour. Just in case the process is stopped for 3 hours. If I had written != hour, the code would continue to wait until the next day. Again, it depends on your exact requirements whether < or != is appropriate in this situation. In any case, you have to make this choice consciously.

Having a possible clock offset of 1 second (from the above sleep(1)) sounds acceptable to me. If you need more precision, you can say time.sleep(0.1) instead as well.

The second detail, ring_the_bell, is similarly simple:

def ring_the_bell(hour):
    repeats = (hour - 1) % 3 + 1

    for _ in range(repeats):
        play(bell_sound)

Now that I think of it, in the definition of ring_until_midnight above, I made an unfortunate choice:

        ring_the_bell(current_hour + 1)

This should rather be:

        ring_the_bell(datetime.now().hour)

The point is that during all this waiting, there is much that can happen, and there is no guarantee that waiting for "the next hour" will finally arrive at exactly that hour. Therefore better ask for the then-current time again.

When you build your programs out of composable pieces like these, you will find that you can reuse some parts by just copying them to your next project, which is good because it saves you the work of writing the same code over and over again, plus you will build up your library of small, hopefully tested code that you know you can trust.

As you saw in the process I outlined above, even writing a simple ring-the-bell program can be made quite complicated. It needs lots of decisions that you might not think about when you first decide to write such a program. The complexity arises mainly because this program is dealing with time and measurement and calendars, which always gets complicated. That's the way it is.

You also saw that it is of utmost importance to get the exact wording right in every line of the code. Is saying "the current hour" really the best way to express your requirement? What does "current" mean, when was it measured, for how long is that measurement valid, what happens in between? Lots of questions, and you sometimes have to revise your initial program. I'm quite experienced, and I didn't get it right either on the first attempt.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

I would have the now.minute != 0 as an else since the if above is the exact opposite case.

I would also assign a 'sleep_time' value inside both branches and call the sleep function after the if - else to emphasize that sleep is always done.

I also don't like while True since it means I have to search the code for the exit condition. I would have the now = datetime.now() line twice, one before the while and once at the end of the block and make the condition while now.hour != 23:. This is personal, some people are more against the duplicated line than the while True:.

| improve this answer | |
\$\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.