I'm opening this new topic with the sole purpose of providing a guidelines for patches and team development practices.
Please, follow those guidelines when working on something else than your own private branch.
If you disagree with anything, please comment and we'll update this first post accordingly once we agree on the change.
The purpose of all this is to permit more code quality to happen to this project and to permit proper team work. Bear this in mind when coding. Thanks.
- Development course:
For reference, OD development is happening here: http://sourceforge.net/p/opendungeons/g ... ster/tree/
- The 'master' branch is used for clean release purpose only. This means only bugfixes or release-ready patches should go there, usually coming from the 'development' branch.
- The 'development' branch is a Team branch. This means patches that are either not desired, incomplete, untested, breaking game functionalities, or not following the code style guidelines must not go there. Please, see this as a public area, where you should avoid bothering your coding neighbors.
This branch is made to code on bleeding edge features, yet with reviewed and streamlined patches.
- Each dev can have its own private branch, eg: 'bertram-integration' where he/she does tink whatever he/she wants. Each dev should try to keep its own branch up-to-date
with the development branch, and put his/her commits on top of it, using 'git pull --rebase' (See the hints part.). Why? Because it makes commit reviews much easier, and you are your first own code reviewer, after all, right?
- The best way to put things on 'development':
Once you've done a patch or a serie of patches, and are happy with it, you can ask for a review of another coder. To be reviewed, the resulting change should the most possible:
- Follow the code style guidelines.
- Be readable. (this also means be properly indented.)
- Be commented on the difficult parts.
- Doesn't add warnings.
- Compile. (eh)
- Is tested to not break any functionalities, especially in the game area of the change. (By both devs if possible.)
- When adding a feature, well, adds a feature that is desired. This means you shouldn't add, well for instance, GPS support to OD without everyone agree with it, as it is a nonsense.
The dev reviewed could then fix all the stuff noticed by the other one in another patch until it's good. He/she will then be able push the overall stuff to development.
- Hints:
- Don't avoid doing some of those things because it is 'quicker', or 'easier'.
We have no time pressure as it is a spare-time project. We have no deadlines, so you have the time to make things properly. If you feel in a rush, it means you lack spare-time, not that you should hurry pushing things. Better resume at another time, believe me.
- Discuss new features details and make them validated before starting coding on them: The risk is fairly simple, we might end up with an undesired feature, or a features that is working but doesn't fit the other components and the team plans. This usually ends badly as you want to push it on the common branches and others don't, so don't do that.
- Try to keep your unmerged patches based on the tip of 'development' branch by using 'git pull --rebase development' on your private branch before asking for review. This will make the life of the reviewer easier.
- Try to avoid using too much deepness in your code as it makes it harder to read and usually forces people to make more complex things to get out of the loops properly.
Eg:
- {l Code}: {l Select All Code}
function()
{
if (true)
{
// long stuff
// ...
}
}
is uselessly complex as you can do:
- {l Code}: {l Select All Code}
function()
{
if (false)
return;
// long stuff
// ...
}
If you can't do that for any reason and your code is becoming complex, maybe it's time to put some of it into a sub-function.
- Respect the life-cycle of objects
This means that when you create an object somewhere (with keyword 'new'), you should the most possible delete it within the same parent object or function if not a parent object.
Doing otherwise makes things tedious and insane to debug. It is considered a very bad practice in most cases, anyway.
- Don't use an object or a functionality because you fancy it, but rather because you need it to make things work.
As an extreme example, it's not because B-tree components exist that you have to use them to parse a simple stack of items. There are sets, vectors or list for that.
The more simply you'll use objects, the more it will be readable by yourself and by others. It's not a competition about the guy who knows the most about high-grade c++ stuff.
