Code review

[rakiru]

I've made a few changes in separate commits, so you can cherrypick only some of them if preferred:

• Added .idea folder to .gitignore - this is PyCharm's project directory
• Fixed mutable default arguments, as discussed on IRC - see commit message for possible alternative
• Replaced None equality checks with identity checks - see commit message for explanation
• Removed a redundant semicolon
• Fixed a couple errors in client/connection.py:
  • data is potentially used before definition if there's an exception while receiving it
  • sockType won't exist if socketType isn't one of the checked options

Other things:

• Many (if not all) components set self.owner inside attach(), but it isn't set anywhere else. This should probably be set to None in __init__() so that self.owner always at least exists. I'm not sure if this is something that every component should do, or if it should just be done in Component. I would do the latter, given that it seems all (or almost all) components set it anyway, but I wasn't sure what you'd rather. There are some other attributes that get created for the first time in attach() too, but those are component-specific, so those would be set to None in the respective components' __init__s. This isn't a major issue, as the only practical difference is using this code wrongly would give a slightly different error than it currently does, but it is a code smell, so thought I should point it out.
• (i) is the same as i - it does not create a tuple containing i. For that, you would need (i,). This is because otherwise parentheses couldn't be used to organise code. In cases like "%s" % (i), you can just do "%s" % i - these are equivalent. If i may be a tuple itself, you'd want to do "%s" % (i,). In fact, parentheses are not required for tuples at all - it is the comma that creates them, and the parentheses are just there to disambiguate it from, say, two separate arguments. In cases such as return (a, b), you can just do return a, b. Note: There is one special case - () creates an empty tuple, rather than just being structural parentheses.
• When you want to catch all exceptions, you generally want to use except Exception: rather than just except:. This is because things such as Ctrl-C work by raising an exception, and can end up getting caught at random parts of your code - if you use the latter snippet, you won't catch these types of exceptions and Ctrl-C will successfully propagate up the stack and close the program (or hit an actual handler for keyboard interrupts). The reason it works this way is because there is a base exception class called BaseException, which has a subclass called Exception. Almost every exception, such as ValueError and KeyError, are subclasses of Exception. Exceptions that aren't really program exceptions and more system expressions, such as KeyboardInterrupt and SystemExit, are subclasses of BaseException, as you generally don't want to catch these. I haven't fixed this because you've only got it in one place (server/socketserver.py), and didn't notice it before making this PR.
• You have a few unused imports in some files. These aren't hurting, but it's generally a good idea to clean these up. A lot of these are curses imports, so I'm assuming they're left over from when more things called curses directly.
• There are a couple of unused variables that I'm not sure if are leftovers, or if they're mistakenly not used.
  • server/room.py - The preserved parameter in Room.__init__ isn't actually stored (or used anywhere) - does this need to exist?
  • server/inputcontroller.py - InputController.do_interact has a variable nearPlaces, but then gets another list of nearby objects and uses that instead - can this be removed?

[jmdejong]

Nice, thanks!

I'll check all changes and commits before merging, but I'll reply to your mentioned poins first:

• attach() is sort of like a second constructor. Almost nothing should happen to the components before attach() is called. But yes, maybe it's better to initialize them to None

• Yes, I know that (i) is not a tuple. Where in the code is this written?

• That except: should actually be specialized to just catch a specific socket error. Good catch!

• Your assumption about the curses import is right. I later added a wrapper around window and I didn't think of the imports.

• The preserved parameter is something from a feature I'm currently working on, but then I started to question if a parameter is actually a good idea and if it wouldn't be better to use some form of polymophism instead. The nearPlaces variable is indeed redudant and probably a leftover from an attempt to make the player interact with items in nearby squares.

[rakiru]

2. I just realised these are in separate scripts, not the main project, so even less of an issue:
   • test/colourtest.py:12
   • test/test2.py:11
3. I pushed a commit to just make it catch Exception since there are quite a few possible errors there, and the error handler closes the connection, which seems the best thing to do in every case. Of course, skipping that commit and just making it catch the socket errors would be perfectly fine too.

[rakiru]

Oh, right, the files I mentioned there are just the (i) thing. There are redundant parentheses for return values in client/display/window.py lines 19, 21, 25, and 27, and in server/components/fighter.py line 75 (and while I'm at it - MessagePad has some in the class definition too.