development Conventions: Difference between revisions

From OpenKore Wiki
Jump to navigation Jump to search
(fixed in 8304)
Line 41: Line 41:
* total mismatch between name and feature ([[teleportAuto_search]] isn't for teleporting and won't really do any search by itself; <code>buy_bulk_vender</code> packet is not for buying, but for selling items)
* total mismatch between name and feature ([[teleportAuto_search]] isn't for teleporting and won't really do any search by itself; <code>buy_bulk_vender</code> 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)
* 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)
* 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; <code>GANSI_RANK</code> and <code>ISVR_DISCONNECT</code> packets, while all other packets prefer lowercase names)
* not following general name uppercasing theme ([[shop_LockOnly]], while all other options use lowercase letter after an underscore; <code>GANSI_RANK</code> and <code>ISVR_DISCONNECT</code> 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]])
* not following similar name theme across different features ([[shop_LockOnly]], while there already were [[inLockOnly]], [[attackAuto_inLockOnly]], [[avoidList_inLockOnly]] - fixed)


If you're unsure of how to name your new options or other stuff, just ask at forums for opinions.
If you're unsure of how to name your new options or other stuff, just ask at [http://forums.openkore.com/viewforum.php?f=37 forums] for opinions.


== Formatting ==
== Formatting ==

Revision as of 04:30, 17 November 2012

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

Following the idea of backwards compatibility, new features and names should be made while looking over existing code and names. On the other side, names should try to reflect what they 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)

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