1
\$\begingroup\$

So, I have decided to make a little "guessing game" where the user tries to decode what the encoded piece of text prompted. I'm sure it works. However, I know that there are places where I can improve. I am open to any constructive criticism about this. This program was done in HTML + JavaScript.

<!DOCTYPE HTML>

<HTML>

<HEAD>
  <META CHARSET="UTF-8" />
  <TITLE>Decryption Guessing Game</TITLE>

</HEAD>

<BODY>
  <p id="para">Text:</p>
  <form name="game" action="#" onsubmit="return validateForm()">
    Decrypt: <input type="text" name="text">
    <input type="submit" value="Check!">
  </form>
  <SCRIPT LANGUAGE="Javascript">
    function randomInt(min, max) {
      min = Math.ceil(min);
      max = Math.floor(max);

      return Math.floor(Math.random() * (max - min + 1)) + min;
    } // defines the function that gets a random number, will be useful later

    var text = ['smell', 'cat', 'jump']; // pieces of text to decrypt
    var encryptedText = []; // the decrypted pieces of text.

    for (var i = 0; i < text.length; i++) {
      encryptedText.push(window.btoa(text[i]));
    }

    var pieceOfText = encryptedText[randomInt(0, 2)];
    console.log(pieceOfText);
    document.getElementById('para').innerHTML += " " + pieceOfText;

    function validateForm() {
      var form = document.forms['game']['text'];
      var input = form.value;

      if (input == "") {
        alert("Enter your answer within the input.");
        return false;
      } else if (!(/^[a-zA-Z0-9-]*$/.test(input))) {
        alert("Your input contains illegal characters.");
        return false;
      } else if (input != window.atob(pieceOfText)) {
        alert("Incorrect; try again.");
        location.reload();
      } else {
        alert("Correct!");
        location.reload();
      }
    }
  </SCRIPT>
</BODY>

</HTML>

\$\endgroup\$
2
  • 2
    \$\begingroup\$ (1) I would suggest that you use lower-case for your html tag names. (2) Instead of reloading the page each time you should just reset the form (document.getElementsByName('game')[0].reset()) \$\endgroup\$
    – Marc Rohloff
    Commented Apr 14, 2017 at 3:21
  • \$\begingroup\$ @MarcRohloff Comments are for seeking clarification to the question. Please write all suggestions as answers, even if the answer is short. \$\endgroup\$
    – 200_success
    Commented Apr 14, 2017 at 6:04

1 Answer 1

Reset to default
4
\$\begingroup\$

Use lowercase tag names and attributes

Quoting Quentin from "Is it bad to use uppercase letters for html tags?":

The only relevant parts of specifications say:

  • HTML tag and attribute names are case insensitive.
  • XHTML tag and attribute names are case sensitive and must be lower case

Stuff that isn't mentioned in any standard:

  • Lower case is generally considered easier to read
  • Lower case is most common (and what people are used to working with)
  • Holding down the shift key or toggling CAPS LOCK all the time is a pain

Don't use onsubmit attribute

It's a rather bad code style to still use attributes like this one, these days. For example, that way the submit event would not be raised by form.submit() function, which you may want to use later. Use addEventListener() instead, it's supported even by IE9+.

Don't use deprecated language attribute of script tag

Not only it is deprecated and it's not used anymore, but also a standard regarding this attribute's values never existed. You should use type="text/javascript" instead or no attribute at all, since in HTML5 in case of lack of specification of language used by script tag, it is assumed to be JavaScript, anyway.

Randomness source and distribution

Your function randomInt() uses Math.random() to obtain random values in a manner that will give you non-uniform distribution of returned values. It should also be noted, that it provides non-cryptographically secure randomness, although this is not a problem in this case.

Comments

Not all comments are useful, especially this invalid one:

var encryptedText = []; // the decrypted pieces of text.

Global scope

You shouldn't declare variables in the global scope, like you did between the two functions you made. Break that code into another function or combine with already existing ones, if appropriate.

Use map()

This

var encryptedText = []; // the decrypted pieces of text.

for (var i = 0; i < text.length; i++) {
  encryptedText.push(window.btoa(text[i]));
}

could be shortened to one, very straight-forward line:

var encodedText = text.map(window.btoa);

Use .textContent instead of .innerHTML

Since you don't want nor need to have your string parsed, you should use .textContent property instead of .innerHTML.

Inconsistent quotation marks

You mix single ('') and double ("") quotation marks. Use single ones, except where it would force you to escape what's between them.

Dot notation

This

var form = document.forms['game']['text'];

would look better and be written by most as:

var form = document.forms.game.text;

Use strict equality operator

You should use strict equality operator in your code, as it doesn't performs type conversion and is generally considered a good practice.

Don't location.reload()

You don't need to location.reload(), just reset() the form.

Don't encode before necessary

Currently you are encoding all strings before they are even randomly chosen. There is no need to do that. What if there were thousands of strings? Reverse the order of operations, first choose random string and just then encode it. That way, you won't even need to store results of encoding, by the way.

Naming

What you do is encoding and decoding with Base64, but one variable is named encryptedText. Comment on the same line have exactly the same naming issue. There is no encryption involved in your code, at all.

Replace two location.reload()s with one

You could remove two instances of location.reload() that you have in your code and add just one below the chain of conditional checks.

Edit: Although remember that even this one location.reload() should be replaced with form resetting, as mentioned before.

Empty string submission check

Instead of checking if (input === '') you could add required="" attribute to your input tag.

\$\endgroup\$
8
  • \$\begingroup\$ @ObinnaNwakwue: Absolutely, if even IE9 supports it, more so does Chrome. Webpage I linked you to had browser compatibility table at it's bottom too. \$\endgroup\$
    – Przemek
    Commented Apr 14, 2017 at 15:21
  • \$\begingroup\$ When I apply your suggestion of using addEventListener(), reset() and using only 1 location.reload(), when I go to test it out, it doesn't execute the if statement. What happened? \$\endgroup\$
    – Obinna Nwakwue
    Commented Apr 14, 2017 at 15:22
  • \$\begingroup\$ Never mind, I used the "onsubmit" event which doesn't exist when I should've used "submit." \$\endgroup\$
    – Obinna Nwakwue
    Commented Apr 14, 2017 at 15:24
  • \$\begingroup\$ @ObinnaNwakwue: Also remember about replacing location.reload() with form resetting. \$\endgroup\$
    – Przemek
    Commented Apr 14, 2017 at 15:43
  • \$\begingroup\$ I did that already. Everything's working now. \$\endgroup\$
    – Obinna Nwakwue
    Commented Apr 14, 2017 at 16:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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