2
\$\begingroup\$

I've made this function to map alphabetic coordinates to their corresponding ordinal number.

var exec = document.querySelector('#exec');
		   
// Maps alphabetic characters to their index
//   within the alphabet. 
// --------------------------------------------  
// @param { string || array } chars - String
//  consisting of a single alphabetic character  
//  or an array which elements are single 
//  alphabetic characters.
// @throws Error in case of invalid
//  parameter ( == not a single alphabetic
//  character ).
// --------------------------------------------  
// @returns { array } with numbers. The indexes
//  of the characters within the alphabet.
// 
// ------- Usage examples ----------
// getIndexInAlphabet('i')); // Result : [9]
// getIndexInAlphabet(['a', 'B', 'c'])[1] // 2
// getIndexInAlphabet(['a', 'G', 'z'])); // [1, 7, 26]
		   
function getIndexInAlphabet( chars ) {
  var alphabet = [ 'a', 'b', 'c', 'd', 'e',
                   'f', 'g', 'h', 'i', 'j',
                   'k', 'l', 'm', 'n', 'o',
                   'p', 'q', 'r', 's', 't',
                   'u', 'v', 'w', 'x', 'y',
                   'z' ];

  if (!Array.isArray(chars)) {
    if (typeof chars === 'string') {
      let tmp = [];
      tmp.push(chars);

      chars = tmp; 
    } else {
      throw new Error(
        'Parameter invalid because not of type string.');
    }
  }

  chars.forEach(function(item, i) {
    if (typeof item !== 'string') {
      throw new Error('Element ' + i +
                      ' invalid because not of type string.');
    }
  });

  return chars.map(function(char, i) {
    var ret = alphabet.indexOf(char.toLowerCase()) + 1;

    if (ret === 0) {
      throw new Error('Element ' + i + ' invalid because' +
                      ' not an alphabetic character.');
    }

    return ret;
  });
}

// -- From here on : Just testing ...
exec.addEventListener('click', function() {
  try {
    console.log(getIndexInAlphabet(['a', 'B', 'c'])[1]) 
    console.log(getIndexInAlphabet('i')); 
    console.log(getIndexInAlphabet(['a', 'G', 'z'])); 

    var charStr = ['a', 'b', 'c', 'd', 'e', 'f'];
    var indexes = getIndexInAlphabet(charStr);			   
    var charMap = {};

    charStr.forEach(function(char, i) {
      charMap[char] = indexes[i];
    });

    console.log(charMap.f);
  } catch (e) {
    console.log(e.message);
    console.log(e.stack);
  }
});
.wrap {
  width: 800px;
  margin: 50px auto;
}
<div class="wrap">
  <div class="buttons">
    <a href="#" id="exec" class="slideshow-nav">Exec!</a>
  </div>
</div>

\$\endgroup\$
1
\$\begingroup\$

Changed the way you wrote the alphabet even though it's fine tuning at this point, I prefer one liner when they're still clean and easy to understand.

I'd rather use a more declarative way of writing the process.

Fiddle : https://jsfiddle.net/y6zx2rht/1/

function getIndexInAlphabet(chars) {
  var alphabet = 'abcdefghijklmnopqrstuvwxyz'.split(''),
    validateItemIsString = function(item) {
      if (typeof item !== 'string') {
        throw new Error('Element ' + i +
          ' invalid because not of type string.');
      }
    },
    validateCharsAreStrings = function(chars) {
      chars.forEach(function(item, i) {
        validateItemIsString(item);
      });
    },
    getSanitizedChars = function(chars) {
      if (!Array.isArray(chars)) {
          chars = [chars];
      }

      validateCharsAreStrings(chars);

      return chars;
    },
    getIndexInAlphabet = function(char, i) {
      var ret = alphabet.indexOf(char.toLowerCase()) + 1;

      if (ret === 0) {
        throw new Error('Element ' + i + ' invalid because' + ' not an alphabetic character.');
      }

      return ret;
    };

  return getSanitizedChars(chars).map(function(char, i) {
    return getIndexInAlphabet(char, i);
  });
}

This lets you prepare the whole process within small isolated steps. Also, you won't have to focus so much on the "how I want this", but on the "I want this". This is pretty clear in this portion of code :

  if (!Array.isArray(chars)) {
    if (typeof chars === 'string') {
      let tmp = [];
      tmp.push(chars);

      chars = tmp; 
    } //...
  }

