7
\$\begingroup\$

This set of Google Apps Script helper functions are published in this gist, along with a bunch of other utilities. Background info is in this SO question. Since no Google services are used, this is pure JavaScript.

What I hope to get from a code review:

  • General feedback on clarity & maintainability.
  • Is the regex in cellA1ToIndex() too big a drag on performance? What alternatives would be as-or-more functional, with better performance?

cellA1ToIndex( string cellA1, number index )

Convert a cell reference from A1Notation to 0-based indices (for arrays) or 1-based indices (for Spreadsheet Service methods).

colA1ToIndex( colA1, index )

Return a 0-based array index corresponding to a spreadsheet column label, as in A1 notation.

rowA1ToIndex( string rowA1, number index )

Return a 0-based array index corresponding to a spreadsheet row number, as in A1 notation. Almost pointless, really, but maintains symmetry with colA1ToIndex().

/**
 * Convert a cell reference from A1Notation to 0-based indices (for arrays)
 * or 1-based indices (for Spreadsheet Service methods).
 *
 * @param {String}    cellA1   Cell reference to be converted.
 * @param {Number}    index    (optional, default 0) Indicate 0 or 1 indexing
 *
 * @return {object}            {row,col}, both 0-based array indices.
 *
 * @throws                     Error if invalid parameter
 */
function cellA1ToIndex( cellA1, index ) {
  // Ensure index is (default) 0 or 1, no other values accepted.
  index = index || 0;
  index = (index == 0) ? 0 : 1;

  // Use regex match to find column & row references.
  // Must start with letters, end with numbers.
  // This regex still allows induhviduals to provide illegal strings like "AB.#%123"
  var match = cellA1.match(/(^[A-Z]+)|([0-9]+$)/gm);

  if (match.length != 2) throw new Error( "Invalid cell reference" );

  var colA1 = match[0];
  var rowA1 = match[1];

  return { row: rowA1ToIndex( rowA1, index ),
           col: colA1ToIndex( colA1, index ) };
}

/**
 * Return a 0-based array index corresponding to a spreadsheet column
 * label, as in A1 notation.
 *
 * @param {String}    colA1    Column label to be converted.
 *
 * @return {Number}            0-based array index.
 * @param {Number}    index    (optional, default 0) Indicate 0 or 1 indexing
 *
 * @throws                     Error if invalid parameter
 */
function colA1ToIndex( colA1, index ) {
  if (typeof colA1 !== 'string' || colA1.length > 2) 
    throw new Error( "Expected column label." );

  // Ensure index is (default) 0 or 1, no other values accepted.
  index = index || 0;
  index = (index == 0) ? 0 : 1;

  var A = "A".charCodeAt(0);

  var number = colA1.charCodeAt(colA1.length-1) - A;
  if (colA1.length == 2) {
    number += 26 * (colA1.charCodeAt(0) - A + 1);
  }
  return number + index;
}



/**
 * Return a 0-based array index corresponding to a spreadsheet row
 * number, as in A1 notation. Almost pointless, really, but maintains
 * symmetry with colA1ToIndex().
 *
 * @param {Number}    rowA1    Row number to be converted.
 * @param {Number}    index    (optional, default 0) Indicate 0 or 1 indexing
 *
 * @return {Number}            0-based array index.
 */
function rowA1ToIndex( rowA1, index ) {
  // Ensure index is (default) 0 or 1, no other values accepted.
  index = index || 0;
  index = (index == 0) ? 0 : 1;

  return rowA1 - 1 + index;
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ It looks like this code always assigns 1 to index: index == index || 0; index = 0 ? 0 : 1; Perhaps you meant something like index = (index) ? 1 : 0 ? \$\endgroup\$
    – gengkev
    May 8, 2015 at 5:25
  • \$\begingroup\$ @gengkev Good one! Bug there with ==, should be =, assignment. Intent is to first default to zero if not defined, then enforce zero or one. \$\endgroup\$
    – Mogsdad
    May 8, 2015 at 9:25
  • \$\begingroup\$ You could also make that one line index = (index || 0) && 1; \$\endgroup\$
    – Tesseract
    May 8, 2015 at 13:50
  • \$\begingroup\$ @SpiderPig - Ah, yes, relying on coercion of the boolean to a number 0 or 1. Slick. What about readability / supportability? \$\endgroup\$
    – Mogsdad
    May 8, 2015 at 14:01
  • \$\begingroup\$ @gengkev - I've fixed the bug you pointed out, in all three locations. Also in gist. Thanks! \$\endgroup\$
    – Mogsdad
    May 8, 2015 at 14:05

1 Answer 1

4
\$\begingroup\$

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 be done anywhere.

So I'd recommend just translating to 1-based indices, and if you need zero-based indices, well, subtract 1.

I wouldn't worry about using a regex - but I'd probably make it stricter. I imagine that a cell reference isn't that flexible. I imagine the string may contain "$"s but that's about it. Correct me if I'm wrong.

However, cellA1ToIndex() also does too much. It checks things that the other two functions don't. So if you call the other two functions directly, your input isn't checked as strictly. For instance, colA1ToIndex() will accept a string like "%#", and return something wrong. Similarly, rowA1ToIndex() will gladly accept just about anything, and return NaN.

So I'd keep cellA1ToIndex as "dumb" as possible, and make sure the specialized functions are more robust.

Lastly, why a of maximum 2 letters for columns? Why not 3? Or 4?

I'd do something like this:

function cellA1ToIndex(cellA1) {
  var match = cellA1.match(/^\$?([A-Z]+)\$?(\d+)$/);

  if(!match) {
    throw new Error( "Invalid cell reference" );
  }

  return {
    row: rowA1ToIndex(match[2]),
    col: colA1ToIndex(match[1])
  };
}

function colA1ToIndex(colA1) {
  var i, l, chr,
      sum = 0,
      A = "A".charCodeAt(0),
      radix = "Z".charCodeAt(0) - A + 1;

  if(typeof colA1 !== 'string' || !/^[A-Z]+$/.test(colA1)) {
    throw new Error("Expected column label");
  }

  for(i = 0, l = colA1.length ; i < l ; i++) {
    chr = colA1.charCodeAt(i);
    sum = sum * radix + chr - A + 1
  }

  return sum;
}

function rowA1ToIndex(rowA1) {
  var index = parseInt(rowA1, 10)
  if(isNaN(index)) {
    throw new Error("Expected row number");
  }
  return index;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks, many good points! Counter-points & answers to your questions: While subtracting 1 can be done anywhere, that in itself leads to bugs. Six of one, etc. "Cell references not that flexible" - true. As for $, I had intentionally ignored it, but stripping it first would be effective. Pushing the richer checking down into the specialized functions is smart, thanks. Why 2 column letters? Old sheets allowed a max of 256 columns, so 2 was all you could use. (And the whole range wouldn't be legal - but later use of the col number would generate exceptions.) Now 2M cells: limit not needed. \$\endgroup\$
    – Mogsdad
    May 10, 2015 at 11:01

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.