Copy and Pasting Values from Spreadsheets

Question

Background

This code is apart of a library I'm developing for my company. It takes in anywhere from 3 to 5 arguments. All of the code functions properly. To be more specific, a user has data on a spreadsheet that they want copied somewhere else. The first function takes the data the user wants copied and puts it somewhere else on the same sheet in a spreadsheet. The second function takes the data and pastes it on a different sheet within the same spreadsheet. The third and fourth functions are the ones that I expect to be used most often. They copy and paste data from one spreadsheet to another spreadsheet. They are the more complex of the methods within this function. The reason there are two of them is because the format of the range can either be in array or string format. For example: if you wanted to paste data from cell A1 to cell B1, you could set copyRange = 'A1' or copyRange = [1,1,1,1], and then set pasteRange = 'B1' or pasteRange = [1,1].

A caveat though is that if the copyRange is in array format, pasteRange must also be in array format, and vice-versa. Same if either was in string format.

Another caveat that may not have been obvious is that copyRange has four indexes, whereas pasteRange only has two. This is because the setValues() function that GAS uses, the size of the data array being copied must be the same size as the data array being pasted to. To avoid user error that the data arrays are not the same size, the third and fourth indexes of the copyRange array are used in place of pasteRange's third and fourth indexes in the getRange() function. For visualization, it looks like this:

ss.getRange(pasteRange[0], pasteRange[1], copyRange[2], copyRange[3]).setValues(ss.getRange(copyRange[0], copyRange[1], copyRange[2], copyRange[3]).getValues())

My Request

I've attempted using objects on my own to improve it, but have had trouble implementing them properly. The convention I've followed to improve my code was from this answer, but have been unsuccessful so far in implementing it in this function specifically. Any assistance is appreciated, thank you.

My Code

//Functions that copy and paste values
function copyPasteValues(){
  var function1 = function(sheet, copyRange, pasteRange){
    let ss = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(sheet)
    if(Array.isArray(copyRange) && Array.isArray(pasteRange)){
      ss.getRange(pasteRange[0], pasteRange[1], copyRange[2], copyRange[3]).setValues(ss.getRange(copyRange[0], copyRange[1], copyRange[2], copyRange[3]).getValues())
    //Should there be a typeof check for string here? (Throwing more errors)
    } else{
      ss.getRange(copyRange).copyTo(ss.getRange(pasteRange))
    } 
  }

  var function2 = function(sourceSheet, targetSheet, copyRange, pasteRange){
    let ss = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(sourceSheet)
    let sa = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(targetSheet)
    if(Array.isArray(copyRange) && Array.isArray(pasteRange)){
      sa.getRange(pasteRange[0], pasteRange[1], copyRange[2], copyRange[3]).setValues(ss.getRange(copyRange[0], copyRange[1], copyRange[2], copyRange[3]).getValues())
    } else{
      ss.getRange(copyRange).copyTo(sa.getRange(pasteRange))
    }
  }

  var function3 = function(sourceSheet, targetSheet, spreadsheetID, copyRange, pasteRange){
    //Pushing data to another spreadsheet and pasting it to another
    try{
      let ss = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(sourceSheet)
      let sa = SpreadsheetApp.openById(spreadsheetID).getSheetByName(targetSheet)
      ss.getRange(copyRange).setValues(sa.getRange(pasteRange).getValues())
    //Pulling data from another spreadsheet and pasting it
    } catch(e){
      if(e instanceof Error){
        let ss = SpreadsheetApp.openById(spreadsheetID).getSheetByName(sourceSheet)
        let sa = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(targetSheet)
        ss.getRange(copyRange).setValues(sa.getRange(pasteRange).getValues())
      } else{
        throw e
      }
    }
  }

  var function4 = function(sourceSheet, targetSheet, spreadsheetID, copyRange, pasteRange){
    //Pushing data from another spreadsheet and pasting it to another
    try{
      if(arrayCheck(copyRange, pasteRange)){
        let ss = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(sourceSheet)
        let sa = SpreadsheetApp.openById(spreadsheetID).getSheetByName(targetSheet)
        sa.getRange(pasteRange[0], pasteRange[1], copyRange[2], copyRange[3]).setValues(ss.getRange(copyRange[0], copyRange[1], copyRange[2], copyRange[3]).getValues())
      }
    //Pulling data from another spreadsheet and pasting it to another
    } catch(e){
      if(e instanceof Error){
        let ss = SpreadsheetApp.openById(spreadsheetID).getSheetByName(sourceSheet)
        let sa = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(targetSheet)
        sa.getRange(pasteRange[0], pasteRange[1], copyRange[2], copyRange[3]).setValues(ss.getRange(copyRange[0], copyRange[1], copyRange[2], copyRange[3]).getValues())
      } else{
        throw e
      }
    }
  }

  try{
    if(arguments.length === 3){
      function1(arguments[0], arguments[1], arguments[2])
    } else if(arguments.length === 4){
      function2(arguments[0], arguments[1], arguments[2], arguments[3])
    } else if(arguments.length === 5){
      if(Array.isArray(arguments[3]) && Array.isArray(arguments[4])){
        function4(arguments[0], arguments[1], arguments[2], arguments[3], arguments[4])
      } else{
        function3(arguments[0], arguments[1], arguments[2], arguments[3], arguments[4])
      }
    }
  } catch(e){
    Logger.log(e)
  }
}

