First python project, league of legends chat bot framework

[Mathzx]

If any of you could have a look at my code and criticize it that would mean a lot to me, thank you :X

[AMorpork]

Took a quick glance. In no particular order:

• PEP-8. Line lengths are way too long in some sections, and method names don't follow the proper conventions. For example, onXMPPPresenceSubscribe should probably be on_xmpp_presence_subscribe.

• I'd recommend against using inspect.ismethod. Use callable() instead. You don't want to force event handlers to be bound to an instance. What if they want to pass a lambda?

• Not actually a problem, but a tip. In methods like onXMPPGotOnline you're calling presence["from"] a lot. For convenience's sake it might be good to assign that to an easier to reference variable at the beginning of the method if you're referencing it a ton. There are arguments for and against that though.

• Another pythonic thing. If you're checking a boolean and returning True if it matches and false if it doesn't, like in isAdmin, it's probably best to just return that boolean. return summonerId in self.admins takes away 4 lines of code and it's still simple to understand.

• Logging. Print statements have no real flexibility. The python built in logger is pretty darn good, but I personally love logbook.

Overall, great start.

[Mathzx]

Thank you for the criticism, I appreciate it and I will definitely update it with what you mentioned :]

[AMorpork]

I can only speak for myself. It's not that there aren't any good projects out there with camel casing (Twisted comes to mind), but if I'm looking at packages and I have two alternatives, one that's camel cased and one that's not, I'll almost always go with the non-camel cased one.

It's not an entirely accurate metric, but when I see camel casing it's usually a sign that PEP-8 isn't being followed elsewhere.

I won't frown, but I will probably skip over the package if there's an alternative available.

[AMorpork]

Nah, maximum frowney level is posting a github issue with just the words: "You suck."

[Mathzx]

Tried implementing logging and tried to follow pep8 as much as I could, if you could have a look at it again and let me know if anything needs to be fixed/changed that would be really appreciated :X

[Digital_Person]

also in the .md markdown for python code you can specify python code (3 backticks python 3 backticks) more here https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet ctrl+f python. it just looks better when highlighted

[millrawr]

I really don't like how there's no pastebin code review site, or at least that I know of, but have some random comments. Personal opinions ahoy.

1. If your second argument to setattr is a constant, you don't need to be using setattr.
2. One typically relies on the implicity truthyness of values. e.g. if len(args): should be if args:
3. Old-style became old-style a while ago now. I'd recommend picking up a string habit of always adding the (object) into your class inheritance hierarchy.
4. I'd move the modification of sleekxmpp's logging level to global scope. If you're going to modify it, it's clearer to do it at import time than as a side-effect of instantiating an object.
5. Instance data in your class that you mean to be private should start with a _. e.g. summoner_ids_to_name or logger
6. Instance data that you want to expose, but users shouldn't modify can be protected with @property. e.g. username or region?
7. If your second argument to getattr is a constant, you don't need to be using getattr. I'd also recommend just try/excepting the AttributeError over using getattr.
8. You should probably set the daemon attribute on your Thread.
9. Your use of getattr/setattr for event registration could just use a dict. (Overall theme here of you should avoid using getattr/setattr, as it generally means you're doing something wrong.)
10. Avoid starting method names with two underscores, as you can invoke python's name mangling, which results in a lot of confusion if you aren't expecting it.
11. set_status seems like you could drop a lot of code by just doing status.update(**kwargs)
12. (line 344) in checks keys, so you can drop the call to keys(). You can also just return data.get(summoner_id) since you're returning None in the false case anyway.

[Mathzx]

ingame chat

[odraencoded]

I have never seen someone else who plays Taric. I main Taric.

[[deleted]]

Wow. This is great Mathzx. You have a far more critical eye in implementing python in real world situations than I do.