-
Notifications
You must be signed in to change notification settings - Fork 101
Description
The current code has major issues that make it nearly unusable for anything reliable or production-grade. Honestly, it’s hard to believe this is the code shipped with a product. I want to point out some of the most serious problems.
First off, you're using ctypes to kill threads like this is a normal thing to do:
try:
stop_thread(self.video)
stop_thread(self.instruction)
except:
passNot only is killing threads this way dangerous, but it can also leave hardware in unpredictable states or cause memory corruption. This is not how threads should be stopped in Python. There are much safer ways to handle this, like threading.Event, which would let threads exit cleanly rather than being terminated abruptly.
Next, the use of comparison operators instead of assignments is just baffling:
self.balance_flag == FalseThat line doesn’t actually do anything. What you probably meant to write was self.balance_flag = False, but with ==, it just checks a condition and then moves on. No change happens, and no one notices, until the bug pops up down the line. For something controlling actual hardware, this is borderline reckless.
But the worst part might be this:
try:
stop_thread(thread_power)
except:
passThere is no thread_power. That variable literally doesn’t exist. It’s like writing abrakadabra and hoping it works. And even if it did exist, we’d still have the same unsafe thread termination issue I mentioned earlier. But come on. You’ve got code here that doesn’t even run, and it’s obvious no one even tested this.
There’s also the overuse of magic numbers scattered throughout the code, making it nearly impossible to know what's really going on:
for i in range(450, 89, -self.speed):What does 450 represent? Why is 89 the cutoff? Without any explanation or comments, anyone trying to work with this code is left guessing. Magic numbers like these should at least be named constants or, even better, loaded from a configuration.
Oh, and please stop doing this:
from * import *This is terrible practice. It makes code harder to read, harder to track where things are coming from, and generally causes more problems than it solves. Stick to importing only what you need or use a namespace.
Now to sum up the real-world problems this code causes:
- Undefined variables (like
thread_power) and logic errors (such as using==instead of=) lead to unexpected behavior, or worse - silent bugs you won’t notice until things start failing at random. - Abrupt thread termination could crash the hardware or leave it in unreliable states.
- Magic numbers littered everywhere make the system impossible to maintain or modify.
What needs to change:
- Stop using
ctypesto kill threads. Use proper thread termination withthreading.Event. - Fix the blatant errors with
==where=should be used, and get rid of undefined variables likethread_power. - Remove magic numbers, or at least replace them with named constants or configuration parameters.
- Add basic exception handling. Don't just write
except: pass, because then you'll never see useful error messages for debugging. - Perform static code linting using tools like
pyright,flake8orpylint.
Honestly, it feels like you shipped this without running even the most basic tests. People paid for this, and they should at least get code that has been checked to work correctly.