Other Relevant Functions

function arrayCheck(copyRange, pasteRange){
    if(!Array.isArray(copyRange) || !Array.isArray(pasteRange)){
      throw 'Ranges must be in array format'
    } else{
      return true
    }
  }

Updated Version (2/7/2023)

Here is an even more updated one that I feel is more legible and understandable, as the previous updated one was just brute forcing it based on argument length. Most of the checks are going to be used for error handling once I feel comfortable with where the program currently sits

function copyValues(){
  try{
    const options = {
      copy: arguments[0],
      paste: arguments[1],
      source: arguments[2]
    }

    if(arguments.length >= 4){
      options.target = arguments[3]
      if(arguments.length >= 5){
        options.ss_ID = arguments[4]
      }
    }

    if(arrayCheck(options.copy, options.paste)){
      switch(true){
        case arguments.length === 3:
          ss = spreadsheet.getSheetByName(options.source)
          ss.getRange(options.paste[0], options.paste[1], options.copy[2], options.copy[3]).setValues(ss.getRange(options.copy[0], options.copy[1], options.copy[2], options.copy[3]).getValues())
          break;
        
        case arguments.length === 4:
          arrayPaste(options.copy, options.paste, spreadsheet.getSheetByName(options.source), spreadsheet.getSheetByName(options.target))
          break;

        case arguments.length === 5:
          let obj = spreadsheetCheck(options.source, options.target, options.ss_ID)
          arrayPaste(options.copy, options.paste, obj.ss, obj.sa)
          break;
      }
    } else if(stringCheck(options.copy, options.paste)){
      switch(true){
        case arguments.length === 3:
          ss = spreadsheet.getSheetByName(options.source)
          ss.getRange(options.paste).setValues(ss.getRange(options.copy).getValues())
          break;

        case arguments.length === 4:
          stringPaste(options.copy, options.paste, spreadsheet.getSheetByName(options.source), spreadsheet.getSheetByName(options.target))
          break;

        case arguments.length === 5:
          let obj = spreadsheetCheck(options.source, options.target, options.ss_ID)
          stringPaste(options.copy, options.paste, obj.ss, obj.sa)
      }
    }
  
  } catch(e){
    Logger.log(e)
  }

  function spreadsheetCheck(source, target, ss_ID){
    let ss, sa
    switch(true){
      case SpreadsheetApp.openById(ss_ID).getSheetByName(source) === null:
        ss = spreadsheet.getSheetByName(source)
        sa = SpreadsheetApp.openById(ss_ID).getSheetByName(target)
        break;

      case SpreadsheetApp.openById(ss_ID).getSheetByName(target) === null:
        ss = SpreadsheetApp.openById(ss_ID).getSheetByName(source)
        sa = SpreadsheetApp.getActiveSpreadsheet().getSheetByName(target)
        break;
    }
    return {ss, sa}
  }

  function arrayPaste(copy, paste, ss, sa){
    sa.getRange(paste[0], paste[1], copy[2], copy[3]).setValues(ss.getRange(copy[0], copy[1], copy[2], copy[3]).getValues())
  } 

  function stringPaste(copy, paste, ss, sa){
    sa.getRange(paste).setValues(ss.getRange(copy).getValues())
  }   

  function arrayCheck(copy, paste){
    if(Array.isArray(copy) && Array.isArray(paste)){
      return true
    } else{
      return false
    }
  }

  function stringCheck(copy, paste){
    if(String(copy) === copy && String(paste) === paste){
      return true
    } else{
      return false
    }
  }
}

Answer 1

Review

This review pertains to the updated version of the code.

Overall the code is bloated and noisy. Even after a clean up it feels wrong (ambiguous) as the try catch implies there are bugs, but the code gives no clue as what they may be.

Syntax error!

If your code uses strict mode, or is part of a module (both of which should be true) then it will throw a syntax error before it even runs. The reason is that it uses an undeclared variable ss.

Keep it simple.

Every line of code is a line that needs to be maintained and understood. Each line also contains potential bugs. Less code means code that is easier to maintain, understand, and contains fewer bugs.

Code that is not required to complete the task is also known as noise.