What you want here is : "Return an array containing chars". What you do instead is : "Create an empty array. Push chars into it. Return the array". Then it becomes :

if (!Array.isArray(chars)) {
    if (typeof chars === 'string') {
        return [chars];
    }
}

Also, as 200_success said, validating that char is a string and that it is not an array is redundant, so it can be further simplified to :

if (!Array.isArray(chars)) {
    return [chars];
}
\$\endgroup\$
  • \$\begingroup\$ if (!Array.isArray(chars)) { if (typeof chars === 'string') { … }} is redundant. You only need the inner test. \$\endgroup\$ – 200_success Jun 3 '16 at 8:57
  • \$\begingroup\$ I totally agree with you, I'm modifying my answer to fit yours. I couldn't comment neither thumb up your answer :/ \$\endgroup\$ – Steve Chamaillard Jun 3 '16 at 8:58
2
\$\begingroup\$

The alphabet would be simpler if it were written as a string, and it should be const. I'd also name it ALPHABET to emphasize that it is constant.

Using three statements to stuff a string into an array is too cumbersome. In my opinion, there is not much value in verifying that a non-array parameter is a string, since you'll validate each element of the array anyway. Does it really matter that the error message is slightly different?

The validation of each array element can be rolled into the map callback. In keeping with JavaScript's duck-typing philosophy, I would check that it has a .toLowerCase() method, rather than that it is of type string.

I find it weird that the function returns an array even if the input is a single character. I suggest renaming the function to getIndexesInAlphabet to make it clear that it returns an array.

function getIndexesInAlphabet(chars) {
  const ALPHABET = 'abcdefghijklmnopqrstuvwxyz'.split('');

  if (!Array.isArray(chars)) {
    chars = [chars];
  }

  return chars.map(function(char, i) {
    if (!char.toLowerCase) {
      throw new Error('Element ' + i +
                      ' invalid because it is not a string.');
    }

    var ret = ALPHABET.indexOf(char.toLowerCase()) + 1;

    if (ret === 0) {
      throw new Error('Element ' + i + ' invalid because' +
                      ' not an alphabetic character.');
    }

    return ret;
  });
}

Also consider whether you really want to accept arrays at all. Unless you plan to be able to support spreadsheet-style coordinates (e.g. AA to represent column 27), every coordinate will be a single character. You could rationalize the interface by accepting only strings:

// Maps alphabetic characters to their 1-based index within the alphabet. 
// --------------------------------------------  
// @param { string } chars - String
//  consisting of a one or more alphabetic characters.
// @throws Error if the input contains a non-alphabetic character.
// --------------------------------------------  
// @returns { array } with 1-based indexes. The indexes
//  of the characters within the alphabet.
// 
// ------- Usage examples ----------
// getIndexesInAlphabet('i'); // Result : [9]
// getIndexesInAlphabet('aBc')[1] // 2
// getIndexesInAlphabet('aGz'); // [1, 7, 26]      
function getIndexesInAlphabet(chars) {
  const ALPHABET = 'abcdefghijklmnopqrstuvwxyz';

  return chars.split('').map(function(char, i) {
    var index = ALPHABET.indexOf(char.toLowerCase());
    if (index < 0) {
      throw new Error(char + 'is not a valid alphabetic character.');
    }
    return index + 1;
  });
}

If what is passed is not a string, it would be standard JavaScript programming practice not to bother trying to make sense of it or throwing a custom error.

\$\endgroup\$
  • 1
    \$\begingroup\$ If you use const, you should also use let and template strings (assuming the environment you're targeting supports ES6). \$\endgroup\$ – gcampbell Jun 3 '16 at 9:43
2
\$\begingroup\$

You could also use recursion:

function getIndexInAlphabet (char) {
  const alphabet = 'abcdefghijklmnopqrstuvwxyz'

  if (Array.isArray(char)) {
    return chars.map(char => getIndexInAlphabet(char)[0]) // <-- call itself if the input's an array
  } else if (!char.toLowerCase) {
    throw new TypeError(`${char} is not string-like`)
  } else if (char.length !== 1) {
    throw new Error(`${char} is not a single character`)
  }

  const num = alphabet.indexOf(char.toLowerCase()) + 1

  if (num === 0) throw new Error(`${char} is not an alphabetic character`)

  return [num]
}

This returns an array with one element if the input is a single-character string, and if the input is an array it calls itself for each character in the array.

Duck-typing inspired by @200_success.

\$\endgroup\$

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.