Code improvement proposals

[Taldor]

• Split up files that contain multiple public classes and/or public functions into small files with each one class or function. It allows to structure the code using directories, and makes naming files very easy: name the file like the only class or function in it. (When you have a set of functions that use the same private variables, these functions should not be separated.)
  
  
• Move all non-executable python files under one common directory (e.g. scripts/ ), or a couple with very clear differences ( e.g. engine/ and tests/ ).
  
  
• Use a model-presentation separation, instead of the much more difficult MVC structure.
  
  
• Try to give clear names to files and directories. E.g. 'common' doesn't say anything. If you can't think of a clear name, it usually means that the file or directory is not necessary.
  
  
• In 'scripts/objects/base.py' a lot of base classes are defined that set just one variable. This is considered bad practice in python. Instead, you should set the variables directly in the subclass. Don't use inheritance because you can, but when you have to.
  
  
• Use a tool like pylint to check if everyone used the mandatory coding style.
  
  
• Use verbs for methods and functions, and nouns and adjectives for variables. E.g. is_wearable would become wearable, because it's just a variable.
  
  
• When using multiple inheritance, always add super(<type>, self).__init__(), even when the superclass is 'object'. This is important to make sure all initializers are called.

Edit 1: added the seventh bullet point.
Edit 2: added the eight bullet point.

[maximinus]

I should explain that we had a good chat with Taldor over IRC, this isn't just some random posting from "A Random Guy".

Anyhow, it seems clear that a bunch of us want the code cleaned up, or at least want it in a state that people stop saying "clean it up". So feel free to post here any problems you have with the architecture of the current code.

If you don't post, we can't fix your issue!

[zenbitz]

• Split up files that contain multiple public classes and/or public functions into small files with each one class or function. It allows to structure the code using directories, and makes naming files very easy: name the file like the only class or function in it. (When you have a set of functions that use the same private variables, these functions should not be separated.)
  

I actually agree with this and I find the python practice of grouped classes maddening.  For one, you have to know both the name of the  class and the name of the package file when importing.  I am not a huge java fan but one class = one file makes way more sense to me.  I am naively assuming python can import all classes from all files in a given directory, however.

Like maximinus, I also hate having huge numbers of source files... but it's not like we have only a few now.  We have 64 .py files and 75 Classes (well, I am guessing by find ./ -name '*.py' -exec grep 'def __init__' {}\    10-20% more files isn't going to make anything any worse.  You are still going to have to be a vi/emacs whiz or use a IDE.

Use a model-presentation separation, instead of the much more difficult MVC structure.

The distinction seems subtle.  I guess the UI is reponsible for both input and output?

In 'scripts/objects/base.py' a lot of base classes are defined that set just one variable. This is considered bad practice in python. Instead, you should set the variables directly in the subclass. Don't use inheritance because you can, but when you have to.

This I don't think is relevant to base.py (although I don't disagree with the principal)  Those classes are clearly unfinished dummy classes.

Rest of your comments seem fine.

[pirum]

Namespaces (using both the name of the class and its package(s)) are really the python way of doing things. See that last line in the zen of python (open a python interpreter and type "import this"). To use a file as a single class instead of a module, you need to do some ugly hacking like maintaining a big list of file name -> class name mappings or use some kind of scheme to assume the class name based on the file name, e.g. my_class.py -> MyClass, as well as some other custom import code you'd have to slip into every __init__.py. Not only is this ugly and more of a pain to maintain, but now new developers are required to understand this custom hack import scheme. Many people have complained to the developers of the Django project for doing exactly this.

I'd rather see the code stay true to the pythonic way of doing things, e.g. have a weapons.py file and in other files use weapons.WeaponName to use some weapon class. One could also do "from weapons import *" to remove the "weapons." prefix, allowing us to just use WeaponName, but again, its unpythonic.

[maximinus]

One thing that slightly worries me if we split up the code into smaller files (one per class) is that research has been done that shows there is a 'sweet spot' in bug density, around the 200-800 line count (depends on how you count the lines).

Therefore, it's possible too many small files would increase our bugs.

[mvBarracuda]

I'm by no means a programmer so take this motion with a truckload of salt. Splittung up our sourcecode into smaller files sounds like the right approach to me. If you want to have somewhat well defined dependencies (which modules does depend on which others and how can we keep intermodule dependencies low), we'll actually need a more fine-grained folder and file structure.

[maximinus]

It could be that we don't need to split up that many files, TBH.

But there's definitely been research done into this sort of thing, and the bottom line your source shouldn't be composed of too many small files, since it increases bugs.

[zenbitz]

Namespaces (using both the name of the class and its package(s)) are really the python way of doing things. See that last line in the zen of python (open a python interpreter and type "import this"). To use a file as a single class instead of a module, you need to do some ugly hacking like maintaining a big list of file name -> class name mappings or use some kind of scheme to assume the class name based on the file name, e.g. my_class.py -> MyClass, as well as some other custom import code you'd have to slip into every __init__.py. Not only is this ugly and more of a pain to maintain, but now new developers are required to understand this custom hack import scheme. Many people have complained to the developers of the Django project for doing exactly this.

I'd rather see the code stay true to the pythonic way of doing things, e.g. have a weapons.py file and in other files use weapons.WeaponName to use some weapon class. One could also do "from weapons import *" to remove the "weapons." prefix, allowing us to just use WeaponName, but again, its unpythonic.

Both good points.  And remaining pythonic while it seems sort of silly, it probably a Good Thing (tm) for a python FOSS game.  The kind of project that will attract python zealots.

I guess what that would imply given Max and my "feelings" is that we should keep MODULES small and tight (i.e., weapons.py would contain ONLY Weapons classes and no "Fighting logic" or whatever.

[maximinus]

[/li][li]Use verbs for methods and functions, and nouns and adjectives for variables. E.g. is_wearable would become wearable, because it's just a variable.

This seems pretty logical to me, so I'll go through and check what we currently have.

[WeeGoSh]

Hi guys, this is a quite interesting thread.

I wanted to recommend one type of code structure, that is used in one of my current research-project:

This is a structure that has been some time in development and alot of tuning has been done.
Please note that not all the code might benefit from this structure but I hope this can be used as a guideline.

The project this is used in is basically a 3D (Panda3D) Simulation Framework for Social Human Behaviors.


We define all entities in the environment as class derived from Entity. The Entity class holds the essential attributes needed for all the objects in the world, like position, texture, model and more.

Then a very shallow tree is created to group together objects that share some attributes.

E.g. in our project we have:

Entity
|
|----Agent
|       |---------Avatar
|       |---------NPC
|
|----Prop

For the core of the game, e.g. the connection to the FIFE engine and setup code related to that I suggest a core package:

E.g. in our project the core is:
MainEngine               # Which is responsible for all basic operation on the Framework used and has an arcitecture that allows plugging in subSystem into it to handle other operation (editor-interface, physics, perception-system and more).

Regarding having one class per module, I agree to that most of the time, but if used too much it will result in annoying and unreadable import statements.

Just some thought.