2
\$\begingroup\$

Where I work, we have 3 forms for leavers: HR, IT and rota. Lots of questions are duplicated across all 3 forms.

So I thought, why not fill ONE form out, remove all duplicate questions, and simply split out the answers into formats that match original forms? I combined the form questions, then wrote a script to get the correct data I wanted, in the layout I wanted.

All is working, but as I'm not great at JavaScript, I'd really appreciate any feedback on what I've done and if there are any improvements to be made.

I've annotated my code as best as I can.

function splitForm() {


// Part 1 of the code. Set the variables and read load data.

// Control variables. Can be changed by user.
var done = 22; // In the source sheet, what column number is "Done". If A = 1, B = 2
var when = 23; // In the source sheet, what column number is "Date Moved". If A = 1, B = 2

// Sheet name variables. Can be changed if sheet names change.
var source = "Form responses"; //Name of the sheet with form responses
var hrSheet = "HR"; // Name of the sheet for HR responses
var itSheet = "IT"; // Name of the sheet for IT responses
var wfmSheet = "WFM"; // Name of the sheet for WFM responses


// Temporary array variables. As the script splits data, this is where we'll store the data.
var hrArray = [];
var itArray = [];
var wfmArray = []; 


// Gets the data from the source sheet and loops through it a row at a time, starting at row 2. Row 1 has headers and can be ignored.
var ss = SpreadsheetApp.getActiveSpreadsheet();
var sheet = ss.getSheetByName(source);
var values = sheet.getDataRange().getValues();




// Part 2 of the code. Read and split the data into formats ready for the 3 different sheets.



for (i=1;i<values.length;i++){


// The if statement is a duplicate check. It checks if the "Done" column is empty. If it is, it then gets data.
if(values[i][done-1] == ""){

// The var name gets the employees full name from the split first and second name.
var name = values[i][2]+ " "+ values[i][3];

// Below we replace the email domain with nothing. Then replace the full stop with a space.
// This is to give us the "Name of the person submitted form" value.
// Email addresses are always in firstname.surname@email-domain.co.uk format

var submit1 = values[i][1].replace("@email-domain.co.uk","");
var submit2 = submit1.replace("."," ");


// Pushes unsubmitted data to the HR data Array. This also trims the data so it fits the HR sheet layout.

hrArray.push(values[i].slice(0,13));

// Sorts the layout of the IT data so it matches destination sheet, then pushes it to an array
// I've created an array called itLayout, so the final array's data, matches the destinations.

var itLayout = [ values[i][0],values[i][1],name, values[i][7], values[i][10],values[i][13],values[i][14],values[i][15]];
itArray.push(itLayout);

// Only if Values[i][20] has No will this push data into an array, anything eles we are not interested in.
// Sorts the layout of the wfm data, so it matches the destination sheet, then pushes the data to an array.
// I've created an array called wfmLayout, so the final array's data, matches the destinations.

if(values[i][20] == "No")
{
var wfmLayout = [values[i][0],values[i][1],submit2,name,"","","",values[i][17],values[i][10],values[i][18],values[i][19],values[i][16]];
wfmArray.push(wfmLayout);
}









// This row of data has been pushed to the various Arrays, time to mark the items as done on the source sheet before it checks the next line of data

var date = new Date();
sheet.getRange(i+1, done).setValue("Hell yeah");
sheet.getRange(i+1, when).setValue(date);

}


}
// Part 3 of the code which writes the data into the appropriate sheets.

// Checks how many rows are needed per sheet and stores for later
var hrLength = hrArray.length;
var itLength = itArray.length;
var wfmLength = wfmArray.length;




// This checks first if there is data to write, then sees if there are enough rows. Finally the data is written to the sheet
 if (hrLength != 0){
  var hrTarget = ss.getSheetByName(hrSheet);
  var lastRow = hrTarget.getLastRow();
  var requiredRows = lastRow + hrLength - hrTarget.getMaxRows();
  if (requiredRows > 0) hrTarget.insertRowsAfter(lastRow, requiredRows);
  hrTarget.getRange(lastRow + 1, 1, hrLength, hrArray[0].length).setValues(hrArray);
  }


// This checks first if there is data to write, then sees if there are enough rows. Finally the data is written to the sheet
  if (itLength != 0){
  var itTarget = ss.getSheetByName(itSheet);
  var lastRow1 = itTarget.getLastRow();
  var requiredRows1 = lastRow1 + itLength - itTarget.getMaxRows();
  if (requiredRows1 > 0) itTarget.insertRowsAfter(lastRow1, requiredRows1);
  itTarget.getRange(lastRow1 + 1, 1, itLength, itArray[0].length).setValues(itArray);
  }

// This checks first if there is data to write, then sees if there are enough rows. Finally the data is written to the sheet
  if (wfmLength != 0){
  var wfmTarget = ss.getSheetByName(wfmSheet);
  var lastRow2 = wfmTarget.getLastRow();
  var requiredRows2 = lastRow2 + wfmLength - wfmTarget.getMaxRows();
  if (requiredRows2 > 0) wfmTarget.insertRowsAfter(lastRow2, requiredRows2);
  wfmTarget.getRange(lastRow2 + 1, 1, wfmLength, wfmArray[0].length).setValues(wfmArray);
  }


}
\$\endgroup\$
2
\$\begingroup\$

