Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

My code will generate a spelling mistake inside of a string 50% of the time.

It will retrieve a letter from a random index in the string like, "t" and then duplicate that letter like, "tt" and store it in the spelling mistake variable.

It will then replace, "t" with "tt" in the string to replicate a spelling mistake. (50% of the time.)

How can I improve my code to make it complete the same task, but with less lines of code and using the least amount of resources possible?

(function() {

    function replaceStr(str, pos, value) {
        var arr = str.split('');
        arr[pos] = value;
        return arr.join('');
    }

    var myString = "Stack Overflow";
    var letterIndex = Math.floor(Math.random() * myString.length); // Example: 1
    var letter = myString.charAt(letterIndex); // Example: "t"
    var mistake = letter + letter; // Example: "tt"

    // 0 -> 9 (coin toss)
    if (Math.floor(Math.random() * 10) >= 5) {
        return replaceStr(myString, letterIndex, mistake);
    } else {
        return myString;
    }

})();

Result from 10 runs:

Stack Overflow
Stackk Overflow
Stack Overflow
Sttack Overflow
Stack Overflow
Stack Oveerflow
Stack Overrflow
Stack Overfllow
Stack Overflow
Stack Oveerflow
share|improve this question
2  
Does it matter that duplicating a letter doesn't always generate a misspelled word? E.g., duplicating t in later produces latter which is another correctly spelled word. – Joshua Taylor Mar 2 at 22:18
    
I've never considered the potential applications for a typo machine. Interesting. – JamesFaix Mar 2 at 23:53
    
Sometimes less lines of code and less resources are opposite things. – Pharap Mar 3 at 5:18
3  
But, why would you need to make a spell error? Just curious :-) – xyz Mar 3 at 6:57
2  
@xyz reassons... – mikeTheLiar Mar 3 at 14:50
up vote 22 down vote accepted

Break it down into one function that randomly repeats a character, and another function which randomly executes that function 50% of the time:

function repeatRandomChar(str) {
  var i = Math.floor(Math.random() * str.length);
  return str.slice(0, i+1) + str.slice(i);
}

function randomTypo(str) {
  return Math.random() > 0.5 ? repeatRandomChar(str) : str;
}

Now you can just call randomTypo('Stack Overflow') 10 times to reproduce your sample data.

Note also that the original code has needless multiplication, in that

 Math.random() > 0.5 

is equivalent to:

 (Math.random() * 10) >= 5
share|improve this answer
    
Your elimination of the needless multiplication with Math.random() < 0.5 is also worth pointing out. – 200_success Mar 2 at 19:39
2  
Wouldn't it be more efficient to avoid an extra concatenation by replacing str.slice(0, i+1) + str.charAt(i) + str.slice(i+1) with str.slice(0, i+1) + str.slice(i)? – Michael Urman Mar 2 at 22:47
    
@MichaelUrman Love it. I made the change. – Jonah Mar 2 at 23:04
5  
Actually Math.random() >= 0.5 would be equivalent to (Math.random() * 10) >= 5. In this case it doesn't matter whether you check >= or <, but for any other number (say 7 and 0.7) it does. – immibis Mar 3 at 2:07
    
One may also want to check for whites. – DarioP Mar 3 at 14:50

This code is at the end of your function:

// 0 -> 9 (coin toss)
if (Math.floor(Math.random() * 10) >= 5) {
    return replaceStr(myString, letterIndex, mistake);
} else {
    return myString;
}

Half of the time on average, all the code above this code will not matter, because you simply return myString unchanged. So it would be better to move this condition higher, before the other logic, such as picking a random position to introduce the mistake, etc.

And instead of splitting the string and changing a position, perhaps it's more efficient to work with substrings.

Something like this:

function addMistake(myString) {
    // 0 -> 9 (coin toss)
    if (Math.floor(Math.random() * 10) < 5) {
        return myString;
    }

    function replaceStr(str, pos, value) {
        return str.substr(0, pos) + value + str.substr(pos + 1);
    }

    var letterIndex = Math.floor(Math.random() * myString.length); // Example: 1
    var letter = myString.charAt(letterIndex); // Example: "t"
    var mistake = letter + letter; // Example: "tt"

    return replaceStr(myString, letterIndex, mistake);
}
share|improve this answer

You can reduce your code, merely avoiding useless complications, like this:
(also improved using @Michael Urman's idea to compact the substitution statement)

function doMistake(str) {
  if (Math.floor(Math.random() * 10) >= 5) {
    var letterIndex = Math.floor(Math.random() * str.length);
    str = str.substr(0, letterIndex + 1) + str.substr(letterIndex);
  }
  return str;
}

Here it is working with the example provided by the OP:

function doMistake(str) {
  if (Math.floor(Math.random() * 10) >= 5) {
    var letterIndex = Math.floor(Math.random() * str.length);
    str = str.substr(0, letterIndex + 1) + str.substr(letterIndex);
  }
  return str;
}

for (var i = 0; i < 10; i++) {
  console.log(doMistake('Stack Overflow'));
}

share|improve this answer
3  
Wouldn't it be more efficient to avoid an extra concatenation by replacing str.substr(0, letterIndex) + str.charAt(letterIndex) + str.substr(letterIndex) with str.substr(0, letterIndex + 1) + str.substr(letterIndex)? – Michael Urman Mar 2 at 22:48
    
@MichaelUrman Wow, nice improvement, and a clever one! I edit my answer with it. Thanks! – cFreed Mar 2 at 22:58

One could String.replace() with an replacement-function. Moreover I have incorporated a few of the suggestions already made.

var myString = "Stack Overflow";
var letterIndex; // Example: 1
var letter; // Example: "t"

function replacer(match, offset, string) {
  if (offset === letterIndex) {
    return match + match;
  }
}

// 0 -> 9 (coin toss)
if (Math.random() >= 0.5) {
    letterIndex = Math.floor(Math.random() * myString.length);
    letter = myString.charAt(letterIndex);

    return myString.replace(letter, replacer);
} else {
    return myString;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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