0
\$\begingroup\$

I've just created a script in Google Spreadsheet in order to archive some things. I have data from four countries. If something is marked done, then that line could be archived into that country's archive.

My code is pretty ugly. Is there a clearer way to do this?

function CroatianArchive() 
{  
  var source_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('original');
  var target_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('croatian_backup');
  var lastRow = source_sheet.getLastRow();
  var source_range = source_sheet.getDataRange();
  var target_range = target_sheet.getDataRange();  
  var rowsToBeDeleted = [];

  var i = 2;
  while (i <= lastRow) {
  if (source_sheet.getRange("A"+i).getValue() == "CRO" && source_sheet.getRange("M"+i).getValue() == "DONE" ) {
    var office = source_sheet.getRange("A"+i).getValue();
    var title = source_sheet.getRange("B"+i).getValue();
    var imdbId = source_sheet.getRange("C"+i).getValue();
    var channel = source_sheet.getRange("D"+i).getValue();
    var type = source_sheet.getRange("E"+i).getValue();
    var added = source_sheet.getRange("F"+i).getValue();
    var deadline = source_sheet.getRange("G"+i).getValue();
    var airing = source_sheet.getRange("H"+i).getValue();
    var link = source_sheet.getRange("I"+i).getValue();
    var picture = source_sheet.getRange("J"+i).getValue();
    var comment = source_sheet.getRange("K"+i).getValue();
    var portId = source_sheet.getRange("L"+i).getValue();
    var status = source_sheet.getRange("M"+i).getValue();
    var data = [office,title,imdbId,channel,type,added,deadline,airing,link,picture,comment,portId,status];
    target_sheet.appendRow(data);    
    rowsToBeDeleted.push(i);
    i++;
    } else {
      i++;
      }
    }  
    rowsToBeDeleted.reverse();
    for (var j = 0; j < rowsToBeDeleted.length; j++) 
    {
        source_sheet.deleteRow(rowsToBeDeleted[j]);
    }   
    SpreadsheetApp.getActiveSpreadsheet().toast('Archivation finished.', 'Status');
  }
function SerbianArchive() 
{  
  var source_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('original');
  var target_sheet = SpreadsheetApp.getActiveSpreadsheet().getSheetByName('serbian_backup');
  var lastRow = source_sheet.getLastRow();
  var source_range = source_sheet.getDataRange();
  var target_range = target_sheet.getDataRange();  
  var rowsToBeDeleted = [];

  var i = 2;
  while (i <= lastRow) {
  if (source_sheet.getRange("A"+i).getValue() == "SER" && source_sheet.getRange("M"+i).getValue() == "DONE" ) {
    var office = source_sheet.getRange("A"+i).getValue();
    var title = source_sheet.getRange("B"+i).getValue();
    var imdbId = source_sheet.getRange("C"+i).getValue();
    var channel = source_sheet.getRange("D"+i).getValue();
    var type = source_sheet.getRange("E"+i).getValue();
    var added = source_sheet.getRange("F"+i).getValue();
    var deadline = source_sheet.getRange("G"+i).getValue();
    var airing = source_sheet.getRange("H"+i).getValue();
    var link = source_sheet.getRange("I"+i).getValue();
    var picture = source_sheet.getRange("J"+i).getValue();
    var comment = source_sheet.getRange("K"+i).getValue();
    var portId = source_sheet.getRange("L"+i).getValue();
    var status = source_sheet.getRange("M"+i).getValue();
    var data = [office,title,imdbId,channel,type,added,deadline,airing,link,picture,comment,portId,status];
    target_sheet.appendRow(data);    
    rowsToBeDeleted.push(i);
    i++;
    } else {
      i++;
      }
    }  
    rowsToBeDeleted.reverse();
    for (var j = 0; j < rowsToBeDeleted.length; j++) 
    {
        source_sheet.deleteRow(rowsToBeDeleted[j]);
    }   
    SpreadsheetApp.getActiveSpreadsheet().toast('Archivation finished.', 'Status');
  }
\$\endgroup\$
4
\$\begingroup\$

You have a lot of repeated code when creating the data array. Note that the names of each cell value are ultimately irrelevant. That part could also be expressed as:

var columns = ["A" // office
              ,"B" // title
              ,"C" // imdbId
              ,"D" // channel
              ,"E" // type
              ,"F" // added
              ,"G" // deadline
              ,"H" // airing
              ,"I" // link
              ,"J" // picture
              ,"K" // comment
              ,"L" // portId
              ,"M" // status
              ];
var data = columns.map(function (col) {
  return source_sheet.getRange(col + i).getValue();
});

Less redundant code, same level of documentation.

Code like

if (...) {
  ...;
  i++;
} else {
  i++;
}

is usually better written as:

if (...) {
  ...;
}
i++;

However, we now have

var i = 2;
while (i <= lastRow) {
  ...;
  i++;
}

which is a more complicated formulation of

for (var i = 2; i <= lastRow; i++) {
  ...;
}

Similarly,

rowsToBeDeleted.reverse();
for (var j = 0; j < rowsToBeDeleted.length; j++) 
{
    source_sheet.deleteRow(rowsToBeDeleted[j]);
}

is the same as:

for (var i = rowsToBeDeleted.length - 1; i >= 0; i--) {
  source_sheet.deleteRow(rowsToBeDeleted[i]);
}

If I see that correctly, your two function only differ in the strings croatian_backup vs. serbian_backup and CRO vs. SER. Instead of copy-pasting that code, take those strings from the function parameters. Now we have a general archive function:

function archive(outputSheet, languageCode) { ... }

If you need two seperate funtions that don't take any arguments, we can use the currying technique (also known as partial application) to pre-fill the arguments:

function CroatianArchive() {
  return archive("croatian_backup", "CRO");
}

Some of your names use underscores, other capitalization to separate words:

source_range
lastRow

You should settle for one style – Javascript tends to prefer capitalization rather than underscores.

\$\endgroup\$
  • \$\begingroup\$ I wouldn't call it currying or partial application since there's nothing partial about it. Your example doesn't curry a function; it just calls one. \$\endgroup\$ – Flambino Nov 4 '13 at 17:15
  • \$\begingroup\$ @Flambino You are technically correct, and I lied in my answer in order to pin a name on this specialization technique. I picked “currying” because I couldn't think of anything better, and because it is effectively a similar abstraction. Because JavaScript has side effects, I transformed archive :: (String, String) → () into CroatianArchive :: () → (), which can be seen as the end step of CroatianArchive' :: String → String → () → (). The word “priming” might be a better alternative (which is what Perl6 calls this, and which I emulated). \$\endgroup\$ – amon Nov 4 '13 at 18:01
  • \$\begingroup\$ Priming's a good name for it. To be honest, I never thought of what one might call it; I'd probably just call it a "shortcut" or something :) But it's funny, I reasoned the same as you when I wrote my comment. If currying means reducing the arity of a function, then reducing the arity to zero would be "currying". But then every function invocation would fall in that category and it'd sorta lose its meaning \$\endgroup\$ – Flambino Nov 4 '13 at 21:53

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.