5
\$\begingroup\$

Have you got any suggestions to improve this code? The idea is to generate a one-line string that is visually symmetric.

Revisions at Github

<p id="display"></p>

<script>
var display = document.getElementById('display');

var chars = [33,34,39,42,43,45,46,48,58,61,72,73,77,79,84,92,94,95,111,124];
var double_chars = [40,41,47,92,60,62,91,93,123,125];
var double_chars_assoc = {40:41,47:92,60:62,91:93,123:125,
41:40,92:47,62:60,93:91,125:123};

function generateSymmetricAscii() {
    var ascii_string = "";
    var left_side = [];

    for(var i=0; i< randomInt(2,50); i++) {
        if(randomBool() == true) {
            left_side.push(randomChoice(chars));
        }
        else {
            left_side.push(randomChoice(double_chars));
        }
    }

    var right_side = left_side.slice(0);
    right_side.reverse();

    for(var i=0; i < right_side.length; i++) {
        if(inArray(right_side[i], double_chars) == true) {
            previous_char = right_side[i];
            right_side[i] = double_chars_assoc[previous_char];
        }
    }

    var total_array = left_side.concat(right_side);

    for(var i=0; i < total_array.length; i++) {
        ascii_string += String.fromCharCode(total_array[i]);
    }
    return ascii_string;
}

function randomInt(min, max) { return Math.floor(min + (Math.random() * (max - min))); }
function randomChoice(choices) { return choices[randomInt(0, choices.length-1)]; }
function randomBool() { return Math.random() >= 0.5; };
function inArray(needle, haystack) {
    var length = haystack.length;
    for(var i = 0; i < length; i++) {
        if(haystack[i] == needle) return true;
    }
    return false;
}

display.innerHTML = generateSymmetricAscii();

</script>
\$\endgroup\$
  • 1
    \$\begingroup\$ if(haystack[i] == needle) - wouldn't you want strict equivalence, if(haystack[i] === needle), instead? \$\endgroup\$ – eis Dec 24 '15 at 11:45
  • 2
    \$\begingroup\$ Can you explain what this code does in your question? \$\endgroup\$ – Nic Hartley Dec 24 '15 at 15:32
3
\$\begingroup\$

Wheel re-use

I Iike your usage of helper functions, inArray can be simplified by the use of indexOf:

 function inArray(x, xs) {return xs.indexOf(x) != -1}

Repetition

You repeat left_side.push(randomChoice twice. You should avoid repetition in code and state each concept or action once and just once.

FP

This part of your code would benefit from Functional Programming:

var total_array = left_side.concat(right_side);

for(var i=0; i < total_array.length; i++) {
    ascii_string += String.fromCharCode(total_array[i]);
}
return ascii_string;

Becomes:

return left_side.concat(right_side).map( String.fromCharCode).join("");

That looks much more declarative.

\$\endgroup\$
  • \$\begingroup\$ How about return String.fromCharCode.call(left_side.concat(right_side));? That does exactly the same as your FP section. \$\endgroup\$ – Ismael Miguel Dec 24 '15 at 18:10
  • \$\begingroup\$ @IsmaelMiguel If you feel like it is simpler, use it. Where is the equivalent of map though? \$\endgroup\$ – Caridorc Dec 24 '15 at 18:13
  • 1
    \$\begingroup\$ The map equivalent is what you have on your answer. But that is way clucky: String.fromCharCode() actually accepts n parameters. Using String.fromCharCode.call(array) will actually be somewhat faster than array.map(String.fromCharCode).join(''). With your implementation, you have to: 1) mutate an array (horrible idea for performance), 2) run String.fromCharCode n times and 3) .join(''). With String.fromCharCode.call(array), you cut down your operations a lot. But whichever one is cleaner to you. I'm just saying an alternative that I've used before. \$\endgroup\$ – Ismael Miguel Dec 24 '15 at 18:19
  • \$\begingroup\$ @IsmaelMiguel Cool I did not know that fromCharCode could have multiple arguments. Is this common in Javascript? \$\endgroup\$ – Caridorc Dec 24 '15 at 18:24
  • \$\begingroup\$ As common as using Array.prototype.slice.call(arguments) and Object.prototype.toString.call(object). I'm sure you have seen both. \$\endgroup\$ – Ismael Miguel Dec 24 '15 at 18:25
4
\$\begingroup\$

Styling and readability

Your coding style is inconsistent. Sometimes you use whitespace around your operators, sometimes you do not. Sometimes you put functions on one line, sometimes you put them on multiple lines. Which one you use seem to be up to random chance, which makes it harder to read your code.

variable == boolean is a bad practice. If variable evaluates to a boolean value true, then you can just use variable. Likewise, if you need the inverse, use !variable. If you want to test if the variable is true and of type boolean, use the === operator.

Javascript processes variable declarations in a context before executing the code. This process is called variable hoisting. This means that when you declare variables in the middle of a function, they are moved to the top. To avoid confusion why variables are defined before you declare them, or why local variables are used when you expected global variables to be used, I would recommend declaring all variables at the beginning of the context.

You are using more or less global variables with common names. This means that if you add another script, you might overwrite the contents of your current variables. I would recommend making it a closure:

(function() {
  //your current code here
})();

Your code would benefit from some comments. For example, a comment above the loop that substitutes characters with their symmetric counter part would become more intuitive that way.

You are using magic numbers in randomInt(2,50). Consider replacing 2 and 50 with a variable, and define these variables at the top. This allows you to change such arbitrary values without having to touch the logic part of your program.

Logic errors

Your function randomInt(min, max) will return an integer between min (included) and max (excluded). Your function randomChoice(choices) will select an element between 0 (included) and length-1 (excluded). This means that it is impossible to get the last element from choices back as a result.

Your function inArray(..) can produce false-positives if the types of the elements in your Array and the needle are not the same.

Other improvements

You use your own function inArray(..), but you can accomplish the same by just using indexOf(..) and testing if the result is > -1.

You are using an Array and an Object for keys of an Object, and the Object itself. To get the keys of an Object, use Object.keys( variable ). With this you avoid bugs related to out-of-sync variables.

\$\endgroup\$
4
\$\begingroup\$

Omit the extra == true in conditions:

if(randomBool())

Use the ternary if operator to compact your code:

left_side.push(randomChoice(randomBool() ? chars : double_chars))

If this seems to dense for you, you can also extract the inner expression like this:

var options = randomBool() ? chars : double_chars;
left_side.push(randomChoice(options))
\$\endgroup\$
  • \$\begingroup\$ The use of ternary if is one of personal preference. I personally dislike using it outside simple assignments, because it makes grouping the right things together harder for me, thus making it less readable for me. \$\endgroup\$ – Sumurai8 Dec 24 '15 at 14:45
  • 1
    \$\begingroup\$ Yes, I think this is a corner case. I also thought about extracting the parameter to randomChoice into a local variable. I'll change that so we have two simple lines instead of one slightly more complex one. But I still prefer the ternary operator. \$\endgroup\$ – lex82 Dec 24 '15 at 15:51
  • \$\begingroup\$ @Sumurai8 His suggestion avoids the repetition of left_side.push(randomChoice according to DRY principle, so it is objectively better. \$\endgroup\$ – Caridorc Dec 24 '15 at 15:51
  • \$\begingroup\$ @carldorc I extracted the charArray parameter on purpose. I'll just provide both versions in the answer. \$\endgroup\$ – lex82 Dec 24 '15 at 15:55

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.