6
\$\begingroup\$

I am working through some of the exercises at Coberbyte. The instructions for this exercise are to replace every letter in a string with the letter following it in the alphabet (i.e., c becomes d, z becomes a), then capitalize every vowel in this new string (a, e, i, o, u) and finally return this modified string.

  1. Is there a better way to advance characters alphabetically than converting to ASCII, adding one, and converting back?

  2. Is there a way to do this with one loop?

  3. In general, are there ways to shorten/improve the code?

var txt = prompt("Please enter some text: ");
var newTxt = "";
var newTxt2= "";

for (var i = 0; i<txt.length; i++){
  if (txt.charAt(i).contains(" ")){
    newTxt = newTxt.concat(" ");//checks for spaces
  }else if (txt.charAt(i) === "z"){
    newTxt = newTxt.concat("a");//checks for "z" and sets to "a"
  }else{
    newTxt = newTxt.concat(String.fromCharCode(txt.charAt(i).charCodeAt(0)+1));
  }//converts letter to the next letter in the alphabet
}

// checks for vowels and sets to upper case
for (var i=0; i < newTxt.length; i++){ 
  switch(newTxt.charAt(i)){
    case "a":
    case "e":
    case "i":
    case "o":
    case "u":
    newTxt2 = newTxt2 + (newTxt.charAt(i).toUpperCase());    break;
    default:
    newTxt2 = newTxt2 + newTxt.charAt(i);
    break; 
 }
}

console.log(newTxt2);
\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

First, I'll note that str.charAt(n).charCodeAt(0) is better written as str.charCodeAt(n).

Strings are immutable, so it's better to work on array elements than to build them by character-by-character concatenation.

function transform(text) {
    var s = text.split('');
    for (var i = 0; i < s.length; i++) {
        // Caesar cipher
        switch(s[i]) {
          case ' ':
            break;
          case 'z':
            s[i] = 'a';
            break;
          case 'Z':     // One case you forgot to handle
            s[i] = 'A';
            break;
          default:
            s[i] = String.fromCharCode(1 + s[i].charCodeAt(0));
        }

        // Upper-case vowels
        switch(s[i]) {
          case 'a': case 'e': case 'i': case 'o': case 'u':
            s[i] = s[i].toUpperCase();
        }
    }
    return s.join('');
}

Edit note: My original answer had the for-loop written as for (var i in s) { ... }. As pointed out by @EliasVanOotegem, that is more suited for iterating over object properties than array elements. As suggested by @plalx and confirmed with jsperf, changing it to for (var i = 0; i < s.length; i++) { ... } makes it much faster. I have therefore amended my response, changing for (var i in s) to for (var i = 0; i < s.length; i++).


Note that your solution and the solution above blindly increments every character except spaces. If you want to increment just letters, then a replace operation would be more appropriate:

function transform(text) {
    var caesar = text.replace(/[a-z]/gi, function(c) {
        switch (c) {
          case 'z': return 'a';
          case 'Z': return 'A';
          default:  return String.fromCharCode(1 + c.charCodeAt(0));
        }
    });
    return caesar.replace(/[aeiou]+/g, function(vowel) {
        return vowel.toUpperCase();
    });
}

You could cheat on the Caesar cipher, mapping both z and Z to an uppercase A, since you know that vowels will need to be uppercased.


Addendum: Follow-Up Q&A

@Nathan asks in the comments…

1) I keep reading that strings are immutable and I guess this means that in my code, I'm not really concatenating, I'm replacing. I take it this is bad but I'm not sure why. I understand how your array works, just not why it is better.

If a and b are strings, when you do var c = a + b, then it creates a new string for the result (allocates memory for the length of a and b, then copies the contents of a, followed by the contents of b). If you iteratively do a concatenation for each character in a string, that works out to be O(n^2), where n is the length of the input.

In contrast, operating on an array of characters lets you modify the elements in place.

2) Both your solutions were functions. Is this so that they can be reused? The code is more self-contained and complete?

