Evairfairy wrote:

Apologies for the very late patch application, it's now live on the develop branch however it currently breaks compile on linux, so I need to fix that before merging to master</p>

<p>I applied your patch and then modified it to fix a few bugs and clean the code up. I also refactored a lot of the code that was in the wrong place, however unfortunately several core functions also required refactoring, so it's a bit hard to see the changes I made to your patch.</p>

<p>Feel free to ask me about any specific changes, however these are the main flaws I noticed in your patch:</p>

<p>1. You did not update the character db version when modifying the character table - this is reason for patch rejection as it's crucial to ensure the database is up to date before trying to use it<br />
2. There were a lot of issues with the GetRbgBonus function which I'll outline in a moment (but thank you for documenting it with doxygen)<br />
3. I'd consider renaming the variable in the conf file for honor points and arena points as it's not immediately clear that the value given for honor is a multiplier, whereas the value given for arena is a constant<br />
4. (I don't really blame you for this seeing as it's not documented, but it's still an issue) Not handling the battleground list packet correctly - isRandom</p>

<p>As for the GetRbgBonus function...</p>

<p>1. Variable &quot;pl&quot; is poorly named - &quot;plr&quot; is the accepted shorthand for player<br />
2. Function is poorly named - &quot;get&quot; implies a few things, most notably that<br />
&nbsp; a. the function will not modify anything (this however is not a guarantee, use a const modifier for that)<br />
&nbsp; b. the function will return the value you are asking for</p>

<p>&nbsp; To implement GetRbgBonus as a &quot;Get&quot; function would mean returning some kind of array or struct, or splitting each result into its own function. I agree with your design choice to supply all variables at once and have the function modify them, however that's indicative of a &quot;Fill&quot; or &quot;Build&quot; function rather than &quot;Get&quot;<br />
3. Using pointers where not appropriate - whenever you supply a pointer, the function using it has to verify the pointer is not nullptr, which leads to pointless boilerplate and crashes if people forget to check it. Use references instead of pointers where possible. It's also worth noting that you can't supply a reference as a default argument, which means when building the packet using only honor points for winning, honor points for losing and arena points for winning, you still have to declare a variable to store arena points for losing. I consider this to be a minor inconvenience compared to the headache of dealing with pointers.<br />
4. Passing in an entire object where not necessary - You passed in the player object when all you needed to know was whether the player had won a battleground that day and what their level was. If you only need access to an object's data, consider just passing in the data rather than the entire object - it makes the function much easier to test and refactor.<br />
5. Writing a function knowing that it can cause a server crash - this is never, ever okay. If a function receives bad input it should throw an exception or silently return (depending on context), not just try to use it anyway. I appreciate you documenting this, but it's still important to make sure functions fail safely.</p>

<p>That's all I can think of off the top of my head, I'm sure I could nitpick some more but honestly, the patch was mostly fine - almost all of the issue were in a single function that was designed to be reasonably consistent with existing functions. It's just a shame the existing functions are such a mess ;)</p>

<p>Thank you for your patch, we greatly appreciate you taking the time to contribute to AscEmu.