I have a block of code that is re-used and I want to use functional programming to remove this duplication.
My code takes an array of items, splits the items into batches of 500 and then does some kind of work on them.
In the first function it deletes items from a database:
Delete function:
const deleteDocuments = async (documentReferences) => {
const batchesOf500 = Math.ceil(documentReferences.length / 500);
for(let batchNumber = 0; batchNumber < batchesOf500; batchNumber += 1) {
const batch = getBatchWriter();
const startingIndex = batchNumber * 500;
const maxIndex = startingIndex + 500;
for(let index = startingIndex; index < maxIndex; index += 1) {
if(index < documentReferences.length) {
const documentPath = documentReferences[index];
batch.delete(documentPath);
}
}
await batch.commit();
}
}
The second function is almost identical but instead of deleting from a database, it writes to the database:
Add function:
const writeToCollection = async (dataArray, collectionRef) => {
const batchesOf500 = Math.ceil(dataArray.length / 500);
for(let batchNumber = 0; batchNumber < batchesOf500; batchNumber += 1) {
const batch = getBatchWriter();
const startingIndex = batchNumber * 500;
const maxIndex = startingIndex + 500;
for(let index = startingIndex; index < maxIndex; index += 1) {
if(index < dataArray.length) {
const [key, value] = dataArray[index];
const doc = getDocFromPath(key);
batch.set(doc, value);
}
}
}
await batch.commit();
}
}
These functions are almost identical, so I have write a higher order function to do most of the leg-work.
Higher order function:
const runFunctionInBatchesOf500 = (func, dataArray) => {
const batchesOf500 = Math.ceil(dataArray.length / 500);
for(let batchNumber = 0; batchNumber < batchesOf500; batchNumber += 1) {
const batch = this.firestore.batch();
const startingIndex = batchNumber * 500;
const maxIndex = startingIndex + 500;
for(let index = startingIndex; index < maxIndex; index += 1) {
const document = dataArray[index];
func(document, batch);
}
}
await batch.commit();
}
And to it you can create your own functionality to apply to each document and use it like this:
const write = (document, batch) => {
const doc = getDocFromPath(key);
batch.set(doc, value);
};
await runFunctionInBatchesOf500(write, dataArray);
This all works but I think I am missing something. Is this an efficient use of higher order functions? What would a more elegant, FP-style solution be?
1 Answer 1
From a short review;
- Why hardcode the batch length to 500 ?
- Why not have the batch length be a nice constant ?
- You have even hard coded the length in the function name, which is really unfortunate
batchNumber++
is more canonical thanbatchNumber += 1
- I would have gone for
maxIndex = Math.min(startingIndex + 500, dataArray.length);
because now you have a lot of calls tofunc
withundefined
as adocument
value await
requiresrunFunctionInBatchesOf500
to beasync
(it is missing now)- I would use
Array.prototype.slice()
to create batches as an array, and then useforEach
on each slice/batch const doc = getDocFromPath(key);
<- where doeskey
come from, an evil global?
I personally would be mildly evil by adjusting the Array prototype so that I can keep chaining, FP style;
Array.prototype.mapSlice = function arrrayMapSlice(n){
//Just return `this` if we get a non-sensical parameter
if(isNaN(n) || n <= 0){
return this;
}
let start = 0, out = [];
while(start < this.length){
out.push(this.slice(start, start+=n));
}
return out;
}
async function runBatches(list, f, batchSize){
batchSize = batchSize || 500;
list.mapSlice(batchSize).forEach(batch => {
const firestoreBatch = this.firestore.batch();
batch.forEach(document => f(document, firestoreBatch ));
});
await batch.commit();
}
-
\$\begingroup\$ Thanks for the feedback. I will answer each of your points. \$\endgroup\$MSOACC– MSOACC2020年08月20日 22:19:19 +00:00Commented Aug 20, 2020 at 22:19
-
\$\begingroup\$ 1. The DB I am using (Firebase) has a hard limit of 500 documents per write so I have to adhere to that. 2. It could be a constant yeah. 3. The 500 number won't change so I'm not worried about putting the number in the name. 4. My linter enforces incrementing via
+=1
rather than++
. Check the "no-plusplus" rule. 5. I do actually exit the function when the index exceeds the max; I just removed the code to make the example more succinct. Good spot though ;) \$\endgroup\$MSOACC– MSOACC2020年08月20日 22:24:06 +00:00Commented Aug 20, 2020 at 22:24 -
\$\begingroup\$ 6. Why would I not want to make the function async? I don't want the code to proceed until the write has completed. 7. Yeah, using slice() probably is better. 8. That's another mistake from me shortening the code for readability. I've updated the question to fix it. \$\endgroup\$MSOACC– MSOACC2020年08月20日 22:25:52 +00:00Commented Aug 20, 2020 at 22:25
-
\$\begingroup\$ This is good feedback; I am also looking for advice regarding the overall approach. This is the first time I have used higher order / FP in real-world usage and I still think there's room for improvement; I just can't put my finger on it. \$\endgroup\$MSOACC– MSOACC2020年08月20日 22:27:13 +00:00Commented Aug 20, 2020 at 22:27
-
1\$\begingroup\$ Last comment since comments should not be a chat, but please post the actual code next time, dont simplify it for CodeReview, it is against the rules of CodeReview. \$\endgroup\$konijn– konijn2020年08月21日 09:45:37 +00:00Commented Aug 21, 2020 at 9:45
Explore related questions
See similar questions with these tags.
key
comes from but changing the code would invalidate that answer. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see What should I do when someone answers my question? as well as what you may and may not do after receiving answers. \$\endgroup\$key
value back in but to what end? Other users will notice it and either mention it or be confused when my request is regarding functional programming. \$\endgroup\$