Skip to main content
added 151 characters in body
Source Link
Peter Cordes
  • 3.4k
  • 15
  • 26

This is more of a design / algorithm / architecture review than of the code. There are some major issues the other answers didn't address.


First of all, wasting network bandwidth running a speed-test on repeat seems like a bad idea. It will fill up your connection every hour (by default), so it hurts you personally if you happen to be doing something on the Internet at the time. Besides that, it adds to congestion for other users of your ISP.

First of all, wasting network bandwidth running a speed-test on repeat seems like a bad idea. It will fill up your connection every hour (by default), so it hurts you personally if you happen to be doing something on the Internet at the time. Besides that, it adds to congestion for other users of your ISP.

This is more of a design / algorithm / architecture review than of the code. There are some major issues the other answers didn't address.


First of all, wasting network bandwidth running a speed-test on repeat seems like a bad idea. It will fill up your connection every hour (by default), so it hurts you personally if you happen to be doing something on the Internet at the time. Besides that, it adds to congestion for other users of your ISP.

Source Link
Peter Cordes
  • 3.4k
  • 15
  • 26

First of all, wasting network bandwidth running a speed-test on repeat seems like a bad idea. It will fill up your connection every hour (by default), so it hurts you personally if you happen to be doing something on the Internet at the time. Besides that, it adds to congestion for other users of your ISP.


In your config file on the github repo, your default tweet format strings are WAY too aggressive and hostile for something that might not be your ISP's fault:

"tweetContent": [
    "{0}! I'm meant to get 52mb/s down, 10mb/s up. I got {1}mb/s down, {2}mb/s up!",
    "Hey {0}, think {1}mb/s down, {2}mb/s up instead of 52mb/s down, 10mb/s up is ok - it's not!",
    "Don't break your promise {0}. {1}mb/s down, {2}mb/s up != 52mb/s down, 10mb/s up",
    "{0}, how do I Netflix as expected with {1}mb/s down, {2}mb/s instead of 52mb/s down, 10mb/s up?"
]

Speed-test results can depend on congestion elsewhere in the network, or on the speed-test servers. Even congestion from your own downloads (or Netflix-watching) that's happening while the speed-test is running will reduce the measured value.

A reasonable message might be: "Automated speed-test got {1}M down, {2}M up, much lower than {0}'s expected 52M down 10M up speed."

It's just a plain statement of fact, and leaves open the interpretation that something went wrong with the test. But people reading it will still get the message. In some ways, a neutral presentation of facts is more likely to be taken seriously than accusations of promise-breaking, or whining about Netflix.

Your speed-test is only measuring it's own bandwidth, not the total bandwidth of your connection to your ISP. Doing that would be much better, but would require something on your router to monitor traffic, and a way for you to query it from this program. (You could still use a speed-test to generate traffic, and check that it pushed the total up to the expected throughput.)


Your while True: loop never sleeps while waiting for either flag to be set. Busy-waiting with no sleep is a huge waste of CPU time (and electrical power).

I don't really know Python, so I don't have any recommendations for what to use, but you definitely want some kind of language-supported synchronization variable / flag that lets you sleep until another thread modifies it. (You could just sleep for 10 seconds and then check the variables again, but polling sucks compared to an OS-supported notification.)

You don't need to multi-thread this at all, really.

Any reasonable interval between speed-tests is long enough for a tweeting function to return (even if it times out). If you don't want to include time spent tweeting in the sleep between speed-test, you could check the time before / after calling the tweet function, and subtract that many seconds from the sleep interval.

You could implement a queue like this pseudocode:

while not exitFlag:                   # Graipher suggested this loop structure
    result = speedtest()
    deadline = now() + speedtest_interval
    check_result(result, tweet_queue)  # logs and adds to queue if slow
    if (not tweet_queue.is_empty())
        try_tweet(retry_queue, deadline)    # loops until queue empty or deadline reached.
    sleep(deadline - now())

This is much simpler than threading, but can still catch up on a backlog of tweets. It keeps retrying when it can't tweet, same as your threaded version. We can make it this simple because it doesn't matter very much that the interval between speed-tests is exactly the configured number of seconds. Being stuck in a tweet-posting timeout for an extra minute is fine.


Queueing tweets that couldn't be posted is questionable.

You should at least save a timestamp for delayed tweets. Unless I missed it, currently they'll show up on twitter with just the time they're posted, not the time when the low speed was measured. And if your connection was down for a while, you'll post a flood of angry tweets when you come back online (because the results will be zero). Or maybe you handled the case where the speed-test results in an error instead of a low speed, I didn't look that carefully.