Even if you call the code just once, putting it in a function is beneficial, because it… - lets you give your code a name. Naming is important because it describes the purpose of the code. Good naming is a form of self-documentation. Awkward naming is a sign of muddled thinking. - compartmentalizes the code into small, understandable chunks. In JavaScript, variables can be scoped by function; without functions, all variables would be in the global scope, and assigning to a global variable can have side effects anywhere in your code.

Admittedly, transform(text) is not a very descriptive name, the way caesarCipher(text) is. However, the problem was a bit weird and artificial to begin with. Anyway, it's worthwhile to make it a function, to separate it from the prompting and outputting code. (Someday you might choose to use the same text transformation, but taking the text from an HTML <input> field and outputting the result to a <span>.

3) How does the function get it's argument (c)?

That's how the String.replace() function behaves. If you pass a function as the second argument to replace(), then for every match, that function will be called with the matched substring as its first parameter.

\$\endgroup\$
7
  • \$\begingroup\$ for...in is not a more modern way to write a loop, it's a way to iterate an object. For arrays, it's recommended you use a classical for loop. Besides, there's no reason to write 2 switch statements, they can easily be combined \$\endgroup\$ Oct 14, 2013 at 12:00
  • \$\begingroup\$ @EliasVanOotegem Thanks for the for-loop correction. Indeed, the classical for-loop is much faster, so I have amended my response. However, combining the two switch-blocks would drastically hurt the code clarity. \$\endgroup\$ Oct 14, 2013 at 14:40
  • \$\begingroup\$ Yah, it probably won't make the code more readable... I'm a bit too fond of switch-case fallthrough abuse :) \$\endgroup\$ Oct 14, 2013 at 14:43
  • \$\begingroup\$ Thank you. Everyone's answers were excellent. I'm blown away. Three questions for you. 1) I keep reading that strings are immutable and I guess this means that in my code, I'm not really concatenating, I'm replacing. I take it this is bad but I'm not sure why. I understand how your array works, just not why it is better. 2) Both your solutions were functions. Is this so that they can be reused? The code is more self-contained and complete? 3) How does the function get it's argument (c)? \$\endgroup\$
    – Nathan
    Oct 14, 2013 at 19:28
  • 1
    \$\begingroup\$ @200_success, Actually you should really be caching the length property because property lookups are expensive in JS. for (var i = 0, len = arr.length; i < len; i++), also looping backward is slightly faster because you can remove one condition check, var i = arr.length; then for (; i--;) {} but that might be overkill ;) \$\endgroup\$
    – plalx
    Oct 15, 2013 at 1:55
3
\$\begingroup\$

Something like this perhaps? I've used an object as a map for quick vowels lookups and took advantage of the fact that the replace function takes a replacement function. The whole transform is done in a single replace operation.

Note: I assume that we were only replacing lowercase letters, but it would be easy to handle all cases.

var vowels = ['a', 'e', 'i', 'o', 'u'].reduce(function (res, l) {
    res[l] = true;
    return res;
}, {});

'abcdefghijklmnopqrstuvwxyz'.replace(/[a-z]/g, function (l) {
    if (l === 'z') return 'A';
    l = String.fromCharCode(l.charCodeAt(0) + 1);
    return vowels[l]? l.toUpperCase() : l;
}); //bcdEfghIjklmnOpqrstUvwxyzA

EDIT:

Here's another way to create the map without using reduce. Like pointe out by @JanDvorak, the reduce construct might affect readability, however I like it since it allows you to define the map in a single statement, which is quite useful when you advocate using a single var statement.

var vowels = {};

['a', 'e', 'i', 'o', 'u'].forEach(function (l) {
    vowels[l] = true;
});

EDIT2:

Here's my implementation packaged within a reusable function. I know this is an exercise and that it's probably not meant to be reused, however it's worth knowing how to do it ;)

var transformLetters = (function () {
    var vowels = ['a', 'e', 'i', 'o', 'u'].reduce(function (res, l) {
            res[l] = true;
            return res;
        }, {}),
        rx = /[a-z]/g,
        replaceFn = function (l) {
            if (l === 'z') return 'A';
            l = String.fromCharCode(l.charCodeAt(0) + 1);
            return vowels[l]? l.toUpperCase() : l;
        };

    return function (s) {
        return s.replace(rx, replaceFn);
    };
})();

