Haru

It's safe to ignore that assertion for now, or use https://github.com/HerculesWS/Hercules/pull/2877 (just merged to master) which will fix it

Item DB file structure overhaul
 
Hello~! Uguu~?!
We noticed that the Item Database file format, unchanged for years, is less than optimal (read: terrible)
The file is really hard to read (is it the fifteenth or the sixteenth zero? No wait, that line has an extra comma!!) Whenever you merge an update, if you had a customized entry, you're sure to encounter large conflicts that won't be trivial to solve How do we fix it?
We're switching to a brand new, modern, file format, making use of the libconfig library we're already using for other configuration files.
It uses libconfig. This means the parser is more solid and, perhaps not faster, but surely easier to maintain, with simpler code. And the file format is something you're already used to, since it's the same as many other configuration files we use! Empty fields and the long sequences of those hard to count commas are gone! You just specify the fields you need, and the others can be completely skipped. The item_db2 entries can be left incomplete and set to inherit the original item_db entry. If you have a custom script for your Knife[3], you can just write the script in your item_db, and let it read the other values from the item_db, so that if we update them, you get the update automatically Item scripts can be split into several lines, so they can made easier to read, especially the long ones. We can finally add more fields (to support new features) to the file at any time, easily and without having to edit all the lines (or force you to edit all the lines of your custom item_db2)! Pre-Renewal and Renewal Item databases now use the same format. This also means that you can make use of the min/max level feature in both renewal and pre-renewal (of course, pre-renewal servers will ignore the Matk field, if you specify it, since it's meaningless there) What does it look like?
 
Each entry follows this structure:
{     // =================== Mandatory fields ===============================     Id: ID                        (int)     AegisName: "Aegis_Name"       (string, optional if Inherit: true)     Name: "Item Name"             (string, optional if Inherit: true)     // =================== Optional fields ================================     Type: Item Type               (int, defaults to 3 = etc item)     Buy: Buy Price                (int, defaults to Sell * 2)     Sell: Sell Price              (int, defaults to Buy / 2)     Weight: Item Weight           (int, defaults to 0)     Atk: Attack                   (int, defaults to 0)     Matk: Magical Attack          (int, defaults to 0, ignored in pre-re)     Def: Defense                  (int, defaults to 0)     Range: Attack Range           (int, defaults to 0)     Slots: Slots                  (int, defaults to 0)     Job: Job mask                 (int, defaults to all jobs = 0xFFFFFFFF)     Upper: Upper mask             (int, defaults to any = 0x3f)     Gender: Gender                (int, defaults to both = 2)     Loc: Equip location           (int, required value for equipment)     WeaponLv: Weapon Level        (int, defaults to 0)     EquipLv: Equip required level (int, defaults to 0)     EquipLv: [min, max]           (alternative syntax with min / max level)     Refine: Refineable            (boolean, defaults to true)     View: View ID                 (int, defaults to 0)     Script: <"         Script         (it can be multi-line)     ">     OnEquipScript: <" OnEquip Script (can also be multi-line) ">     OnUnequipScript: <" OnUnequip Script (can also be multi-line) ">     // =================== Optional fields (item_db2 only) ================     Inherit: true/false           (boolean, if true, inherit the values                                   that weren't specified, from item_db.conf,                                   else override it and use default values) }, Here's a Red Potion in the old format:
501,Red_Potion,Red Potion,0,50,,70,,,,,0xFFFFFFFF,7,2,,,,,,{ itemheal rand(45,65),0; },{},{} And here's the same Red Potion in the new format:
{     Id: 501     AegisName: "Red_Potion"     Name: "Red Potion"     Type: 0     Buy: 50     Weight: 70     Script: <" itemheal rand(45,65),0; "> }, Not convinced yet it's easier to read? Okay, let's try a Poison Bottle:
678,Poison_Bottle,Poison Bottle,2,5000,,100,,,,,0xFFFFFFFF,7,2,,,,,,{ if(Class==Job_Assassin_Cross) { sc_start SC_DPOISON,60000,0; sc_start SC_ATTHASTE_INFINITY,60000,0; } else percentheal -100,-100; },{},{} changes to:
{     Id: 678     AegisName: "Poison_Bottle"     Name: "Poison Bottle"     Type: 2     Buy: 5000     Weight: 100     Script: <"         if(Class==Job_Assassin_Cross) {             sc_start SC_DPOISON,60000,0;             sc_start SC_ATTHASTE_INFINITY,60000,0;         }         else percentheal -100,-100;     "> }, Better, isn't it!? Let's now try a Jellopy:
909,Jellopy,Jellopy,3,6,,10,,,,,,,,,,,,,,{},{},{} Count the commas! Did I miss any? An extra comma you say? No, I don't want to count them, thanks...
{     Id: 909     AegisName: "Jellopy"     Name: "Jellopy"     Buy: 6     Weight: 10 }, Not even a comma!
Now, help me read what this item does?
13307,Krieger_Huuma_Shuriken1,Glorious Shuriken,4,20,,0,55,,1,0,0x02000000,7,2,34,4,80,1,22,{ bonus2 bAddRace,RC_DemiHuman,95; bonus2 bIgnoreDefRate,RC_DemiHuman,20; bonus bMatkRate,15; autobonus "{ bonus2 bSkillAtk,NJ_HUUMA,100; bonus2 bSkillAtk,NJ_ISSEN,100; }",50,10000; bonus bUnbreakableWeapon,0; if(getrefine()>5) { bonus2 bAddRace,RC_DemiHuman,(getrefine()-3)*(getrefine()-3); bonus2 bIgnoreDefRate,RC_DemiHuman,5; } if(getrefine()>8) { bonus5 bAutoSpellOnSkill,NJ_ISSEN,AL_HEAL,10,1000,1; bonus4 bAutoSpellOnSkill,NJ_HUUMA,NPC_CRITICALWOUND,2,200; } },{},{} {     Id: 13307     AegisName: "Krieger_Huuma_Shuriken1"     Name: "Glorious Shuriken"     Type: 4     Buy: 20     Atk: 55     Range: 1     Job: 0x02000000     Loc: 34     WeaponLv: 4     EquipLv: 80     View: 22     Script: <"         bonus2 bAddRace,RC_DemiHuman,95;         bonus2 bIgnoreDefRate,RC_DemiHuman,20;         bonus bMatkRate,15;         autobonus "{ bonus2 bSkillAtk,NJ_HUUMA,100; bonus2 bSkillAtk,NJ_ISSEN,100; }",50,10000;         bonus bUnbreakableWeapon,0;         if(getrefine()>5) {             bonus2 bAddRace,RC_DemiHuman,(getrefine()-3)*(getrefine()-3);             bonus2 bIgnoreDefRate,RC_DemiHuman,5;         }         if(getrefine()>8) {             bonus5 bAutoSpellOnSkill,NJ_ISSEN,AL_HEAL,10,1000,1;             bonus4 bAutoSpellOnSkill,NJ_HUUMA,NPC_CRITICALWOUND,2,200;         }     "> }, Which one did you find easier to read? Old or new format?
 
Want to see an example that specifies both min and max levels? Here you go:
{     Id: 2819     AegisName: "Swordman_Manual"     Name: "Swordman Manual"     Type: 5     Buy: 0     Weight: 100     Job: 0x00000001     Upper: 47     Loc: 136     EquipLv: [1, 12]     Refine: false     Script: <"         bonus bMaxSP,100;         skill SM_BASH,1;         skill SM_PROVOKE,1;         skill SM_MAGNUM,1;     "> }, Okay, one last example. Let's try to make an item_db2 entry for a Pupa Card that gives a bonus of 1000 HP rather than just 700.
{     Id: 4003     Inherit: true     Script: <" bonus bMaxHP,1000; "> }, Done. No need to rewrite the name, location, prices... those are already in the item_db!
 
But... I have several custom items, do I have to manually convert all of them...?
 
Of course you do! No, I'm kidding, don't hate me
 
It's true that you need to convert your item database to the new format, but we can do it for you!
Go to http://haru.ws/hercules/itemdbconverter/ and paste (or upload) your item_db2.txt (or even your item_db.txt if you have custom entries there), press the Convert button, wait a few seconds and you're done! It's already converted for you. Easy, isn't it? Don't trust us? No, no, we don't need your custom items, you can sleep safe... But if you still don't want to paste anything on a website... well, we have provided the source code of the converter script! It's in the 'tools' folder of the Hercules repository. All you need is a Perl interpreter (and if you're running Linux or Mac OS, on either your server or your own computer, it's almost certain that you already have that). All you have to do is run it (example: perl tools/itemdbconverter.pl < db/item_db2.txt > db/item_db2.conf), and your item database will be converted in a split second! What if I was using SQL item databases?
 
Well... Then you won't have to worry about the txt databases, unless you were creating the items in txt, then converting them to sql (if you're not doing this, maybe you should consider it, you know? It's easier to create and manage them in txt even if you use the sql version!)
 
If you were converting your custom item databases from the txt version to sql through the db2sql plugin, there are good news for you! We just updated the plugin, and now it's able to create a .sql script rather than just inserting the entries into your database. This means that you can create the SQL entries in your own computer, without even needing a database (just a copy of Hercules), without risking to slow down your server, and then upload them whenever you want.
 
And if you're relying on the .sql scripts we provide... well, there are still good news for you! Outdated item_db.sql files are a thing of the past now: those scripts will get updated automatically after every commit by our HerculesWS API bot, exactly like the HPM Hooks!
 
As a final note, now the pre-re and re databases share the same structure in SQL as well! no more column mismatch if you're trying to import into your Renewal server an item that was meant for pre-renewal.
 
If you have data already imported to your item_db2 SQL table, it'll be updated to the new table format through the regular upgrade sql mechanism (just run the provided script in the sql-files/upgrades folder). Please note that the script requires MySQL 5.0 or newer -- if you're still on MySQL 4.0, please open it in a text editor, read the comments and run the provided queries manually.
 
I have this event item entry that came with an old script I downloaded...
 
No worries, you can get it converted. Use the same script (or the provided web page) you'd use to convert an entire item database, it'll work just fine even for an item or two!
 
Special thanks
To Ind, for bringing up the idea and pushing it forward until it was implemented. And for his awesome work on the HerculesWS API and database converter plugin. To Yommy, for the original idea, and for some invaluable help finalizing the actual database format. Links~u!
Main commit. Removed redundant item_db2_re.sql (now both have the same structure). db2sql plugin update. item_db2 inheritance. item_db2 SQL upgrade script.

Kenpachi's fix for this is included in the v2020.03.08+2 release I just pushed, thank you!

Rationale:
This changeset offers improvements to the HPMHooking, making it capable to detect, at compile time, an error in the type of a hook function, as well as allowing pre-hooks to be more powerful when it comes to pointer-type arguments.
 
Contents:
The HPMHooking macros addHookPre() and addHookPost() have been slightly edited, and they can now detect if the type of the passed function is the correct type for the hooked function. In order to do so, the HPMHookingGen script produces one more header (HPMHooking.Defs.inc) that lists the hook function types.
This means that, if a plugin hooks into a function through HPMHooking, and the core function changes, the plugin will show a compile-time warning instead of silently compiling (and crashing at runtime or causing undesired effects).
The post-hook function types have been simplified, dropping all the extra indirection levels that were added originally.
The pre-hook function types have been changed, increasing the indirection level for pointers (now all variable types require an extra '*' in pre-hooks). This makes it possible to override const pointers from pre-hooks.
 
Impact:
Scripts that use the HPMHooking will need some small syntax changes.
 
Details:
All plugins that want to use the HPMHooking will need to #include "plugins/HPMHooking.h" (it's recommended to include it just above HPMDataCheck.h)
#include "plugins/HPMHooking.h" // Included by plugins that use the HPMHooking #include "common/HPMDataCheck.h" // Included by all plugins Then the addHookPre() and addHookPost() calls need to be updated to the new syntax, separating interface name and function name:/* Before */ HPExport void plugin_init (void) { addHookPre("pc->dropitem", my_pc_dropitem_pre); addHookPost("pc->dropitem", my_pc_dropitem_post); } /* Now */ HPExport void plugin_init (void) { addHookPre(pc, dropitem, my_pc_dropitem_pre); addHookPost(pc, dropitem, my_pc_dropitem_post); } Pre-hook functions will need an additional indirection level in their pointer-type arguments:/* Hooked function: */ int (*dropitem) (struct map_session_data *sd, int n, int amount); /* Pre-hook (before) */ int my_pc_dropitem_pre(struct map_session_data *sd, int *n, int *amount) // Only adds '*' to the non-pointers /* Pre-hook (after) */ int my_pc_dropitem_pre(struct map_session_data **sd, int *n, int *amount) // Adds '*' to everything Note: arguments of type va_list do not require an additional indirection level. 'va_list ap' remains 'va_list ap' and does not become 'va_list *ap' 
Post-hook functions will no longer need any additional indirection level in their arguments:
/* Hooked function: */ int (*dropitem) (struct map_session_data *sd, int n, int amount); /* Post-hook (before) */ int my_pc_dropitem_post(int retVal, struct map_session_data *sd, int *n, int *amount) // Adds '*' to the non-pointers /* Post-hook (after) */ int my_pc_dropitem_post(int retVal, struct map_session_data *sd, int n, int amount) // No longer adds any '*' Merge Date:Sun, 1 May 2016 20:22:03 +0300
 
Related Pull Requests:
- #1253 - https://github.com/HerculesWS/Hercules/pull/1253 - HPMHooking improvements [Haru]
 
Related Commits:
1ec9328 - https://github.com/HerculesWS/Hercules/commit/1ec9328 - Sun, 28 Feb 2016 02:12:48 +0100 Moved HPMHooking-related definitions to plugins/HPMHooking.h [Haru]
5db7c79 - https://github.com/HerculesWS/Hercules/commit/5db7c79 - Sun, 28 Feb 2016 02:17:21 +0100 Added type-checking for the addHookPre() and addHookPost() macros [Haru]
4e49441 - https://github.com/HerculesWS/Hercules/commit/4e49441 - Sun, 28 Feb 2016 02:20:40 +0100 HPM Hooks Update [Haru]
2788afc - https://github.com/HerculesWS/Hercules/commit/2788afc - Sun, 28 Feb 2016 02:40:15 +0100 Replaced memset with braced initializers in the HPMHooking hook handlers [Haru]
fa2f2f4 - https://github.com/HerculesWS/Hercules/commit/fa2f2f4 - Sun, 28 Feb 2016 02:41:01 +0100 HPM Hooks Update [Haru]
8aacecc - https://github.com/HerculesWS/Hercules/commit/8aacecc - Fri, 15 Apr 2016 19:37:54 +0200 Removed extra indirection level in HPMHooking post-hooks [Haru]
7eb4ae4 - https://github.com/HerculesWS/Hercules/commit/7eb4ae4 - Sun, 17 Apr 2016 00:38:37 +0200 HPM Hooks Update [Haru]
89e0550 - https://github.com/HerculesWS/Hercules/commit/89e0550 - Sun, 28 Feb 2016 02:48:47 +0100 Added one level of indirection to all variables in pre-hook functions [Haru]
e9c98a1 - https://github.com/HerculesWS/Hercules/commit/e9c98a1 - Sun, 28 Feb 2016 02:50:40 +0100 HPM Hooks Update [Haru]
95b4e32 - https://github.com/HerculesWS/Hercules/commit/95b4e32 - Sun, 1 May 2016 20:22:03 +0300 Merge pull request #1253 from HerculesWS/hpmhooking [Andrei Karas]

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

Rationale:
This is part of the NPC scripts standardization project. In the past, NPCs were defined with numeric View IDs, while now we've replaced most of them with more human-readable (and as such easier to maintain) constants - the same constants that AEGIS scripts use, making the numeric IDs obsolete (and deprecated).
 
Contents:
When the NPC View IDs were converted to constants, the only leftover was the special ID '-1' we use for invisible / floating NPCs, that didn't have an equivalent constant defined at the time.
This changeset defines a constant 'FAKE_NPC' for it, and replaces all the '-1' view IDs with the new constant.
The exception for the -1 case is removed from the code (making it effectively deprecated, just like the other numeric IDs).
 
Impact:
The impact of this changeset on custom scripts is low. All old code will still work (but it'll throw a deprecation warning).
It's recommended to update all the affected code as soon as possible - the support for the old style IDs may be removed from the code at any time in the future (after at least a month from the commit).
 
Details:
The use of the sprite ID '-1' in NPC headers to specify an invisible NPC is now deprecated. 'FAKE_NPC' should be used instead. This affects all NPC types (script, shop, trader, duplicate, etc)
/* Before: */ - script TurboTrap#tt_main -1,{ // ... } in_moc_16,65,162,0 duplicate(SinTrap) 02_2 -1,0,0 /* Now: */ - script TurboTrap#tt_main FAKE_NPC,{ // ... } in_moc_16,65,162,0 duplicate(SinTrap) 02_2 FAKE_NPC,0,0 Merge Date:Thu, 31 Dec 2015 23:40:48 +0100
 
Related Pull Requests:
- #1000 - https://github.com/HerculesWS/Hercules/pull/1000 - define FAKE_NPC as -1 in const.txt [AnnieRuru]
 
Related Commits:
- ef171a8 - https://github.com/HerculesWS/Hercules/commit/ef171a8 - Mon, 21 Dec 2015 19:59:38 +0800 define FAKE_NPC as -1 in const.txt [AnnieRuru]
- efaaf84 - https://github.com/HerculesWS/Hercules/commit/efaaf84 - Mon, 21 Dec 2015 20:12:45 +0800 Replace -1,{ with FAKE_NPC,{ replace using Notepad++ [AnnieRuru]
- b4c99db - https://github.com/HerculesWS/Hercules/commit/b4c99db - Tue, 29 Dec 2015 02:16:49 +0100 Replaced leftover -1 view IDs with FAKE_NPC [Haru]
- 32a42ee - https://github.com/HerculesWS/Hercules/commit/32a42ee - Tue, 29 Dec 2015 02:17:26 +0100 Extended the numeric view ID deprecation to '-1' (FAKE_NPC) [Haru]
- 0e99004 - https://github.com/HerculesWS/Hercules/commit/0e99004 - Thu, 31 Dec 2015 23:40:48 +0100 Merge branch 'AnnieRuru-request_29' into hercules [Haru]
 
Trivia:
Commit efaaf84 was done with a find and replace in Notepad++, to globally replace '-1' view IDs with 'FAKE_NPC' (as described in the commit notes).
Commit b4c99db caught the leftovers (less common cases where the -1 is not followed by ',{'), and was powered by vim macros and grep (and vimgrep). It also features manual clean up of trailing whitespace or extra tabs in a small subset of the affected lines.

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

Rationale:
This is part of a larger source cleanup project. Following the code style guidelines we decided to adopt (linux kernel guidelines), typedefs should be avoided except where necessary.
At the same time, we noticed that we have far too many explicit casts through the code, that might easily hide a coding error (for example a variable that changed type), so we're going to try and remove as many of those as possible as well.
 
Contents:
The various TBL_* typedefs are completely unnecessary (they're only needed for internal use by the BL_CAST() macro). As such, they shouldn't be used through the code, so they have been deprecated (they're not marked as deprecated yet, due to technical reasons, but you can assume they are).
Taking the chance, the BL_CAST() macro was changed to a family of macros (BL_CAST, BL_CCAST, BL_UCAST, BL_UCCAST), each with different behavior and purpose:
 
- BL_CAST(): Casts its argument to the specified type, after ensuring the block list was of the correct type. The argument must be non-constant, and it may be evaluated more than once (i.e. unsafe to use with mapit->next(x))
- BL_CCAST(): Same as BL_CAST, but it takes a const block list pointer, and returns a const object.
- BL_UCAST(): Casts its argument to the specified type, without verifying the source block list (it is the caller's care to do that). The argument is guaranteed to be evaluated only once, making it suitable to be used with mapit->next(x) or similar functions.
- BL_UCCAST(): Same as BL_UCAST(), but takes a const block list pointer, and returns a const object.
 
Impact:
Custom code may stop compiling until the appropriate changes are made. Changes are easy/trivial (mostly doable as a find and replace)
 
Details:
Any use of the TBL_* typedefs must be replaced with the corresponding struct:
 
- TBL_PC -> struct map_session_data
- TBL_NPC -> struct npc_data
- TBL_MOB -> struct mob_data
- TBL_ITEM -> struct flooritem_data
- TBL_CHAT -> struct chat_data
- TBL_SKILL -> struct skill_unit
- TBL_PET -> struct pet_data
- TBL_HOM -> struct homun_data
- TBL_MER -> struct mercenary_data
- TBL_ELEM -> struct elemental_data
 
Any explicit casts of a struct block_list to any of the above types, should be replaced with the most appropriate BL_CAST() variant (as described above).
 
Merge Date:
Wed, 6 Jan 2016 17:36:33 +0300
 
Related Pull Requests:
- #1034 - https://github.com/HerculesWS/Hercules/pull/1034 - Changed all TBL_* to the appropriate structs [Haru]
- #1024 - https://github.com/HerculesWS/Hercules/pull/1024 - Change all TBL_* to actual struct to follow unix kernel syntax [hemagx]
 
Related Commits:
- b69f7b8 - https://github.com/HerculesWS/Hercules/commit/b69f7b8 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_PC to struct map_session_data as per style guidelines [hemagx]
- 1249dc2 - https://github.com/HerculesWS/Hercules/commit/1249dc2 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_MOB to struct mob_data as per strly guidelines [hemagx]
- 3364658 - https://github.com/HerculesWS/Hercules/commit/3364658 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_HOM to struct homun_data as per style guidelines [hemagx]
- 00c95f6 - https://github.com/HerculesWS/Hercules/commit/00c95f6 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_MER to struct mercenary_data as per style guidelines [hemagx]
- b040c61 - https://github.com/HerculesWS/Hercules/commit/b040c61 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_SKILL to struct skill_data as per style guidelines [hemagx]
- 829ebdd - https://github.com/HerculesWS/Hercules/commit/829ebdd - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_PET to struct pet_data as per style guidelines [hemagx]
- 1f71f41 - https://github.com/HerculesWS/Hercules/commit/1f71f41 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_ELEM to struct elemental_data as per style guidelines [hemagx]
- 3007a37 - https://github.com/HerculesWS/Hercules/commit/3007a37 - Sat, 26 Dec 2015 11:17:14 +0200 Change all TBL_NPC to struct npc_data as per style guidelines [hemagx]
- a1b0dae - https://github.com/HerculesWS/Hercules/commit/a1b0dae - Sun, 27 Dec 2015 17:35:25 +0100 Introduced the BL_UCAST() macro as an alternative to explicit casts [Haru]
- aa574e3 - https://github.com/HerculesWS/Hercules/commit/aa574e3 - Mon, 28 Dec 2015 15:14:22 +0100 Added const variants of BL_CAST/BL_UCAST: BL_CCAST/BL_UCCAST [Haru]
- b3c722e - https://github.com/HerculesWS/Hercules/commit/b3c722e - Sun, 27 Dec 2015 18:17:24 +0100 Replaced some explicit casts with BL_UCAST/BL_UCCAST [Haru]
- f878d5e - https://github.com/HerculesWS/Hercules/commit/f878d5e - Mon, 28 Dec 2015 00:16:39 +0100 Replaced some explicit casts with BL_UCAST/BL_UCCAST [Haru]
- 2055585 - https://github.com/HerculesWS/Hercules/commit/2055585 - Mon, 28 Dec 2015 00:24:24 +0100 Replaced some map->id2sd calls with the proper map->id2XX function [Haru]
- 5db8be9 - https://github.com/HerculesWS/Hercules/commit/5db8be9 - Mon, 28 Dec 2015 02:05:09 +0100 Moved status_get_homXXX macros to status.c [Haru]
- 0e05c1e - https://github.com/HerculesWS/Hercules/commit/0e05c1e - Mon, 28 Dec 2015 15:13:02 +0100 Replaced some explicit casts with BL_UCAST [Haru]
- e3eac13 - https://github.com/HerculesWS/Hercules/commit/e3eac13 - Mon, 28 Dec 2015 15:41:36 +0100 Replaced the remaining explicit casts with BL_CAST/BL_UCAST [Haru]
- d5199ce - https://github.com/HerculesWS/Hercules/commit/d5199ce - Wed, 6 Jan 2016 17:36:33 +0300 Merge pull request #1034 from HerculesWS/bl_cast [Andrei Karas]
- f0fb759 - https://github.com/HerculesWS/Hercules/commit/f0fb759 - Wed, 6 Jan 2016 15:37:16 +0100 HPM Hooks Update [Hercules.ws]
- 0772dd0 - https://github.com/HerculesWS/Hercules/commit/0772dd0 - Wed, 6 Jan 2016 23:56:48 +0300 Fix null pointer access after previous commits. [Andrei Karas]
- 2af2132 - https://github.com/HerculesWS/Hercules/commit/2af2132 - Fri, 8 Jan 2016 16:51:59 +0100 Fixed a mapserver crash (too small allocation) [Haru]
 
Trivia:
While many of the changes here would have been possible with find and replace, I decided to review each and every of them (the actual replacement was mostly done with a vim macro, but not fully automated so I could see what the code was, and possibly do additional cleanup by hand), in order to review up some old, wrong code. It turned out that we were casting things that we didn't need to cast, or using wrong map->bl2XX() functions in several places. It costed some hours of work, but it was definitely worth it.

1. If the key passes the test on that URL, you can use it on all clients, yes (provided that you compile Hercules with the same key)
 
2. That URL will tell you. I don't really know how to generate a strong key, but I observed that the second key must be an odd number (ending with 1, 3, 5, 7, 9, B, D, F). If it's an even number, it will repeat after a few iterations. Not all the sets with an odd key are strong, but all sets where the second key is even, are weak (that happens because of the key schedule function that Gravity uses).
 
3. I'll try to explain it briefly.
After each packet sent, the client (and the server), applies a function to the current encryption (sub-)key to make it different for each packet sent (this is what prevents tools such as WPE from working for packet spamming, because if the key changes, a packet can't be re-played as it is).
Each packet will be a different encryption/decryption sub-key, which is derived from the three original keys, by applying some sort of mathematical function (this is called the key schedule). The page you linked, runs the key schedule algorithm over and over (iterates it) several times, checking if the encryption sub-key changes every time. When it detects that the key didn't change, it reports a failure. The iteration number is how many times the key schedule function was applied before it started repeating the same key.
 
I hope it makes any sense

I just pushed an update to the plugin to make it compatible with the latest Hercules version. Please test it.

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

About Code Review and Why You'd Want Your Code to Be Reviewed
 
Hello, fellow developers and code contributors!
As you certainly know, years ago, Hercules adopted a workflow based on pull requests, that includes code review as one of the necessary steps before any new piece of code makes it into the master branch of the repository.
While being an uncommon and somewhat controversial change in Hercules (and in the RO emulator scene in general), code review is part of the workflow of most software projects, both open source and closed source, and has many benefits.
 
Why Code Review
 
The benefits of code review are several:
"Given enough eyeballs, all bugs are shallow" [Linus's Law by Eric S. Raymond -- The Cathedral and the Bazaar, 1999]. While the law is not strictly true, it's certainly true that the more developers read and analyze a piece of code, the more likely it is that bugs that might be hidden in it are discovered early. Testing is not enough. It's very hard (or in the case of our codebase just plain impossible) to cover all the possible edge cases when testing a new feature or a fix. An additional pair of eyes reading the code may help discovering those more easily. This includes cases where the client would normally prevent a certain thing from happening, but it's not ensured anywhere on the server side. Better quality of code. By having other developers read a piece of code, they'll end up wondering why a certain approach was taken, rather than another, and discuss it with the submitter, leading to better, more efficient algorithms, or better engineered code. Better documentation. Since the code needs to be read by other people, it'll require proper comments (or they'll ask for explanations about the parts they can't easily explain). This increases the chance that the author, or anyone else that will need to read the same code again months or years after it's been submitted, will be able to understand it again, by finding appropriate comments in the appropriate parts of the code. Better insight into the code across the team. By reading code from different parts of the emulator as part of the review process, every team member increases their own general knowledge of the software, bit by bit. This is a very efficient way of learning how different parts of the emulator work, and why they were implemented that way. Future-proofing. By having public reviews, we keep a permanent trace of what were the hot topics and why certain decisions were taken, when a certain part of the emulator was implemented. If a bug arises, or something needs to be redesigned in future, we can look up the associated pull request and related discussion, and learn more about the discussion that went on in the past, and what's hiding behind code design decisions. Reviewing code from other people, as well as having one's own code reviewed by others might not be easy for everyone, especially at the beginning, but please try your best. Here are some suggestions on how to approach code review from either side. 
How to approach code review (for code authors)
 
As a code author, the worst thing you can do is to be afraid or shy about other people judging your code. This is the wrong approach! Don't be shy, have your code looked at by others, have them praise you for your genial approach to tackle a problem, listen to their suggestions on how to improve it. But be ready to defend your implementation, if you believe it's better than the suggestions you receive, or if the critics that are moved against it are wrong or meaningless.
 
Always keep in mind that:
Having your code reviewed and commented on isn't humiliating. Other people are spending their time looking at your code, asking you why you did something in a certain way rather than another, suggesting improvements. Both sides have a lot to learn from each others. (On the other hand, if no one reviews your code, that's somewhat humiliating!) If someone spots an issue in your code, it doesn't mean that you're a bad developer. We all make mistakes, and we should be happy to learn from them (and it's definitely better if someone spots them and points them out to us before it's too late and they were able to do some harm). Never, ever, take code review personally. No one will laugh about you, fire you, kill you, shame you, etc. if your code is commented on. If you believe you're right and the comments you received are pointless or wrong, chance is that you really are right. Be ready to defend your reasons, it's possible that the reviewer didn't think of them. It is your duty to explain them your reasons. How to approach code review (for code reviewers) 
Reviewing code is several orders of magnitude harder than having your own code reviewed. You have to check the code for several classes of problems, point out your findings, suggest improvements. And you still have to deal with the worry about hurting the code author's feelings when pointing out a mistake.
 
Here are some things you should keep in mind when reading and reviewing code from other people:
You're not judging a person. You're judging code. Don't make your review sound personal. Always think of uncommon and edge cases, and never assume they can't happen, unless there's an explicit check that makes them impossible to happen. Even if the code was tested by the author, it doesn't mean that it can't cause problems to other existing features, or have some issues the author couldn't think of. If the same person writes and tests a piece of code, the chance that they don't test the cases they forgot to handle while coding, is very close to 100%. If the code is not following the project's style guidelines (and this isn't just about indentation, but also about names, conventions about function calls, proper modularization, etc), it is your duty to point it out now, before it's merged. This will make the life of your fellow developers easier later on. Think defensively. Consider the code you have in front of you as buggy until you can prove its correctness. If you see that a sanity check is missing, ask the author to add it. If you believe that a function returns the wrong value in certain cases, even if very unlikely to occur, prepare an example of input for which that happens and point it out. Remember that threats such as overflows, underflows, buffer overruns, null pointers, invalid pointers, numeric (floating point) approximation, etc. are always behind the corner, check for them as often as possible and prove that they can't occur. And remember that, while the code author isn't your enemy (and code review shouldn't generate negative feelings), it's often a good idea to think of them as your "professional enemies". There's a chance that something nasty is hiding in their code, even if they didn't write it with ill intent, and as such, you shouldn't blindly trust the code, regardless of who the author is. Don't be afraid when you comment on other people's code. Your goal isn't to hurt their feelings, you're asking them for explanations and/or suggesting the way you would have done something. Likewise, don't be afraid of making a pointless comment. If the author has a good reason for their implementation, be ready to take back your comment and learn from them. Don't accept compromises. If you're firmly convinced that the author's defense of their code is wrong, your duty is to prove them wrong. But if they manage to convince you, don't be ashamed of admitting you were wrong. Happy reviewing!

This is something I started working on a while ago (but I had to suspend it in order to take care of some other urgent issues). 
The reason why I wanted to split it is another: the current .po file is very large, and it crashes (or is rejected by) various .po editors (both online and offline). My idea was to generate, instead, one .po for each .txt file in the scripts folder (and load them when loading each .txt). This would also have a positive side-effect of only loading the strings that are actually needed (and potentially warn you if a .po is missing).
Preliminary work to support this was started in the split-huld branch in my fork of the repository (commit 908d531f. So far only the generation is handled, the loading part is not yet implemented.
If there's interest for that, I'll resume working on it.

This is something I started working on a while ago (but I had to suspend it in order to take care of some other urgent issues). 
The reason why I wanted to split it is another: the current .po file is very large, and it crashes (or is rejected by) various .po editors (both online and offline). My idea was to generate, instead, one .po for each .txt file in the scripts folder (and load them when loading each .txt). This would also have a positive side-effect of only loading the strings that are actually needed (and potentially warn you if a .po is missing).
Preliminary work to support this was started in the split-huld branch in my fork of the repository (commit 908d531f. So far only the generation is handled, the loading part is not yet implemented.
If there's interest for that, I'll resume working on it.

This is something I started working on a while ago (but I had to suspend it in order to take care of some other urgent issues). 
The reason why I wanted to split it is another: the current .po file is very large, and it crashes (or is rejected by) various .po editors (both online and offline). My idea was to generate, instead, one .po for each .txt file in the scripts folder (and load them when loading each .txt). This would also have a positive side-effect of only loading the strings that are actually needed (and potentially warn you if a .po is missing).
Preliminary work to support this was started in the split-huld branch in my fork of the repository (commit 908d531f. So far only the generation is handled, the loading part is not yet implemented.
If there's interest for that, I'll resume working on it.