1
\$\begingroup\$

I'd appreciate if someone could cast an eye over my dodgey coding and give me some feedback.

The use / purpose of the sheet/script is to quality grade calls, emails and chats with customers. A form is filled out and then the script takes over.

We feed in a persons name, it grabs the last 9 forms for them. It sorts them in data order.

It then writes data to a blank scorecard, 0-3 Another identical sheet does a vlookup on these scores and changes the 0-3 scores into small traffic light icons.

The script then exports this sheet to a PDF, saves the PDF to a google drive folder (folder name is based on week commencing)

Finally if you have chosen to email the PDF too, it will email it.

This script produces one PDF at a time.

   function getAgentName() {


var ss = SpreadsheetApp.getActiveSpreadsheet();
ss.getRangeByName('Header').clearContent();
ss.getRangeByName('Scores').clearContent();
ss.getRangeByName('Comments').clearContent();
var sheet = ss.getSheetByName("PDF Creator");
var range = sheet.getRange("A2")
var value = range.getValue();


if(value != 0){

getAgentData(value);

} 
else
{
Browser.msgBox("You need to go to the sheet named PDF Creator and put an agent name in cell A2");


}

}





function getAgentData(value){
var ss = SpreadsheetApp.getActiveSpreadsheet();
var sourceSheet = ss.getSheetByName("Form responses 1")
var sourceRange = sourceSheet.getRange(2, 1, sourceSheet.getLastRow(), sourceSheet.getLastColumn());
var sourceValues = sourceRange.getValues();

sourceValues.sort(function(a,b) {
        return b[0]-a[0]
    });


var agentData = [];
var commentsData = [];
for(i=0;i<sourceValues.length;i++)
{
// Defines the data layout for PDF.
var agentName = sourceValues[i][2];
var dateTime = sourceValues[i][3];
var callType = sourceValues[i][7];
var opening = sourceValues[i][8];
var rootCause = sourceValues[i][9];
var rootFix = sourceValues[i][10];
var process = sourceValues[i][11];
var consumer = sourceValues[i][12];
var control = sourceValues[i][13];
var wrapup = sourceValues[i][14];
var dpa = sourceValues[i][15];
var score = sourceValues[i][22];


var comments = sourceValues[i][16];


var agentRow = [dateTime,callType,opening,rootCause,rootFix,process,consumer, control,wrapup,dpa,score];
var commentsRow = [dateTime,comments];




if(agentName == value && agentData.length < 9) {






agentData.push(agentRow)
commentsData.push(commentsRow)
}

}


 agentData.sort(function(a,b) {

        return b[0]-a[0]
    });


 commentsData.sort(function(a,b) {
        return b[0]-a[0]
    });



var destSheet = ss.getSheetByName("AgentPDF");
destSheet.getRange("A1").setValue(value + "'s Quality Score card");
var range = destSheet.getRange(6, 1, agentData.length, agentData[0].length);
range.setValues(agentData);

var commentRange = destSheet.getRange(18, 1, commentsData.length, commentsData[0].length);
commentRange.setValues(commentsData);


SpreadsheetApp.flush();

emailSpreadsheetAsPDF();
}