EDIT 3: Actually I've created a performance test that compares different solutions and it seems that using a map isin't the fastest way and it's actually slower than performing a double replace. I must say I am quite surprised. Anyway, here's the fastest implementation I could write:

PERFORMANCE TESTS (includes @200_success solutions)

var transform_switch = (function() {
  var rx = /[a-z]/g,
    replaceFn = function(l) {
      if (l === 'z') return 'A';
      l = String.fromCharCode(l.charCodeAt(0) + 1);
      switch (l) {
        case 'a':
        case 'e':
        case 'i':
        case 'o':
        case 'u':
          return l.toUpperCase();
        default:
          return l;
      }
    };

  return function(s) {
    return s.replace(rx, replaceFn);
  };
})();
\$\endgroup\$
12
  • \$\begingroup\$ uhh... reduce is a weird way to convert an array to a bit map. A literal loop would probably be better here. \$\endgroup\$ Oct 13, 2013 at 3:54
  • \$\begingroup\$ @JanDvorak, I hear your, however I really like to take avantage of the initialValue parameter and create a map in a single top-level statement. I do not see any disadvantages in using this looping construct. \$\endgroup\$
    – plalx
    Oct 13, 2013 at 3:58
  • \$\begingroup\$ what about readability? I found it hard to understand at first reading. \$\endgroup\$ Oct 13, 2013 at 4:00
  • \$\begingroup\$ @JanDvorak Well, perhaps it depends on your comprehension level, but reduce iterates over every elements in an array, just like forEach would. Then the rest of the code is very similar in both cases, the only difference being that you would have to declare the object in another statement var statement instead of passing it as the initialValue. Honestly I do not find that this affects readability, but that's my point of view an I might be wrong. Anyway I think that the overall solution is quite comprehensive an efficient. \$\endgroup\$
    – plalx
    Oct 13, 2013 at 4:04
  • \$\begingroup\$ as for the second half, what are 97 and 122? I don't like unexplained magic constants. \$\endgroup\$ Oct 13, 2013 at 4:09
2
\$\begingroup\$
  1. No, not really. There are other ways, but nothing that is simpler or faster.

  2. Yes, you can handle the z, the space and the vowels as one case, and the other characters by adding the character code:

    for (var i = 0; i<txt.length; i++){
        var c = txt.charAt(i);
        var index = " zdhnt".indexOf(c);
        if (index != -1) {
            newTxt += " AEIOU".charAt(index);
        } else {
            newTxt += String.fromCharCode(c.charCodeAt(0) + 1);
        }
    }
    
  3. See 2. Alternatively, you could use a regular expression to make the vowels uppercase:

    newTxt2 = newTxt.replace(/[aeiou]+/g, function(m){ return m.toUpperCase(); });
    

(Note that the code in 2 and 3 are exclusive, if you use the code in 2, the code in 3 is not needed.)

\$\endgroup\$
4
  • \$\begingroup\$ Thank you. Your feedback is excellent. I'm surprised that there is not a better way to advance characters alphabetically, but then again, maybe this is not a very common practice in the real world. I'm going to review this again in the morning. I'm not clear yet on " zdhnt" and " AEIOU", although the latter must be replacing lower case vowels with upper case. So much to learn. This is very helpful. \$\endgroup\$
    – Nathan
    Oct 14, 2013 at 3:34
  • \$\begingroup\$ @Nathan: The " zdhnt" and " AEIOU" replacement doesn't only turn vowels to upper case, it also keeps spaces unchanged, and wraps from z to a. \$\endgroup\$
    – Guffa
    Oct 14, 2013 at 8:38
  • \$\begingroup\$ Okay, I'm stuck in a couple places. \$\endgroup\$
    – Nathan
    Oct 14, 2013 at 18:57
  • \$\begingroup\$ Nevermind, the penny just dropped. Very nice! Wow, my code is so verbose compared to this. \$\endgroup\$
    – Nathan
    Oct 14, 2013 at 19:08

Not the answer you're looking for? Browse other questions tagged or ask your own question.