# Tag Info

5

You're in desperate need of a loop. Anytime you start numbering your variables, you should consider whether a loop would be appropriate. I'm sure you did, but you probably got hung up because your lineX assignments vary by just a little. That can be taken care of with a switch statement. I'll start by defining some constants* for the different string values ...

5

UI.gs I left some debugging/development code that should be removed. In particular, this var debugLabel = app.createLabel().setId('DebugLabel').setVisible(false); and this //UiApp.createApplication().createVerticalPanel().setCellHorizontalAlignment(widget, horizontalAlignment); CalendarService.gs I used == when I should have used ===. It's a minor ...

5

A couple of points to make this more readable and maintainable: It has Magic Numbers everywhere: switch ($state) { case 20: Use constants / comment what the values mean. You will probably come back to this and it will be very hard to track down what all the values mean. For example: var AWESOME_STATE_OF_AWESOMENESS = 23; switch ($state) { case ...

4

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" // ...

4

4

I like your argument names for fillInTeam. I think they were good choices. You declare the variable first and only use it here. for (var z = first; z < numbers.length; z++) { You can just set z = i + 1 instead. for (var z = i + 1; z < numbers.length; z++) { Speaking of, use j instead of z. Loop counters traditionally start with i and work their way ...

4

In Google Apps Script, Less code doesn't always mean greater efficiency wrt real-time. According to best practices, focus first on reducing service calls. To save real time, use a single .setValues() with ALL the desired content. "Javascript" operations are very fast compared to calls to Google's services. You do have redundant code because of the ...

4

I see you fixed the index-checking/defaulting bug that gengkev pointed out in the comments. As SpiderPig mentioned the cleanest solution is probably index = index ? 1 : 0. However, I don't know if I'd bother with the index parameter at all. So far, it's led to bugs, duplication across all three functions, and all it really does is add/subtract 1. That can ...

4

You can put all the value content to be set into an array, and set all the values at once. function RD36() { var arrayOfContent,lastrow,outerArray,sheet,ss,startColumn;//Define variables ss = SpreadsheetApp.getActiveSpreadsheet(); sheet = ss.getActiveSheet(); lastrow = sheet.getLastRow();//Find The Last Row With Data In It //Put all the cells ...

4

You can use default parameters to automatically assign a value if none is given. function neat(numToFormat, thousandSep = ".", decimalSep = ",") You should start by checking if the input is valid, instead of doing it at the end. It would also be better to throw an error, since it's not easy to tell from the result if the formatting succeeded or not. if (...

3

I realized I didn't need all those loops so I simplified things to 1 loop: for (rowNum = 2; rowNum <= lastRow; rowNum ++){ //Splitting names var cell = range.getCell(rowNum,colNum).getValues()[0]; var sCell = cell[0].split(", "); sheet.getRange(rowNum,colNum +1,1,sCell.length).setValues([sCell]); var fName = sCell[1]; // first name var ...

3

I flipped your logic a bit in the example below. I only do one API call per calendar and then do all of the processing in the client. It seems a bit faster for me, but I'm not seeing the 60 second processing time that you are. So your results my vary. var one_day = 86400000; //24 * 60 * 60 * 1000 var days = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', '...

3

There is no // try, catch, else in javascript. Instead I would do: var success = false, state = null; try { response = UrlFetchApp.fetch(url); success = true; error = "Up"; state = "Site up: "; } catch(ex) { success = false; error = ex; state = "Site down: "; } insertData(status, url,...

3

Interesting question, there is clearly a lack in the Google Script API to indent cells.. Still, I find the CONCAT approach ugly, if one wants to indent 2 times, the formula will be: =CONCAT(REPT( CHAR( 160 ), 5),"=CONCAT(REPT( CHAR( 160 ), 5),"'Hello World'")") Which is ugly, I would simply pre-calculate the prefix and concatenate in the script and not ...

3

I guess the answer could be useful to someone that begin like me in coding...Anyway the tip here was to create an array first (I named it table) instead of adding objects line by line and then update the sheet in a single operation with setValues. It makes running the code way much faster ! function itemAdd(form) { var ss = SpreadsheetApp....

3

In order to improve the performance of your script you should read cell values from an array that represents the entire range that your are going to scan. This means that instead of repetedly calling var status = sheet.getRange("F"+i).getValue(); //Get the value of the cell. you should get everything in a single call like this one var values = getRange(...

2

The only thing I can suggest is to turn your array-lookups in to object-property lookups. function createAgentLookupTable( agents ) { var table = {}; for (var i = 0 , length = agents.length; i < agents.length; i++) { agents[i].index = i; table[agents[i].toLowerCase()] = agents[i] } return table; } var AgentLookupTable = ...

2

ScriptProperties has been depreciated and should be replaced with the Properties Service. Functions should have verb-noun names. Thus, siteUptimeCheck would be better named checkSiteUptime. 200 is a magic number and should be given a meaningful variable name. (Preferably a constant, but I don't think GAS has constants.)

2

1. Testing: Although your functions don't return a value and they catch any error that is thrown, the functions do log whether the call was successful or not. You could use the Logger to test if they fail like so: Logger.clear(); ui.showSidebar(); var string = Logger.getLog(); if (string.indeOf("(fail)") === -1) throw "test for 'showSidebar' failed"; Note:...

2

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 ...

2

Other than the fact that it's hard to read because your variables are not in Englishref, your code appears solid. Two observations: Some comments would be beneficial, even if only function, parameter and return descriptions. Use of recursion should be clearly explained in comments, for the next poor soul who needs to figure out why they ran out of memory. ...

2

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 ...

2

What you can do is create a data structure (probably an array of arrays) that lists the ranges and the values for each cell/range. Also, don't repeat the code that's common in the then and else parts of the if. Just move them outside the if. You could also assign to a temporary variable instead of repeatedly doing SpreadsheetApp.getActiveSheet(). Then ...

2

You can use .apply() which takes an array of arguments and those arguments will then be passed to the actual function as a series of normal arguments: function funcoesOrcamento( funcao, args ){ if(args) args = args.split("\!|"); else args = []; return FuncoesOrcamentoV2[ funcao ].apply(this, args); } You can read more about .apply() here: ...

2

I didn't run unit tests for this on different search inputs, but it should give an idea of how you can bundle index information into the results for consumption with array class methods. Because you are working in a fairly robust environment (Google Sheets), you can be confident that the search array you are working with has a guaranteed nesting level (which ...

2

Since the activeCell.getRow() is repeated in all ifs, the only way I see of refactoring your code without overcomplicating the script is the following: if(activeCell.getRow() > 2){ if(activeCell.getColumn() == 1){ activeCell.offset(0, 1).setDataValidation(projectTasksAdjItemValidationRule); activeCell.offset(0, 2).setDataValidation(...

2

I don't know enough about the shape of testData to write great code, but you could preprocess and convert testData from an array to an object or Map of key-value pairs. By using the first element of each item in testData as the key, then you can convert your $O(n)$ inner "search" loop into a constant time $O(1)$ key lookup. This would reduce ...

2

Your for () loop is unnecessarily calling SpreadsheetApp.getActiveSpreadsheet() in every single iteration. You can define a varible outside of the loop for chaining: const spreadsheet = SpreadsheetApp.getActiveSpreadsheet() for (let i = 1; i < columnRoom.length; i++) { // something... spreadsheet.insertSheet("No Room") }

Only top voted, non community-wiki answers of a minimum length are eligible