development Conventions

From OpenKore Wiki
Revision as of 12:16, 18 November 2012 by EternalHarvest (talk | contribs)
Jump to navigation Jump to search

Disclaimer: this page is made of existing openkore source and common sense. These are only guidelines which may not always apply.

General

Look at existing code. If nothing says otherwise, do the same as most of the code does (methodology, names, formatting etc).

Backwards Compatibility

OpenKore used to have excellent backwards compatibility both for users and developers, and generally still does, unless non-caring let-s-break-this-I-dont-care contributors prevail. It's so excellent that almost all commands and configuration options of Skore, the predecessor of OpenKore back from 2003, still may be used exactly in the same way as before. That's one of the greatest advantages of Kore - regardless of features added, bugs fixed and systems reworked, you're still good with it as you were some day.

New versions of openkore shouldn't break old configurations and plugins. It means to avoid unwarranted changes to any existing behaviour, any default behaviour or any internal programming interfaces and names. It doesn't help users only, but developers familiar with various parts of codebase as well.

Recently implemented or highly experimental stuff may depend less or backwards compatibility.

Justified compatibility breaking examples:

  • removal of already deprecated APIs which caused problems (%field hash - but even it could have been tie'd instead of removal, and this change caused many problems regardless)
  • rework of broken and hardly functioning systems (XKore transition to be more dependent on serverType system)
  • non-dangerous name changes of utility commands, to get more uniform command naming overall (cl is chat list now, not clear log)

Unjustified compatibility breaking examples:

  • customizable per-server item/skill/monster names, which broke all configurations - broke compatibility and interoperability hard
  • hook argument changes just for the sake of it (r7726) - changing existing hook arguments is never a good idea, just add more arguments or add more hooks instead (adding arguments to existing hooks is fine); anyway, all incompatible hook changes MUST be added to Porting Plugins documentation
  • breaking existing behaviour of commands (r7653 - fixed)
  • obscure and not properly tested (unittests are necessary) changes of core systems, which may break many random things (map instances - fixed; line of sight)

Compatibility workaround examples:

Behaviour Changes

While some changes may not really break compatibility, they may introduce some unwanted behaviour changes. Treat this situation the same as compatibility changes.

Unjustified behaviour breaking examples:

  • removing secure-by-default behaviour which many users rely on, making it a non-default option when motives for such behaviour in the first place weren't changed (r8118, shopAuto_open and lockMap - fixed)

Consistency

Generally, new features and names should follow the general ideas of naming of existing stuff; be guessable so you don't have to look documentation up every time you're need them; and reflect what named features actually do.

Bad examples:

  • total mismatch between name and feature (teleportAuto_search isn't for teleporting and won't really do any search by itself; buy_bulk_vender packet is not for buying, but for selling items)
  • hijacking of names in existing name theme (pl, ml, vl etc listing commands, but cl clear log - fixed)
  • mismatch in naming scope (shop_LockOnly affects only shopAuto_* options, compare with shop_random which affects both auto and manually opened shops - fixed)
  • not following general name uppercasing theme (shop_LockOnly, while all other options use lowercase letter after an underscore; GANSI_RANK and ISVR_DISCONNECT packets, while all other packets prefer lowercase names - fixed)
  • not following similar name theme across different features (shop_LockOnly, while there already were inLockOnly, attackAuto_inLockOnly, avoidList_inLockOnly - fixed)
  • using random abbreviations, especially in a set where no other member is abbreviated (Glt. Cross job name), especially when it's used in configurations (such names are usually unguessable unless you look them up); our own conventional abbreviations, like LOS, KS, Dmg are fine to stick to

If you're unsure of how to name your new options or other stuff, just ask at forums for opinions.

Formatting

K&R style, with opening brace on the same line with all declarations.

Indent with TABs. Don't use TABs for other means. There is no defined column size for TAB, each developer can have its own preference, respect that.

sub test {
<TAB> if ($a == $b) {
<TAB> <TAB> something();
<TAB> <TAB> something_else($a, $b);
<TAB> }
<TAB> $twenty = 20;
<TAB> $ten    = 10; # use spaces if you want to align (not indent) code like this for some reason
}

TabsSpacesBoth.png

Naming

Language: only english

Constants: UPPER_CASE

Variables: $camelCase or $underscore_separated

Packet "names": underscore_separated_words

Config option names: camelCase, regular words like "auto" at the end, however underscore may separate a common prefix (teleportAuto). Underscore is also internally used to represent config blocks

Comments

Language: only english, with no translations to other languages; quotations from sources on another languages may be included if it's necessary

Keywords: FIXME, TODO