Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Can someone have a look at my script and maybe find something which can be optimized, as there shall be later 100 of it running.

First of all this script shall be triggered once per day to receive data from a website into a sheet called "getdata". Some fields of "getdata" are transferred to "blindcopy" in a certain order. After that the found data shall be put into a sheet called "monitoring" by deleting first row of this sheet and filling the data from "blindcopy" to the row after the last row of "monitoring". These data is referenced to other sheets and finally to the sheet "Output". In the sheet "Output" some cells are colored depending on their relative difference to other cells (first set color of all text in E7:E10 red and if any cell in this range is bigger than its corresponding cell in column D then its text color is set to green).

To not overflow the importXML command to the website on the beginning of the script the given URL is set to a cell and from there it is deleted at the end of the script again. This is because I do not want to call the importxml command everytime when I or others view the "Output" sheet, where none of the data is displayed coming directly via importxml, but it would be called, even if the data will not be displayed because the displayed data on "Output" is referencing to sheets where the importxml commands are executed (I hope I could explain this properly).

OK, the script is working fine no problems with that. But as I want to use this script in 100 different spreadsheets triggered on different times of the day once every day I think I might run into "exceeding CPU time" error. And before that I want to make sure that I have stripped the script down to the minimum (like I changed the getValue to getValue*s*).

It would be fine if someone could have a look into it, as there might be something to improve.

function myFunction() {
  // Open spreadsheet

  var docid = "some_long_strange_id_following_the_key=_in_the_URL_of_the_spreadsheet"
  var doc = SpreadsheetApp.openById(docid);
  var getsheet = doc.getSheetByName("getdata");
  // Get url from C1 and insert to multiple-referenced cell C4
  var cellreference = getsheet.getRange("C1");
  var url = cellreference.getValue();
  var cellreference = getsheet.getRange("C4");
  cellreference.setValue(url);
  // Transferring data from "blindcopy" to "monitoring"
  var getsheet = doc.getSheetByName("monitoring");
  getsheet.deleteRow(1)
  var sheetFrom = doc.getSheetByName("blindcopy");
  var sheetTo = doc.getSheetByName("monitoring");  
  var lastRow = sheetFrom.getLastRow();
  var cell = sheetFrom.getRange('A1').offset(lastRow, 0);
  var rowTo = sheetTo.getLastRow() + 1;
  for( var i = 1; i < 33; i++) {
     sheetTo.getRange(rowTo, i).setValues( sheetFrom.getRange(lastRow, i).getValues());
  }
  // Compare values1 to values2 for the coloring
  var s = doc.getSheetByName('Output');
  var values1Rule1 = s.getRange('E7:H10').getValues();
  var values2Rule1 = s.getRange('D7:D10').getValues();
  var matchList = [];
  for (var row in values1Rule1) {
    for (var col in values1Rule1) {
      for (var row2 in values2Rule1) {
        if (values1Rule1[row][col] >= values2Rule1[row][0]) 
          matchList.push(s.getRange('E7').offset(row, col, 1, 1).getA1Notation()); }}}
  // Turn values1 red first 
  s.getRange('E7:H10').setFontColor('RED');
  // Then only turn matches green
  for (var m in matchList)
    s.getRange(matchList[m]).setFontColor('GREEN');
  // delete C4 in "getdata" to avoid too much traffic to URL by importxml due to many lookups of "Output"
  var getsheet = doc.getSheetByName("getdata");
  var blubb = "blockxmlimport";
  var cellreference = getsheet.getRange("C4");
  cellreference.setValue(blubb);
}
share|improve this question

migrated from stackoverflow.com Dec 11 '12 at 11:12

This question came from our site for professional and enthusiast programmers.

    
How long does it take to run now and does it take up lots of resouces? –  DFord Dec 11 '12 at 12:52
    
How can I see how much resources it takes? –  Tassilo Dec 11 '12 at 22:16
    
Script runs round about 10 seconds –  Tassilo Dec 11 '12 at 22:57
    
for (var row in values1Rule1) { for (var col in values1Rule1) { for (var row2 in values2Rule1) { looks fishy –  megawac Jan 21 at 2:35

1 Answer 1

From my research, the slowest part of your script is by far SpreadsheetApp.openById(docid), there is nothing much you can do about that. I am not sure how this script will behave once you have a 100 instances of it running, but my prediction is that it will be bad..

Furthermore from a once over:

  • You never use cell in var cell = sheetFrom.getRange('A1').offset(lastRow, 0);
  • You never use row2 in for (var row2 in values2Rule1) {, I dont think you need it
  • You only have to declare cellreference and getsheet once
  • You should consider lowerCamelCasing: getsheet -> getSheet, cellreference -> cellRefeference
  • getSheet is a terrible name, you should name the variable of the sheet:

    var monitoringSheet = doc.getSheetByName("monitoring");
    
  • You call doc.getSheetByName("monitoring") twice, you could simply:

    var toSheet = monitoringSheet;  
    
  • Instead of reading one cell value and then assigning it to another, you could simply use copyTo:

    var dataSheet = doc.getSheetByName("getdata");
    // Get url from C1 and insert to multiple-referenced cell C4
    var c1 = getsheet.getRange("C1"),
    var c4 = getsheet.getRange("C4");
    c1.copyTo(c4);
    
  • At the end you already had a reference to C4, so you could simply:

    c4.setValue( 'blockxmlimport' );
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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