• You have a lot of noisy code.

  Example: rather than the 13 lines you have for:

    function arrayCheck(copy, paste){
      if(Array.isArray(copy) && Array.isArray(paste)){
        return true
      } else{
        return false
      }
    }
    function stringCheck(copy, paste){
      if(String(copy) === copy && String(paste) === paste){
        return true
      } else{
        return false
      }
    }
  

  you can use 2 lines that do the same...

    const arrayCheck = (copy, paste) => Array.isArray(copy) && Array.isArray(paste);
    const stringCheck = (copy, paste) => String(copy) === copy && String(paste) === paste;
  

• Naming

  • If you are forced to use names that are very long (good names are one or two words and less than 20 characters long) then use aliases to reduce the noise.

  • Names need only have semantic meaning within the scope that they exist; this scope gives names context to complete the named semantic meaning. There is no need to give every name a uniqueness across the entire code base.

  • Avoid names that have no semantic meaning, e.g. from the code the names ss, sa have no readable meaning, nor conforms to any commonly used abbreviations.

• Your use of switch statements is adding source noise where none is needed. Using if, else if, else statements will reduce the noise. Personally JS switch is very poorly implemented and provides none of the advantages it does in other languages. Don't use them in JS

• You have a try catch that only logs the error. This is pointless code. It also makes it harder to debug as the catch will silently hide bugs if you don't have the console open (or in this case the log is consumed by an unknown Logger).

  • Production code should not output to the console or just push thrown exceptions to a log. (this is very amature).

  • Production code should not use try catch to cover unknown bugs

Use modern JS.

• Using arguments is very old school JS, also arguments has some referencing complexities that can be avoided using modern syntax.

  Use rest parameters when you have an unknown number of arguments. E.g. function copyValues(...args) {

• The creating and assigning properties to options just adds noise. You don't need all the references to options.

  You can assign the named variables using destructuring assignment. eg const [copy, paste, source, target, ss_ID] = args;

• If you return an array from spreadsheetCheck you can then use a spread operator to call paste. eg stringPaste(copy, paste, ...spreadsheetCheck(

• Semicolons are required in JS, unless you know all the rules of Automatic Semicolon Insertion (ASI) then use semicolons.

Clean up

Because your code is ambiguous, I have had to make some guesses, and thus it is not faithful to your code. This also means that it is longer than it can be.

To keep noise levels low I have used common abbreviations for source, copy, check, array, string and target (i.e.src, cpy, chk, arr, str, and trg respectively)

The rewrite is 48% the size of your original 92 lines. What would you quote for 100K lines of code compared to 50K lines of code

"use strict";
function copyValues(...args){
    const arrChk = (cpy, paste) => Array.isArray(cpy) && Array.isArray(paste);
    const strChk = (cpy, paste) => cpy instanceof String && paste instanceof String;
    const sheet = spreadsheet, sheetByName = spreadsheet.getSheetByName.bind(sheet);
    
    const argCount = args.length;
    const [cpy, paste, src, trg, ss_ID] = args;
    
    if (arrChk(cpy, paste)) {
        if (argCount === 3) {
            const ss = sheetByName(src);
            ss.getRange(paste[0], paste[1], cpy[2], cpy[3]).setValues(ss.getRange(cpy[0], cpy[1], cpy[2], cpy[3]).getValues());
        } else if (argCount === 4) {
            arrayPaste(cpy, paste, sheetByName(src), sheetByName(trg));
        } else if (argCount === 5) {
            arrayPaste(cpy, paste, ...chkSheet(src, trg, ss_ID));
        }
    } else if (strChk(cpy, paste)) {
        if (argCount === 3) {
            const ss = sheetByName(src);
            ss.getRange(paste).setValues(ss.getRange(cpy).getValues());
        } else if (argCount === 4) {
            stringPaste(cpy, paste, sheetByName(src), sheetByName(trg));
        } else if (argCount === 5) {
            stringPaste(cpy, paste, ...chkSheet(src, trg, ss_ID));
        }
    }

    function chkSheet(src, trg, ss_ID) {
        const id = SpreadsheetApp.openById(ss_ID), openByName = id.getSheetByName.bind(id);
        
        if (openByName(src) === null) {
            return [sheetByName(src), openByName(trg)];
        }
        if (openByName(trg) === null) {
            return [openByName(src), SpreadsheetApp.getActiveSpreadsheet().getSheetByName(trg)];
        }
        return [];
    }

    function arrayPaste(cpy, paste, ss, sa){
        sa.getRange(paste[0], paste[1], cpy[2], cpy[3]).setValues(ss.getRange(cpy[0], cpy[1], cpy[2], cpy[3]).getValues());
    } 

    function stringPaste(cpy, paste, ss, sa){
        sa.getRange(paste).setValues(ss.getRange(cpy).getValues());
    }   
}