HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

l33t Speak translator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
translatorl33tspeak

Problem

Please review the code quality of this l33t Speak translator. Improvement suggestions are welcome. Here it is - http://jsbin.com/IHoFuQE/1

(function() {
  "use strict";

  // http://en.wikipedia.org/wiki/Leet
  // http://www.catb.org/jargon/html/crackers.html

  var alphabets = { // common
    "a": "4",
    "b": "8",
    "e": "3",
    "f": "ph",
    "g": "6", // or 9
    "i": "1", // or |
    "o": "0",
    "s": "5",
    "t": "7" // or +
  },
    alphabets2 = { // less common
      "c": "(", // or k or | 0.5) {
        text = text.replace(text[i], text[i].toUpperCase());
      } // else keep lower case
    }
    return text;
  }

  (function() { // l33t the words object
    for (var word in words) {
      if (words.hasOwnProperty(word)) {
        words[changeLetters(word)] = words[word];
        delete words[word];
      }
    }
  }());

  function tol33t() {
    leet.value = randomcase.checked ? randomizeCase() : changeWords();
  }

  elite.addEventListener("input", tol33t);
  btn.addEventListener("click", tol33t);
}());

Solution

To start with, I'd like to give you credit for using "use strict" and limiting the scope of variables using functions.

Your tol33t() function is weird to me. If the randomcase button is checked, you call randomizeCase() with no arguments. What are you randomizing the case of? It turns out that randomizeCase() calls changeWords(), so it turns out that the two code branches have something in common after all. That wasn't apparent from looking at tol33t().

There should be clearer separation responsibility between the DOM-interaction and text-translation layers of code. The text translation code should reside in pure functions (whose results depend solely on their parameters) that you could call from the JavaScript console or from a unit testing engine. That separation of concerns could be accomplished with:

function onTranslateButtonClicked() {
    var l33tText = toL33t(elite.value, adv.checked);
    if (randomcase.checked) l33tText = randomizeCase(l33tText);
    leet.value = l33tText;
}


Your "l33t the words object" initialization routine lays a trap for anyone trying to read the code. When code in changeWords() refers to the words associative array, one would reasonably think that it's using the definition above (i.e., words = { "am": "m", … }) instead of the munged version (words = { "4m": "m" }). Better to make that munging explicit when you define words (and do so by returning a copy rather than mutating the dictionary as you iterate):

var words = l33tKeys({
    "am": "m",
    …
});

function l33tKeys(dict) {
    var l33tDict = {};
    for (var word in dict) {
        l33tDict[changeWord(word)] = dict[word];
    }
    return l33tDict;
}


I would write changeLetters() as:

// letterSubsts may be alphabet or alphabet2
function changeLetters(text, letterSubsts) {
    return text.toLowerCase().replace(/[a-z]/g, function(letter) {
        return letterSubsts[letter] || letter;
    });
}


… which is actually analogous to your implementation of changeWords().

Code Snippets

function onTranslateButtonClicked() {
    var l33tText = toL33t(elite.value, adv.checked);
    if (randomcase.checked) l33tText = randomizeCase(l33tText);
    leet.value = l33tText;
}
var words = l33tKeys({
    "am": "m",
    …
});

function l33tKeys(dict) {
    var l33tDict = {};
    for (var word in dict) {
        l33tDict[changeWord(word)] = dict[word];
    }
    return l33tDict;
}
// letterSubsts may be alphabet or alphabet2
function changeLetters(text, letterSubsts) {
    return text.toLowerCase().replace(/[a-z]/g, function(letter) {
        return letterSubsts[letter] || letter;
    });
}

Context

StackExchange Code Review Q#39365, answer score: 5

Revisions (0)

No revisions yet.