2
\$\begingroup\$

To practice my JavaScript skills, I wrote a phrase scrambler. I would appreciate feedback on code efficiency and readability, as I want to know the best practices for further development.

function scramble() {

	let phrase = document.getElementById('phrase').value;
	phrase = phrase.split('');
	let newPhrase = "";

	let length = phrase.length;
	for(let i = 0; i < length; i++) {
		let randomNumber = Math.floor(Math.random() * phrase.length);
		newPhrase += phrase[randomNumber];
		phrase.splice(randomNumber, 1);
	}

	let output = document.createElement("p");
	let output_text = document.createTextNode(newPhrase);
	output.appendChild(output_text);

	document.getElementById('output').appendChild(output);

}
<!DOCTYPE html>
<html lang="en-US">
	<head>
		<meta charset="UTF-8">
		<title>Phrase Scrambler</title>
		<script src="scramble.js"></script>
	</head>
	<body bgcolor="pink">
		<h1>Phrase Scrambler</h1>
		<form action="" method="post">
			<label>Phrase:</label>
			<input type="text" id="phrase"><br>
			<button type="button" onclick="scramble()">Scramble</button>
		</form>

		<div id="output">
			<!-- Output goes here -->
		</div>
	</body>
</html>

\$\endgroup\$
  • 1
    \$\begingroup\$ Is it ok that the scramble has a statistical chance on yielding the exact same result as the source? \$\endgroup\$ – dfhwze Jun 4 at 6:47
2
\$\begingroup\$

Reusable

One of programmings main attributes it the ability to reuse code. You should always write code with this in mind

When you create code think about how you may need to do the same thing over and over. Write a function to do that task in such a way so that you can add it to a library. This will reduce the amount of work needed to complete the current project and future projects

User friendly

You can be the greatest programming in the world but if you lack good UI design skills your code will never be great

It is more important to concentrate on the front end than how you do the back-end because users, the ones that ultimately pay for every line of code, never see a line of it

This does not mean you can write bad code. You want reduce your work, and you get that by writing good code.

Some good UI tips

  • Reduce the amount of work a user must do to use your app is one of the best ways to create a good user interface.
  • Be entertaining, animations and additional quirks go a long way to providing a good user experience. BUT never let the entertainment get in the way of usability.
  • Provide feedback. Use tooltips, cursors... etc to help the user use the interface. Again the feedback should never get in the way of the interfaces basic use.

Javascript

  • Use direct object reference to access elements by their id
  • Create utility functions to reduce code size and noise
  • Use up to date JS syntax to reduce code size and keep your skills relevant
  • In many cases while loops suit the algorithm better than for loops
  • splice will return an array of items spliced. You can use bracket notation to get the spliced item

DOM

  • Use CSS to hold page styles, don't embed style into the HTML
  • If you are not relying on a server to process input you can avoid the <form> and associated overhead
  • The input element will not always get focus on load. You can force focus in JS
  • Don't add javascript code inline

Example

  • Uses utilities that I wrote for other apps.
  • UI friendly
    • Focuses on text input so user need not click it to add content
    • Scrambles on enter and button click
    • Inserts new scrambled text at the top of the output so user need not scroll to see new result
    • Reduce spacing between lines
    • Does not scramble empty like strings
    • Adds titles for tool tips to provide user feed back
    • Adds placeholder to input for more information for the user
    • Adds simple animation to focus users eye on new result and provide a little entertainment (I bet you use this version a few more second than average app doing the same basic functionality)
    • Can be used without having to touch the mouse.
  • Uses CSS to set element styles
  • Uses direct object reference to access elements (rather than getElementById)

I also slightly changed how the phrase is scrambled. It first scrambles the words and then scrambles the characters in each word. The shuffle (JS utility) function takes a second argument that modifies each item as they are shuffled

With all the extras and excluding the utility functions the code below is only a little longer then your original function and took me very little time to write.

// DOM utility functions
const tag = (type, props = {}) => Object.assign(document.createElement(type), props);
const insert = (el, ...sibs) => sibs.reduce(
    (el, sib) => (el.insertBefore(sib, el.children[0] ? el.children[0] : null), el), el
);
const addEvent = (el, type, func, opts = {}) => (el.addEventListener(type, func, opts), el);


// JS utility functions
const shuffle = (a, mod = i => i, l = a.length) => {
    while (l) { a.push(mod(a.splice(Math.random() * (l--) | 0, 1)[0])) } 
    return a;
};


// Application code
const SCRAMBLE_FRAMES = 40; // in frames 40/60 is 2/3rds of a second

addEvent(scrambleBut, "click", scramble);
addEvent(phraseEl, "keyup", e => e.code === "Enter" && scramble());
phraseEl.focus();
const wordShuffle = word => shuffle([...word]).join("");
const animateShuffle = (el, count = SCRAMBLE_FRAMES) => {        
    el.textContent = shuffle(el.textContent.split(" "), wordShuffle).join(" ");
    if (count > 0) { requestAnimationFrame(()=> animateShuffle(el, count -1)) } 
}
function scramble() {    
    const phraseText = phraseEl.value.trim();
    if (phraseText) {
        var scrambleRes;
        insert(outputEl, scrambleRes = tag("p", {textContent: phraseText}));
        animateShuffle(scrambleRes);
    }
    phraseEl.focus();
}
body {
    background-color: pink;
    font-family: arial;
}
p {
    margin-block-start: 0.2em;
    margin-block-end: 0.2em;
}
<h1>Phrase Scrambler</h1>
<label>Phrase:</label>
<input type="text" id="phraseEl" placeholder="Enter a phrase" title="Type a phrase, hit enter or click scramble to scramble the phrase"><br>
<button type="button" id="scrambleBut" title="Click to scramble current phrase">Scramble</button>
<div id="outputEl"></div>

\$\endgroup\$
0
\$\begingroup\$

When I first saw the words "phrase scrambler", I thought you would split the phrase into words, not individual characters, and scramble these. I expected your code to solve interesting problems like:

  • If the phrase starts with an uppercase letter and also contains lowercase letters, make sure that the scrambled phrase also starts with an uppercase letter, making the previously first character lowercase.
  • Do not rip emojis apart.
  • Split the phrase into words and scramble these.
  • Split the phrase into words, scramble each word individually, and then scramble the words. (It would be interesting whether it would still be possible for humans to guess the original phrase.)

Your current code gets the job done efficiently and is easy to understand. To allow for the above improvements or experiments, you should extract the part that scrambles an array into its own function:

function scramble(arr, rnd) {
    const scrambled = [];
    ...
    return scrambled;
}

Having this as a separate function means that you can easily test it. For that, you need a predictable random number generator. You can then define:

function predictable() {
    return ...; // From the Stack Overflow question linked above
}

function testScramble(str, expected) {
    const actual = scramble(str, predictable);
    if (actual !== expected) {
        console.log('scramble', str, 'is', actual, 'expected', expected);
    }
}

test('hello', 'ohlel');
// TODO: add more test cases

To make the scramble function faster, you need to know that shuffle is a common synonym, and the standard algorithm for shuffling an array is the Fisher–Yates shuffle, which is pretty simple.

\$\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.