function emailSpreadsheetAsPDF() {


  var ss = SpreadsheetApp.getActiveSpreadsheet();
  var sheet = ss.getSheetByName("PDF Creator");
  var now = new Date().toString();
  var weekCommencing = sheet.getRange("C1").getValue();
  var coachEmail = sheet.getRange("C4").getValue();
  var coachName = sheet.getRange("A4").getValue();
  var agentName = sheet.getRange("A2").getValue();
  var agentEmail = sheet.getRange("C2").getValue();
  var sendEmail = sheet.getRange("A6").getValue();

  var subject = "Quality Scorecard for - "+agentName + " created on: "+now;  


  var url = ss.getUrl();
  url = url.replace(/edit$/,'');



  var monthNames = [
  "Jan", "Feb", "Mar",
  "Apr", "May", "Jun", "Jul",
  "Aug", "Sep", "Oct",
  "Nov", "Dec"
];


var day = weekCommencing.getDate();
var monthIndex = weekCommencing.getMonth();
var year = weekCommencing.getFullYear();

var clean = day + ' ' + monthNames[monthIndex] + ' ' + year;



 var bodyCoach = "Hello " + coachName + ". Please find attached "+ agentName+ "'s quality scorecard." +" Week commencing "+clean;
  var bodyAgent = "Hello " + agentName + ". Please find attached your  quality scorecard." +" Week commencing "+clean;

  /* Specify PDF export parameters
  // From: https://code.google.com/p/google-apps-script-issues/issues/detail?id=3579
    exportFormat = pdf / csv / xls / xlsx
    gridlines = true / false
    printtitle = true (1) / false (0)
    size = legal / letter/ A4
    fzr (repeat frozen rows) = true / false
    portrait = true (1) / false (0)
    fitw (fit to page width) = true (1) / false (0)
    add gid if to export a particular sheet - 0, 1, 2,..
  */

  var url_ext = 'export?exportFormat=pdf&format=pdf'   // export as pdf
                + '&size=a4'                       // paper size
                + '&portrait=1'                    // orientation, false for landscape
                + '&fitw=true&source=labnol'           // fit to width, false for actual size
                + '&sheetnames=false&printtitle=false' // hide optional headers and footers
                + '&pagenumbers=false&gridlines=false' // hide page numbers and gridlines
                + '&fzr=false'                         // do not repeat row headers (frozen rows) on each page
                + '&gid=1809314965';                             // the sheet's Id

  var token = ScriptApp.getOAuthToken();



    var response = UrlFetchApp.fetch(url + url_ext , {
      headers: {
        'Authorization': 'Bearer ' +  token
      }
    });


    var file = response.getBlob().setName(agentName +"'s Quality Score Card Week Commencing: "+clean+ '.pdf');



  writeFiles(file,clean);

  if (coachEmail != "no email"){
  GmailApp.sendEmail(coachEmail, subject, bodyCoach, {attachments:[file]});
 }

 if(sendEmail == "Yes"){
 GmailApp.sendEmail(agentEmail, subject, bodyAgent, {attachments:[file]});
 }

 ss.getRangeByName('Header').clearContent();
ss.getRangeByName('Scores').clearContent();
ss.getRangeByName('Comments').clearContent();
ss.getRangeByName('AgentName').clearContent();
ss.getRangeByName('Coach').clearContent();
ss.getRangeByName('SendEmail').clearContent();
}

function writeFiles(file,clean) {

    var rootFolder = DriveApp.getFolderById("ID HERE");


  var subFolders = rootFolder.getFolders();  


  var testResult = false;

  while(subFolders.hasNext()== true){

    var folders = subFolders.next();

    if(folders == clean)    {
      testResult = true;
    }



  }


  if(testResult == false){
    rootFolder.createFolder(clean);
  }   



  var destFolder = rootFolder.getFoldersByName(clean).next();

 if(destFolder.getFilesByName(file.getName()).hasNext() == false){
 destFolder.createFile(file);
}

else
{

var warning = Browser.msgBox("Warning", "This PDF already exists. If you wish to overwrite this file, press OK to continue.", Browser.Buttons.OK_CANCEL);


if(warning == "ok"){
destFolder.getFilesByName(file.getName()).next().setTrashed(true);

 destFolder.createFile(file);
 }
 }






}

After completing this script, I wrote another small script to create PDFs in bulk. It gets the list of names to create PDFs for from a cell range within my sheet. Here is the bulk creator that calls on the previous function.

function bulkCreatePDF() {
var ss = SpreadsheetApp.getActiveSpreadsheet();
var sheet = ss.getSheetByName("Bulk PDF Creator");
var range = sheet.getRange(8,1, sheet.getLastRow(), 2)
var values = range.getValues();
var dupTest = sheet.getRange("A6").getValue();
var createCount = sheet.getRange("B4").getValue();
var created = 1;


if(dupTest != 0){
Browser.msgBox("Warning, duplicate entry found", "You have selected the same agent's name twice, please fix and then try again.", Browser.Buttons.OK);
}
else
{
for(j=0;j< values.length;j++){
var agentName = values[j][0];
var emailChoice = values[j][1];

if(agentName != ""){

ss.getRangeByName("AgentName").setValue(agentName);
ss.getRangeByName("SendEmail").setValue(emailChoice);

getAgentName();
ss.toast( "PDF Created for: " +agentName+".",created+" of "+createCount+ " PDF's created");
created = created+1;
SpreadsheetApp.flush();
}



}



}

range.clearContent();
Browser.msgBox("Task completed", "All PDF's have been created, saved to Google Drive and emailed to those you seleceted",Browser.Buttons.OK);

}

