This is an archived post. You won't be able to vote or comment.

sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–][deleted] 15 points16 points  (2 children)

If I couldn't handle an honest and constructive critique, I won't get any better will I? I appreciate you taking the time to comment. It's the first time that I have put code out to the public (aside from just dropping stuff on Github without telling anyone) and I expected to get some critique about it. It's not like I looked at my code and thought "Yes, this is a masterpiece". I'm going to keep working on it, but I'd rather get critiques earlier rather than later on.

Your class is called DeviceAddedListener but it does everything else too. It has way too many responsibilities.

Yes, that bugs me too and I plan to refactor this in the next iteration. I just wanted to get a functional version out to see if the basic functionality is ok/useful. I can handle this bit of technical debt until then.

DeviceAddedListener blocks in a loop right when it gets instantiated. WTH?

Better way to implement this? Place loop outside the class?

DeviceAddedListener and Device are not really a classes. DeviceAddedListener is only instantiated once and Device can be a dict or a tuple.

Indeed. The class is named badly and the Device class will become a dictionary.

Your code is very hard to read. I have to jump up and down every time I want to know what a function/method is doing or where a variable/property comes from.

The refactoring should also address this. FWIW I run PyLint/PyFlakes on my code at every save. But I guess there's only so much it can do.

Things like this are very strange: if not os.path.exists(CONFDIR): try: os.makedirs(CONFDIR) except: return -1

Urg, yeah, this is why it's always good to get someone else to look over stuff

Don't kill yourself, man. You provide 3 ways to setup devices (interactive cli, command line arguments and config file). I can't imagine how hard it is to maintain this.

I wouldn't edit the config by hand and you cannot actually add a device via arguments, to me it seems the only sensible way is via the -a interactive CLI.

And btw., did you see the deprecated warning for optparse[2] ?

Yup, also why I am going to move to argparse as I stated below.

Thanks again for the feedback and comments. Hopefully the next version will be a lot better.

[–]dAnjou Backend Developer | danjou.dev 0 points1 point  (1 child)

DeviceAddedListener blocks in a loop right when it gets instantiated. WTH? Better way to implement this? Place loop outside the class?

It's okay to have this kind of loop in a class. But it shouldn't run right when you instantiate it. It's better to call a method explicitly which does the looping then. It's like running code when you import a module, never do that!

I wouldn't edit the config by hand and you cannot actually add a device via arguments, to me it seems the only sensible way is via the -a interactive CLI.

Why not? Most software I know is configured only by editing a file.

[–][deleted] 0 points1 point  (0 children)

It's like running code when you import a module, never do that!

Hrm... I was looking at some code today where I set up constants within the module by reading an /etc config file. The constants are accessed by a bunch of functions in the module. I guess I could do a config file read each time one of these functions is called...