Your current solution has some bugs in it that we can squash.
Always false
equality check
You use the following equality check to determine if the data has not changed:
if (previousData === data) {
return null;
}
This will always be false
as the objects returned by change.before.data()
and change.after.data()
will always be different instances, even if they contain the same data.
Company changes are never handled
While this could be a rare, maybe impossible event, if a user's company was changed, you should remove their weight from the total of the original company and add it to the new company.
In a similar vein, when a employee leaves a company or deletes their account, you should remove their weight from the total in a onDelete
handler.
Handling floating-point sums
In case you didn't know, floating point arithmetic has some minor quirks. Take for example the sum, 0.1 + 0.2
, to a human, the answer is 0.3
, but to JavaScript and many languages, the answer is 0.30000000000000004
. See this question & thread for more information.
Rather than store your weight in the database as a floating point number, consider storing it as an integer. As weight is often not a whole number (e.g. 9.81kg
), you should store this value multiplied by 100 (for 2 significant figures) and then round it to the nearest integer. Then when you display it, you either divide it by 100 or splice in the appropriate decimal symbol.
const v = 1201;
console.log(v/100); // -> 12.01
const vString = String(v);
console.log(vString.slice(0,-2) + "." + vString.slice(-2) + "kg"); // -> "12.01kg"
So for the sum, 0.1 + 0.2
, you would scale it up to 10 + 20
, with a result of 30
.
console.log(0.1 + 0.2); // -> 0.30000000000000004
console.log((0.1*100 + 0.2*100)/100); // -> 0.3
But this strategy on its own isn't bullet proof because some multiplications still end up with these errors, like 0.14*100 = 14.000000000000002
and 0.29*100 = 28.999999999999996
. To weed these out, we round the multiplied value.
console.log(0.01 + 0.14); // -> 0.15000000000000002
console.log((0.01*100 + 0.14*100)/100); // -> 0.15000000000000002
console.log((Math.round(0.01*100) + Math.round(0.14*100))/100) // -> 0.15
You can compare these using:
const arr = Array.from({length: 100}).map((_,i)=>i/100);
console.table(arr.map((a) => arr.map((b) => a + b)));
console.table(arr.map((a) => arr.map((b) => (a*100 + b*100)/100)));
console.table(arr.map((a) => arr.map((b) => (Math.round(a*100) + Math.round(b*100))/100)));
Therefore we can end up with these helper functions:
function sumFloats(a,b) {
return (Math.round(a * 100) + Math.round(b * 100)) / 100;
}
function sumFloatsForStorage(a,b) {
return (Math.round(a * 100) + Math.round(b * 100));
}
The main benefit of handling the weights this way is that you can now use FieldValue#increment()
instead of a full blown transaction to shortcut updating the value. In the rare case that two users from the same company have an update collision, you can either retry the increment or fall back to the full transaction.
Inefficient data parsing
In your current code, you make use of .data()
on the before and after states to get the data you need for your function. However, because you are pulling the user's entire document, you end up parsing all the fields in the document instead of just what you need - the bodyweight
and company
fields. You can do this using DocumentSnapshot#get(fieldName)
.
const afterData = change.after.data(); // parses everything - username, email, etc.
const { bodyweight, company } = afterData;
in comparison to:
const bodyweight = change.after.get("bodyweight"); // parses only "bodyweight"
const company = change.after.get("company"); // parses only "company"
Redundant math
For some reason you are calculating an absolute value of the difference between the weights, storing the sign of difference as a boolean and then using them together to apply the change back to the total weight lost.
The following lines:
const previousData = change.before.data();
const data = change.after.data();
const newWeight = data.bodyweight;
const oldWeight = previousData.bodyweight;
const lostWeight = oldWeight > newWeight;
const difference = diff(newWeight, oldWeight);
const currentWeightLost = companyRef.data().weightLostByAllEmployees;
const calcNewCWL = (currentWeightLost, difference, lostWeight) => {
if (!lostWeight) {
return currentWeightLost - difference;
}
return currentWeightLost + difference;
};
const newWeightLost = calcNewCWL(currentWeightLost, difference, lostWeight);
could be replaced with just:
const newWeight = change.after.get("bodyweight");
const oldWeight = change.before.get("bodyweight");
const deltaWeight = newWeight - oldWeight;
const currentWeightLost = companyRef.get("weightLostByAllEmployees") || 0;
const newWeightLost = currentWeightLost + deltaWeight;
Rolling it all together
exports.updateCompanyProgress = functions.firestore
.document("users/{userID}")
.onUpdate(async (change, context) => {
// "bodyweight" is the weight scaled up by 100
// i.e. "9.81kg" is stored as 981
const oldHundWeight = change.before.get("bodyweight") || 0;
const newHundWeight = change.after.get("bodyweight") || 0;
const oldCompany = change.before.get("company");
const newCompany = change.after.get("company");
const db = admin.firestore();
if (oldCompany === newCompany) {
// company unchanged
const deltaHundWeight = newHundWeight - oldHundWeight;
if (deltaHundWeight === 0) {
return null; // no action needed
}
const companyRef = db.doc(`/companies/${newCompany}`);
await companyRef.update({
weightLostByAllEmployees: admin.firestore.FieldValue.increment(deltaHundWeight)
});
} else {
// company was changed
const batch = db.batch();
const oldCompanyRef = db.doc(`/companies/${oldCompany}`);
const newCompanyRef = db.doc(`/companies/${newCompany}`);
// remove weight from old company
batch.update(oldCompanyRef, {
weightLostByAllEmployees: admin.firestore.FieldValue.increment(-oldHundWeight)
});
// add weight to new company
batch.update(newCompanyRef, {
weightLostByAllEmployees: admin.firestore.FieldValue.increment(newHundWeight)
});
// apply changes
await db.batch();
}
});
With transaction fallbacks
In the rare case where you get a write collision, this variant falls back to a traditional transaction to reattempt the change.
/**
* Increments weightLostByAllEmployees in all documents atomically
* using a transaction.
*
* `arrayOfCompanyRefToDeltaWeightPairs` is an array of company-increment pairs.
*/
function transactionIncrementWeightLostByAllEmployees(db, arrayOfCompanyRefToDeltaWeightPairs) {
return db.runTransaction((transaction) => {
// get all needed documents, then add the update for each to the transaction
return Promise
.all(
arrayOfCompanyRefToDeltaWeightPairs
.map(([companyRef, deltaWeight]) => {
return transaction.get(companyRef)
.then((companyDocSnapshot) => [companyRef, deltaWeight, companyDocSnapshot])
})
)
.then((arrayOfRefWeightSnapshotGroups) => {
arrayOfRefWeightSnapshotGroups.forEach(([companyRef, deltaWeight, companyDocSnapshot]) => {
const currentValue = companyDocSnapshot.get("weightLostByAllEmployees") || 0;
transaction.update(companyRef, {
weightLostByAllEmployees: currentValue + deltaWeight
})
});
});
});
}
exports.updateCompanyProgress = functions.firestore
.document("users/{userID}")
.onUpdate(async (change, context) => {
// "bodyweight" is the weight scaled up by 100
// i.e. "9.81kg" is stored as 981
const oldHundWeight = change.before.get("bodyweight") || 0;
const newHundWeight = change.after.get("bodyweight") || 0;
const oldCompany = change.before.get("company");
const newCompany = change.after.get("company");
const db = admin.firestore();
if (oldCompany === newCompany) {
// company unchanged
const deltaHundWeight = newHundWeight - oldHundWeight;
if (deltaHundWeight === 0) {
return null; // no action needed
}
const companyRef = db.doc(`/companies/${newCompany}`);
await companyRef
.update({
weightLostByAllEmployees: admin.firestore.FieldValue.increment(deltaHundWeight)
})
.catch((error) => {
// if an unexpected error, just rethrow it
if (error.code !== "resource-exhausted")
throw error;
// encountered write conflict, fall back to transaction
return transactionIncrementWeightLostByAllEmployees(db, [
[companyRef, deltaHundWeight]
]);
});
} else {
// company was changed
const batch = db.batch();
const oldCompanyRef = db.doc(`/companies/${oldCompany}`);
const newCompanyRef = db.doc(`/companies/${newCompany}`);
// remove weight from old company
batch.update(oldCompanyRef, {
weightLostByAllEmployees: admin.firestore.FieldValue.increment(-oldHundWeight)
});
// add weight to new company
batch.update(newCompanyRef, {
weightLostByAllEmployees: admin.firestore.FieldValue.increment(newHundWeight)
});
// apply changes
await db.batch()
.catch((error) => {
// if an unexpected error, just rethrow it
if (error.code !== "resource-exhausted")
throw error;
// encountered write conflict, fall back to transaction
return transactionIncrementWeightLostByAllEmployees(db, [
[oldCompanyRef, -oldHundWeight],
[newCompanyRef, newHundWeight]
]);
});
}
});