Skip to main content
added 8 characters in body
Source Link
Graipher
  • 41.1k
  • 7
  • 69
  • 133

Overall this is quite a nice module. Here are a few usability issues/nitpicks, though:

  1. When running the script, there is no easy way to stop it. CTRL+C does not work, I have to manually kill the process. This is probably because of how threading.Thread handles it, but I'm not sure.

  2. It would be nice if it was slightly easier to add a different handler than the tweeter. For example, I might want to send myself an email, instead of tweeting it right away. To fix this, you should remove all mentions of tweeting from the SpeedTestThread class and maybe define a general EventHandler class, from which TwitterThread derives, which calls some method action(self, up, down) which needs to be defined in the derived class. This way you can easily define a MailThread(EventHandler) with a different action method that sends a mail instead.

  3. You are mixing two variable naming styles, camelCase and lower_case_with_underscores. Python's official style-guide, PEP8, recommends sticking to the latter for all variables and functions/methods. Global constants should be in UPPER_CASE.

  4. For flags you should just use True and False, it is a bit easier to read. NotNote that you can do while not exitFlag: instead of while True: if exitFlag == 1: break and if prevError instead of if prevError == 1.

  5. You never increase your error_logger.counter, it should be if prevError: self.error_logger.counter += 1 else: self.error_logger.counter = 0.

Overall this is quite a nice module. Here are a few usability issues/nitpicks, though:

  1. When running the script, there is no easy way to stop it. CTRL+C does not work, I have to manually kill the process. This is probably of how threading.Thread handles it, but I'm not sure.

  2. It would be nice if it was slightly easier to add a different handler than the tweeter. For example, I might want to send myself an email, instead of tweeting it right away. To fix this, you should remove all mentions of tweeting from the SpeedTestThread class and maybe define a general EventHandler class, from which TwitterThread derives, which calls some method action(self, up, down) which needs to be defined in the derived class. This way you can easily define a MailThread(EventHandler) with a different action method that sends a mail instead.

  3. You are mixing two variable naming styles, camelCase and lower_case_with_underscores. Python's official style-guide, PEP8, recommends sticking to the latter for all variables and functions/methods. Global constants should be in UPPER_CASE.

  4. For flags you should just use True and False, it is a bit easier to read. Not that you can do while not exitFlag: instead of while True: if exitFlag == 1: break and if prevError instead of if prevError == 1.

  5. You never increase your error_logger.counter, it should be if prevError: self.error_logger.counter += 1 else: self.error_logger.counter = 0.

Overall this is quite a nice module. Here are a few usability issues/nitpicks, though:

  1. When running the script, there is no easy way to stop it. CTRL+C does not work, I have to manually kill the process. This is probably because of how threading.Thread handles it, but I'm not sure.

  2. It would be nice if it was slightly easier to add a different handler than the tweeter. For example, I might want to send myself an email, instead of tweeting it right away. To fix this, you should remove all mentions of tweeting from the SpeedTestThread class and maybe define a general EventHandler class, from which TwitterThread derives, which calls some method action(self, up, down) which needs to be defined in the derived class. This way you can easily define a MailThread(EventHandler) with a different action method that sends a mail instead.

  3. You are mixing two variable naming styles, camelCase and lower_case_with_underscores. Python's official style-guide, PEP8, recommends sticking to the latter for all variables and functions/methods. Global constants should be in UPPER_CASE.

  4. For flags you should just use True and False, it is a bit easier to read. Note that you can do while not exitFlag: instead of while True: if exitFlag == 1: break and if prevError instead of if prevError == 1.

  5. You never increase your error_logger.counter, it should be if prevError: self.error_logger.counter += 1 else: self.error_logger.counter = 0.

Source Link
Graipher
  • 41.1k
  • 7
  • 69
  • 133

Overall this is quite a nice module. Here are a few usability issues/nitpicks, though:

  1. When running the script, there is no easy way to stop it. CTRL+C does not work, I have to manually kill the process. This is probably of how threading.Thread handles it, but I'm not sure.

  2. It would be nice if it was slightly easier to add a different handler than the tweeter. For example, I might want to send myself an email, instead of tweeting it right away. To fix this, you should remove all mentions of tweeting from the SpeedTestThread class and maybe define a general EventHandler class, from which TwitterThread derives, which calls some method action(self, up, down) which needs to be defined in the derived class. This way you can easily define a MailThread(EventHandler) with a different action method that sends a mail instead.

  3. You are mixing two variable naming styles, camelCase and lower_case_with_underscores. Python's official style-guide, PEP8, recommends sticking to the latter for all variables and functions/methods. Global constants should be in UPPER_CASE.

  4. For flags you should just use True and False, it is a bit easier to read. Not that you can do while not exitFlag: instead of while True: if exitFlag == 1: break and if prevError instead of if prevError == 1.

  5. You never increase your error_logger.counter, it should be if prevError: self.error_logger.counter += 1 else: self.error_logger.counter = 0.