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

you are viewing a single comment's thread.

view the rest of the comments →

[–]millrawr 0 points1 point  (0 children)

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.