5
\$\begingroup\$

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?

\$\endgroup\$
6
  • 3
    \$\begingroup\$ Yes Konijn asked where 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\$ Aug 20, 2020 at 22:22
  • \$\begingroup\$ I wasn't incorporating improvements, I was fixing a mistake that distracted from the overall code review. I removed some code before posting this question to make it shorter and easier to understand; this was a mistake that slipped through. Now you've re-edited my post and put the random 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\$
    – MSOACC
    Aug 20, 2020 at 22:48
  • \$\begingroup\$ I have rolled back your changes. \$\endgroup\$
    – MSOACC
    Aug 21, 2020 at 9:18
  • 2
    \$\begingroup\$ @MSOACC I have undone your rollback. Whilst it is unfortunate that you made a mistake when posting the question that is your fault not ours. It is unfair to answerers to invalidate their answers for any reason. Additionally it creates messes, which if you read Sam's links you'd know. \$\endgroup\$
    – Peilonrayz
    Aug 21, 2020 at 10:45
  • 2
    \$\begingroup\$ Yes, yes and the answer too - otherwise I wouldn't know this is answer invalidation. \$\endgroup\$
    – Peilonrayz
    Aug 21, 2020 at 11:10

1 Answer 1

6
\$\begingroup\$

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 than batchNumber += 1
  • I would have gone for maxIndex = Math.min(startingIndex + 500, dataArray.length); because now you have a lot of calls to func with undefined as a document value
  • await requires runFunctionInBatchesOf500 to be async (it is missing now)
  • I would use Array.prototype.slice() to create batches as an array, and then use forEach on each slice/batch
  • const doc = getDocFromPath(key); <- where does key 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();
}
\$\endgroup\$
6
  • \$\begingroup\$ Thanks for the feedback. I will answer each of your points. \$\endgroup\$
    – MSOACC
    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
    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
    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
    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
    Aug 21, 2020 at 9:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.