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;
}
1 Answer 1
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;
}
-
\$\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– Mogsdad2015年05月10日 11:01:56 +00:00Commented May 10, 2015 at 11:01
Explore related questions
See similar questions with these tags.
index == index || 0; index = 0 ? 0 : 1;
Perhaps you meant something likeindex = (index) ? 1 : 0
? \$\endgroup\$==
, should be=
, assignment. Intent is to first default to zero if not defined, then enforce zero or one. \$\endgroup\$index = (index || 0) && 1;
\$\endgroup\$