in both scripts I've tried to build in warnings, prompts and progress indicators where possible, to minimize any problems. I've also tried to build in checking for file/folder existence before it creates files and folders. This seems to work, but this is my first time using scripting to interface with Google Drive. So perhaps there are better methods.

\$\endgroup\$
4
  • \$\begingroup\$ Is the indentation the same here as in the actual application? \$\endgroup\$
    – Sumurai8
    Commented Jan 17, 2016 at 16:20
  • \$\begingroup\$ @Sumurai8 what do you mean by indentation? \$\endgroup\$
    – Munkey
    Commented Jan 17, 2016 at 16:24
  • \$\begingroup\$ The amount of whitespace before a line of code. en.wikipedia.org/wiki/Indent_style (or in other words: Does the code on this page look like how the code looks like in your favourite editor?) \$\endgroup\$
    – Sumurai8
    Commented Jan 17, 2016 at 16:28
  • \$\begingroup\$ I see, sorry for being dumb, still relatively new at this all. Yes, this is how it's laid out in my script file. I didn't use any JS beautifier or anything like that. \$\endgroup\$
    – Munkey
    Commented Jan 17, 2016 at 16:37

1 Answer 1

2
\$\begingroup\$

Brace yourself. There is a lot that could be improved in your code. I am going to give a review about the basic issues which should improve the code a lot. I feel it does not make sense to dig much deeper in the code while those issues are not resolved.

Styling and readability

Your code is all over the place. Most code is not indented at all, and code that is indented is indented inconsistently.

Your use of whitespace around operators and before and after braces is inconsistent.

You are not closing every statement with a semicolon. While ASI takes care of this in javascript, it is generally better to do this explicitely yourself to prevent obscure bugs where a typo does not give an error that points to the right line, or does not make the code error out at all.

agentData.push(agentRow)

You are not using comments whatsoever, except in something that you seem to have copied.

You are using both single and double quotes for strings. This makes the code more difficult to read, because you can't make assumptions about what string type is used without scanning back in the string. Use one type, and use it consistently. If you need to use that character in the string, escape it (e.g. \').

All this makes the code as-is extremely hard to read and maintain, and possibly error prone. Make sure that your code is consistent in it's layout. To be honest, you should change your coding style and already use the correct indentation when writing the code. This will make reasoning about your code while writing it a lot easier.

Variables

You are using a lot of undescriptive variable names. In the following examples, what is value supposed to contain? An agent's name? It's ID? An ID of some report? Similarly, what is clean supposed to contain? Is it a boolean that somehow signifies if we should clean something? Is it something that is cleaned? What does it mean for a warning to be "ok"? It sounds like it should be a status code instead.

