V1.2.17 - Full recalc bugfixes & FIRST/LAST quality of life updates #96
Conversation
Codecov Report
@@ Coverage Diff @@
## main #96 +/- ##
==========================================
- Coverage 96.86% 96.63% -0.24%
==========================================
Files 9 9
Lines 2742 2821 +79
==========================================
+ Hits 2656 2726 +70
- Misses 86 95 +9
Continue to review full report at Codecov.
|
… adding this manually and including branch in package version tagging process from now on
… commit status on successful package creation
…ackage version
…pended to the package version name prevented the new package version from being found
* First take on addendum for v1.2.17, patching holes in current RollupCalculator functionality for full recalculation routes that weren't already covered (so, everything besides average/first/last) * Added baseBaseCalculation method in RollupCalculator to deal with the possibility of rollups where the evaluator filters out all calc items necessitating a full recalc - which would not have happened for String and Number-based rollups previously
Co-authored-by: Michael Cleaver <Mike@gravitylab.nz>
| Integer lookupHash = String.isBlank(this.lookupKey) ? 1 : this.lookupKey.hashCode(); | ||
| Integer valHash = this.rollupValue == null ? 2 : this.rollupValue.hashCode(); | ||
| try { | ||
| hashToReturn = idHash + lookupHash + valHash; |
There was a problem hiding this comment.
OK, so this is the main problem here. You never really want to just add hashes together. This is how you get collisions. Basically you're going to want another class to do this math for you.
- Your hash should always start non-zero (I choose 17 for mine).
- For every item you want to add to the hash you should do
(currentHash * PRIME_NUMBER) + hashOfThingYouWantToAdd
I don't know the mathematics behind it, but the prime number and non-zero starting point essentially just makes it distribute better. Theres a good chapter in Effective Java by Joshua Bloch on implementing correct hash codes.
If you want I can update my hash code util class to fix its current bugs and you can just use that for this. I'll have to reread to see how he handles overflow.
There was a problem hiding this comment.
@gdoenlen thanks so much for this. I only noticed the potential for overflow this morning while examining some hashes, but then I had to rush this out the door before work actually started. I read through your util class and I'd be happy to take from that (with proper attribution of course), but if you aren't feeling like putting any more work into it I also totally understand. Either way, I'll look to cut over to the proper hash handling in the next few days - thank you again!
| Integer hashToReturn; | ||
| Integer idHash = this.Id == null ? 0 : ((Object) this.Id).hashCode(); | ||
| Integer lookupHash = String.isBlank(this.lookupKey) ? 1 : this.lookupKey.hashCode(); | ||
| Integer valHash = this.rollupValue == null ? 2 : this.rollupValue.hashCode(); |
There was a problem hiding this comment.
What is the significance of 1 and 2 here? Typically for null keys you always want 0. And for the empty string case I would just take the hashCode that SFDC has already define for it.
There was a problem hiding this comment.
No significance at all, I was just running out of steam this morning and was numbering down the constructor (hence 0, 1, 2). I'm going to run with your first comment and dig myself out of this hole.
Rollup Field On Calc Item(like DLRS). When I originally built this feature out, I hadn't architected things to make the default value easy, but things are better now