Line-by-line review

function splitForm() {

Start every function with a comment explaining what it does, no matter how obvious you think the title is. For example, maybe this function isn't really about scoring the form of splits in a gymnastics routine...

I like using jsdoc format in GAS code. It's supported for custom functions, appears in Google's documents, is used for automatic documentation of GAS libraries. Embrace it, get fluent in it... and don't be afraid to use tags that aren't supported by GAS.

For example:

/**
 * Split form responses in a spreadsheet and deliver the relevant
 * parts to each of three role-specific sheets.
 *
 * @param {type}  name   Description of parameter
 *
 * @returns {type}       Description of returns
 * @throws               Description of any thrown errors
 */
function splitForm() {
    ...

// Part 1 of the code. Set the variables and read load data.

// Control variables. Can be changed by user.
var done = 22; // In the source sheet, what column number is "Done". If A = 1, B = 2
var when = 23; // In the source sheet, what column number is "Date Moved". If A = 1, B = 2

Rather than embed these column numbers as constants in code, you could calculate them using the column headers. You'd have to do that after you've read the sheet's values.

Doing this makes it easier to distribute your script to less-technical users, as it frees them to change the spreadsheet layout without needing to make matching changes in the script.

Something like:

var values = sheet.getDataRange().getValues();

// Locate control columns - throw error if missing
var headers = values[0];
var done = headers.indexOf("Done") + 1;
var when = headers.indexOf("Date Moved") + 1;
if (done == 0 || when == 0) {
    throw new Error( 'Must have columns labelled "Done" and "Date Moved".' );
}

Decide whether you need 0-based indexes for use in Javascript arrays, or 1-based indexes for use in Spreadsheet Service methods. This example is for the latter, but I think you really need the former.

// Sheet name variables. Can be changed if sheet names change.
var source = "Form responses"; //Name of the sheet with form responses
var hrSheet = "HR"; // Name of the sheet for HR responses
var itSheet = "IT"; // Name of the sheet for IT responses
var wfmSheet = "WFM"; // Name of the sheet for WFM responses

Similarly, these values present a problem for maintaining & sharing your script. Investigate options that pull the constants out of your code, making them into auto-discovery or configuration data.

  // Temporary array variables. As the script splits data, this is where we'll store the data.
  var hrArray = [];
  var itArray = [];
  var wfmArray = [];

I prefer to keep these sorts of declarations as close to the first use of the variable as possible. It makes no functional difference, but increases readability & understand-ability of code. (IMHO)

  // Gets the data from the source sheet and loops through it a row at a time, starting at row 2. Row 1 has headers and can be ignored.
  var ss = SpreadsheetApp.getActiveSpreadsheet();
  var sheet = ss.getSheetByName(source);
  var values = sheet.getDataRange().getValues();

Ok, although the comment doesn't match the code it's sitting with.

  // Part 2 of the code. Read and split the data into formats ready for the 3 different sheets.

  for (i = 1; i < values.length; i++) {

    // The if statement is a duplicate check. It checks if the "Done" column is empty. If it is, it then gets data.
    if (values[i][done - 1] == "") {

Here you're converting between 0- and 1- based indexes, see previous comment. Eliminating +/- 1 will help avoid these simple, yet hard-to-find bugs.

      // The var name gets the employees full name from the split first and second name.
      var name = values[i][2] + " " + values[i][3];

More magic column numbers! If you decide not to auto-discover these, that's fine, but at least use meaningful name, like values[i][colFirstName].

      // Below we replace the email domain with nothing. Then replace the full stop with a space.
      // This is to give us the "Name of the person submitted form" value.
      // Email addresses are always in firstname.surname@email-domain.co.uk format

      var submit1 = values[i][3].replace("@email-domain.co.uk", "");
      var submit2 = submit1.replace(".", " ");

Once again, you're relying on a constant that limits portability, a string this time. See How can I extract the user name from an email address using javascript?

      // Pushes unsubmitted data to the HR data Array. This also trims the data so it fits the HR sheet layout.

      hrArray.push(values[i].slice(0, 13));

      // Sorts the layout of the IT data so it matches destination sheet, then pushes it to an array
      // I've created an array called itLayout, so the final array's data, matches the destinations.

      var itLayout = [values[i][0], values[i][4], name, values[i][7], values[i][10], values[i][13], values[i][14], values[i][15]];
      itArray.push(itLayout);

      // Only if Values[i][20] has No will this push data into an array, anything eles we are not interested in.
      // Sorts the layout of the wfm data, so it matches the destination sheet, then pushes the data to an array.
      // I've created an array called wfmLayout, so the final array's data, matches the destinations.

      if (values[i][20] == "No") {
        var wfmLayout = [values[i][0], values[i][5], submit2, name, "", "", "", values[i][17], values[i][10], values[i][18], values[i][19], values[i][16]];
        wfmArray.push(wfmLayout);
      }

This whole block is full of magic numbers, so at a minimum that needs to be addressed.

This repeats a cell-by-cell copy three times, with slight differences. When you see repeated logic, you should ask yourself if there is some way to refactor it to just ONE piece of code (often in a separate function) by focusing on the requirements that are common across all.

      // This row of data has been pushed to the various Arrays, time to mark the items as done on the source sheet before it checks the next line of data

      var date = new Date();
      sheet.getRange(i + 1, done).setValue("Hell yeah");
      sheet.getRange(i + 1, when).setValue(date);

    }

  }

Question: What happens if your script gets interrupted right now? You'll have marked rows as done, yet not written them out to their destination sheets. How do you recover from that?

Efficiency would be improved if you held off marking individual rows at this time, as well. That implies a solution that marks rows as done all at once, AFTER all destination updates are done.

Would be helpful to explain the i + 1 in this case, e.g. in code with row = i+1; // 1-based row numbering.

  // Part 3 of the code which writes the data into the appropriate sheets.

  // Checks how many rows are needed per sheet and stores for later
  var hrLength = hrArray.length;
  var itLength = itArray.length;
  var wfmLength = wfmArray.length;

  // This checks first if there is data to write, then sees if there are enough rows. Finally the data is written to the sheet
  if (hrLength != 0) {
    var hrTarget = ss.getSheetByName(hrSheet);
    var lastRow = hrTarget.getLastRow();
    var requiredRows = lastRow + hrLength - hrTarget.getMaxRows();
    if (requiredRows > 0) hrTarget.insertRowsAfter(lastRow, requiredRows);
    hrTarget.getRange(lastRow + 1, 1, hrLength, hrArray[0].length).setValues(hrArray);
  }

  // This checks first if there is data to write, then sees if there are enough rows. Finally the data is written to the sheet
  if (itLength != 0) {
    var itTarget = ss.getSheetByName(itSheet);
    var lastRow1 = itTarget.getLastRow();
    var requiredRows1 = lastRow1 + itLength - itTarget.getMaxRows();
    if (requiredRows1 > 0) itTarget.insertRowsAfter(lastRow1, requiredRows1);
    itTarget.getRange(lastRow1 + 1, 1, itLength, itArray[0].length).setValues(itArray);
  }

  // This checks first if there is data to write, then sees if there are enough rows. Finally the data is written to the sheet
  if (wfmLength != 0) {
    var wfmTarget = ss.getSheetByName(wfmSheet);
    var lastRow2 = wfmTarget.getLastRow();
    var requiredRows2 = lastRow2 + wfmLength - wfmTarget.getMaxRows();
    if (requiredRows2 > 0) wfmTarget.insertRowsAfter(lastRow2, requiredRows2);
    wfmTarget.getRange(lastRow2 + 1, 1, wfmLength, wfmArray[0].length).setValues(wfmArray);
  }

These three nearly-identical pieces would be easily refactored into a parameterized function.

Are you sure you need to worry about adding rows to the sheets? Isn't that taken care of automatically by setValues()?

General comments

  • Consistently well-formatted code is easier to read & maintain. Make sure you've enabled "Indenting" on the GAS editor. Run your existing code through a pretty-printer to tame it. For example, JSBeautifier with the settings below will produce the same results as the editor:

    jsbeautifier settings

    Select all - copy - paste - beautify - copy - paste back - WHAM!

  • Generally speaking, smaller pieces of code are easier to understand and maintain than larger ones. Your single, large function could be separated into a few smaller ones, especially where there is an opportunity to support all three target sheets with one block of code.

  • This looks like it's a function that you intend to execute manually, possibly through a custom menu. Have you considered instead using a form submission trigger function to process forms as they arrive? It would require significantly simpler code, as all the row-looping would be eliminated. You could still support a manual function by simulating form submissions using the technique from How can I test a trigger function in GAS?

\$\endgroup\$
  • \$\begingroup\$ @Jamal - your edit has removed the differentiation between original code (left aligned) and my suggested changes (indented). If you can suggest some other way to express those differences, I'm all ears. \$\endgroup\$ – Mogsdad May 7 '15 at 18:01
  • \$\begingroup\$ Absolutely: blockquotes. You can highlight some text and click the blockquote button, or prepend a line with a ">" character. \$\endgroup\$ – Jamal May 7 '15 at 18:05
  • \$\begingroup\$ Cool! I had a vestigial memory of that reformatting code into continuous text. \$\endgroup\$ – Mogsdad May 7 '15 at 18:08
  • \$\begingroup\$ @Mogsdad thank you for a concise post with lots of feedback. Really helpful. I do want to move away from using array[i][3] and start using column headers. I like the idea of changing the write section into a single function, will have a play with that. Not sure how I'd improve on the "Magic numbers" bit that pushes data to the array, in the correct format. Perhaps only with column names, not numbers. But thanks for all the feedback, it's all very useful on the path of learning. \$\endgroup\$ – Munkey May 7 '15 at 19:04
  • 1
    \$\begingroup\$ That was fun - my first CR answer. If you have trouble conceptualizing or implementing any of the suggestions, just post on StackOverflow - I'm usually lurking there. \$\endgroup\$ – Mogsdad May 7 '15 at 19:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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