function getAgentData(value){
//or...
function writeFiles(file,clean) {
//or...
warning == "ok"

You are defining variables everywhere. Javascript will automatically move them to the beginning of the context. This is called variable hoisting. Make sure that you define your variables at the beginning of the context to prevent the code from doing something different than what you expect it to do visually.

Other notable things

You are comparing boolean expressions (or in this case a function returning a boolean) with a boolean (e.g. see below). Never do that. If something is a boolean, use it as-is. If you need the opposite result, then use the not operator (!).

subFolders.hasNext()== true

You are dropping all your functions in the global namespace. Create your own namespace, or class, and put your own functions there. Alternatively, wrap everything at least in an IIFE. For readability of the rest, I put more explanation in the spoiler quote below, instead of in the actual text.

With "dropping in the global namespace" I mean that you define your functions on the highest level possible. This means that you can access the functions from any <script> tag. This also means that if at some point you include an other script, and use the same function name, you overwrite the previous function, which will lead to confusing errors. You use a custom namespace when you need to use this code in other future code. Then you use namespace.func() to call your function. A class is... well... a class. Look it up if you are not sure what that is. An IIFE is an immediately invoked function expression. Everything you define inside a function is locally scoped. You cannot access anything in the function from outside the function. You also cannot redefine anything accidentally. This is useful when your code is self-contained. You usually invoke it with global objects you want to use inside this IIFE, and make the parameters of the anonymous function the shorthands you use. If you use jQuery for example, you can have $ as a parameter, and jQuery as the invoked object. This way you do not make assumptions of what $ would be.

//Namespace
var namespace = {
  canHaveIcecream: function( a ) {
    //No you can't
    return false;
  }
}

//IIFE
(function( $ ) {
  //Here be code
}(jQuery));

You have several magic values in your script, for example in the following code. I assume this is some kind of convention. Define these things globally (in your namespace) and use them that way. This way it is clear what things mean, and easier to change if you change the convention.

Cleaner code

This is your code after Code Snippet's Tidy function ran on it. I have not checked the output, but it reads a lot easier than your current code. Again: Aim to write code like this from the beginning. While the function I used is great, you should not rely on it to do this for you.

function getAgentName() {
  var ss = SpreadsheetApp.getActiveSpreadsheet();
  ss.getRangeByName('Header').clearContent();
  ss.getRangeByName('Scores').clearContent();
  ss.getRangeByName('Comments').clearContent();
  var sheet = ss.getSheetByName("PDF Creator");
  var range = sheet.getRange("A2")
  var value = range.getValue();

  if (value != 0) {
    getAgentData(value);
  } else {
    Browser.msgBox("You need to go to the sheet named PDF Creator and put an agent name in cell A2");
  }
}

function getAgentData(value) {
  var ss = SpreadsheetApp.getActiveSpreadsheet();
  var sourceSheet = ss.getSheetByName("Form responses 1")
  var sourceRange = sourceSheet.getRange(2, 1, sourceSheet.getLastRow(), sourceSheet.getLastColumn());
  var sourceValues = sourceRange.getValues();

  sourceValues.sort(function(a, b) {
    return b[0] - a[0]
  });

  var agentData = [];
  var commentsData = [];
  for (i = 0; i < sourceValues.length; i++) {
    // Defines the data layout for PDF.
    var agentName = sourceValues[i][2];
    var dateTime = sourceValues[i][3];
    var callType = sourceValues[i][7];
    var opening = sourceValues[i][8];
    var rootCause = sourceValues[i][9];
    var rootFix = sourceValues[i][10];
    var process = sourceValues[i][11];
    var consumer = sourceValues[i][12];
    var control = sourceValues[i][13];
    var wrapup = sourceValues[i][14];
    var dpa = sourceValues[i][15];
    var score = sourceValues[i][22];
    var comments = sourceValues[i][16];
    var agentRow = [dateTime, callType, opening, rootCause, rootFix, process, consumer, control, wrapup, dpa, score];
    var commentsRow = [dateTime, comments];

    if (agentName == value && agentData.length < 9) {
      agentData.push(agentRow)
      commentsData.push(commentsRow)
    }
  }

  agentData.sort(function(a, b) {
    return b[0] - a[0]
  });


  commentsData.sort(function(a, b) {
    return b[0] - a[0]
  });

  var destSheet = ss.getSheetByName("AgentPDF");
  destSheet.getRange("A1").setValue(value + "'s Quality Score card");
  var range = destSheet.getRange(6, 1, agentData.length, agentData[0].length);
  range.setValues(agentData);

  var commentRange = destSheet.getRange(18, 1, commentsData.length, commentsData[0].length);
  commentRange.setValues(commentsData);


  SpreadsheetApp.flush();

  emailSpreadsheetAsPDF();
}

function emailSpreadsheetAsPDF() {
  var ss = SpreadsheetApp.getActiveSpreadsheet();
  var sheet = ss.getSheetByName("PDF Creator");
  var now = new Date().toString();
  var weekCommencing = sheet.getRange("C1").getValue();
  var coachEmail = sheet.getRange("C4").getValue();
  var coachName = sheet.getRange("A4").getValue();
  var agentName = sheet.getRange("A2").getValue();
  var agentEmail = sheet.getRange("C2").getValue();
  var sendEmail = sheet.getRange("A6").getValue();
  var subject = "Quality Scorecard for - " + agentName + " created on: " + now;
  var url = ss.getUrl();

  url = url.replace(/edit$/, '');
  var monthNames = [
    "Jan", "Feb", "Mar",
    "Apr", "May", "Jun", "Jul",
    "Aug", "Sep", "Oct",
    "Nov", "Dec"
  ];

  var day = weekCommencing.getDate();
  var monthIndex = weekCommencing.getMonth();
  var year = weekCommencing.getFullYear();
  var clean = day + ' ' + monthNames[monthIndex] + ' ' + year;
  var bodyCoach = "Hello " + coachName + ". Please find attached " + agentName + "'s quality scorecard." + " Week commencing " + clean;
  var bodyAgent = "Hello " + agentName + ". Please find attached your  quality scorecard." + " Week commencing " + clean;

  /* Specify PDF export parameters
  // From: https://code.google.com/p/google-apps-script-issues/issues/detail?id=3579
    exportFormat = pdf / csv / xls / xlsx
    gridlines = true / false
    printtitle = true (1) / false (0)
    size = legal / letter/ A4
    fzr (repeat frozen rows) = true / false
    portrait = true (1) / false (0)
    fitw (fit to page width) = true (1) / false (0)
    add gid if to export a particular sheet - 0, 1, 2,..
  */

  var url_ext = 'export?exportFormat=pdf&format=pdf' // export as pdf
    + '&size=a4' // paper size
    + '&portrait=1' // orientation, false for landscape
    + '&fitw=true&source=labnol' // fit to width, false for actual size
    + '&sheetnames=false&printtitle=false' // hide optional headers and footers
    + '&pagenumbers=false&gridlines=false' // hide page numbers and gridlines
    + '&fzr=false' // do not repeat row headers (frozen rows) on each page
    + '&gid=1809314965'; // the sheet's Id

  var token = ScriptApp.getOAuthToken();
  var response = UrlFetchApp.fetch(url + url_ext, {
    headers: {
      'Authorization': 'Bearer ' + token
    }
  });
  var file = response.getBlob().setName(agentName + "'s Quality Score Card Week Commencing: " + clean + '.pdf');

  writeFiles(file, clean);

  if (coachEmail != "no email") {
    GmailApp.sendEmail(coachEmail, subject, bodyCoach, {
      attachments: [file]
    });
  }

  if (sendEmail == "Yes") {
    GmailApp.sendEmail(agentEmail, subject, bodyAgent, {
      attachments: [file]
    });
  }

  ss.getRangeByName('Header').clearContent();
  ss.getRangeByName('Scores').clearContent();
  ss.getRangeByName('Comments').clearContent();
  ss.getRangeByName('AgentName').clearContent();
  ss.getRangeByName('Coach').clearContent();
  ss.getRangeByName('SendEmail').clearContent();
}

function writeFiles(file, clean) {
  var rootFolder = DriveApp.getFolderById("ID HERE");
  var subFolders = rootFolder.getFolders();
  var testResult = false;

  while (subFolders.hasNext() == true) {
    var folders = subFolders.next();

    if (folders == clean) {
      testResult = true;
    }
  }

  if (testResult == false) {
    rootFolder.createFolder(clean);
  }

  var destFolder = rootFolder.getFoldersByName(clean).next();

  if (destFolder.getFilesByName(file.getName()).hasNext() == false) {
    destFolder.createFile(file);
  } else {
    var warning = Browser.msgBox("Warning", "This PDF already exists. If you wish to overwrite this file, press OK to continue.", Browser.Buttons.OK_CANCEL);

    if (warning == "ok") {
      destFolder.getFilesByName(file.getName()).next().setTrashed(true);
      destFolder.createFile(file);
    }
  }
}

\$\endgroup\$
3
  • \$\begingroup\$ @samurai8 Thank you for all the feedback. I take on board the layout comments. I didn't pay any attention to that while writing it. If I post any code here in the future, I will make sure to fully annotate it. I will try to re-write the code including your suggestions and repost. Thanks again for taking the time to read and reply \$\endgroup\$
    – Munkey
    Commented Jan 17, 2016 at 17:35
  • \$\begingroup\$ Just re-reading all your comments. Would you mind if I asked you to expand upon this line you wrote "You are dropping all your functions in the global namespace. Create your own namespace, or class, and put your own functions there. Alternativelly, wrap everything at least in an IIFE." or give a brief example? I'd really appreciate it \$\endgroup\$
    – Munkey
    Commented Jan 17, 2016 at 20:49
  • \$\begingroup\$ I have inserted a spoiler quote with additional information as to not disrupt the general flow of the answer too much. \$\endgroup\$
    – Sumurai8
    Commented Jan 18, 2016 at 9:44

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.