patternjavascriptMinor
A1Notation conversion to row & column index
Viewed 0 times
conversioncolumna1notationindexrow
Problem
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:
Convert a cell reference from
Return a 0-based array index corresponding to a spreadsheet column label, as in A1 notation.
Return a 0-based array index corresponding to a spreadsheet row number, as in A1 notation. Almost pointless, really, but maintains symmetry with
```
/**
* 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 )
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 )
Solution
I see you fixed the
However, I don't know if I'd bother with the
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
However,
So I'd keep
Lastly, why a of maximum 2 letters for columns? Why not 3? Or 4?
I'd do something like this:
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;
}Code Snippets
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;
}Context
StackExchange Code Review Q#90112, answer score: 4
Revisions (0)
No revisions yet.