3
\$\begingroup\$

I have created a binary to decimal calculator that needs to follow this spec:

  • Arrays may not be used contain the binary digits entered by the user
  • Determining the decimal equivalent of a particular binary digit in the sequence must be calculated using a single mathematical function, for example the natural logarithm. It's up to you to figure out which function to use.
  • User can enter up to 8 binary digits in one input field
  • User must be notified if anything other than a 0 or 1 was entered
  • User views the results in a single output field containing the decimal (base 10) equivalent of the binary number that was entered
  • User can enter a variable number of binary digits

My code is:

binaryForm = document.getElementById('formComplete');
numberInput = document.getElementById('numberInput');
numberInputButton = document.getElementById('updateNumberBtn');
binaryInput = document.getElementById('binaryInput');
decimalValue = document.getElementById('decimalOutput');

numberInputButton.addEventListener('click', function () {
    var numberInputValue = numberInput.value;
    binaryInput.setAttribute('maxLength', numberInputValue);
});

binaryInput.addEventListener('keydown', function (e) {

    if (e.keyCode !== 48 && e.keyCode !== 49 && e.keyCode !== 13 && e.keyCode !== 8) {
        e.preventDefault();
        alert('Only 0 and 1 is allowed');
    }

});

binaryForm.addEventListener('submit', function (e) {
    e.preventDefault();

    if (binaryInput.value == 0) {
        alert('Empty');
    } else {
        var userInput = binaryInput.value;
        var total = 0;
        for (var i = 0; i < userInput.length; i++) {
            var binaryDigit = parseInt(userInput[i]);
            var calcInput = (total * 2) + binaryDigit;
            total = calcInput;
        }
        decimalValue.innerHTML = total;
    }
});
body {
    background: #2e2e2e;
    color: #fff;
    font-family: Heebo, sans-serif;
}

h1,
h2,
h3,
h4 {
    font-weight: 900;
}

input {
    display: block;
    margin: 0 auto;
    width: 100%;
    max-width: 300px;
    height: 60px;
}

.form-body {
    min-height: 100vh;
    display: flex;
    flex-direction: column;
    justify-content: center;
    text-align: center;
}

#numberInput,
#binaryInput {
    margin-bottom: 1rem;
    border: 0.125rem solid rgba(255,255,255,0.25);
    background: rgba(255,255,255,0.05);
    color: #fff;
    font-size: 2rem;
    text-align: center;
    font-family: Heebo, sans-serif;
    font-weight: 300;
}

#updateNumberBtn,
#convertBtn {
    margin-bottom: 1rem;
    border: 0.125rem solid rgba(0,128,0,0.75);
    color: #fff;
    background: rgba(0,128,0,0.75);
    text-transform: uppercase;
}

#updateNumberBtn:hover,
#convertBtn:hover {
    background: rgba(0,128,0,0.25);
    cursor: pointer;
}

  
  
  
<div class="form-body">
  <h1>Binary to Decimal Calculator</h1>
  <label for="numberInput">Set max number of bits (Max 12 - Default 8)</label>
  <input id="numberInput" type="number" value="8" min="1" max="12">
  <input id="updateNumberBtn" type="button" value="Set Number of Bits">
  <form action="" id="formComplete">
      <label for="binaryInput">Enter a binary value</label>
      <input id="binaryInput" type="text" size="12" maxlength="8">
      <input id="convertBtn" type="submit" value="Convert to Decimal">
  </form>
  <h2>Decimal Value: <span id="decimalOutput"></span></h2>
</div>

Are there areas that I could improve performance-wise or by making the app easier to operate, such as the part where I am targeting the keys that the user can input.

\$\endgroup\$
3
\$\begingroup\$
  • You should use the directive "use strict" that will place the JavaScript context into strict mode. This will throw errors for some common bad practices.

  • Always declare variables as const, var, or let. If you are in strict mode you will get an error if you don't.

  • Don't use alerts or prompts as there is no way to know if they are actually displayed (clients can turn them off) and they are very annoying.

  • The key event properties KeyboardEvent.keyCode and KeyboardEvent.charCode have been depreciated and you should not use them. Use KeyboardEvent.code or KeyboardEvent.key instead

  • Rather than filter the input via the keybpoard events, listen to the input's keyup and change events, removing bad characters automatically. Use a CSS rule to unhide a warning and a JavaScript setTimeout to hide it again

    Filtering keyboard events means you need to check many keys that are valid (left, right, backspace, delete, etc...) which is just unneeded complexity.

  • Don't wait for the user to click "Convert to Decimal", display the output automatically. This makes it a lot friendlier to use.

  • JavaScript can convert binary strings to decimal for you using parseInt. The second argument is the radix (AKA base) of the number that is being parsed.

  • If you are just setting text (no HTML) use the elements textContent rather than innerHTML

Rewrite

The rewrite is following the points you have set-out in your question. I have not implemented how your code differs from these points.

The rewrite uses

  • a RegExp to test and filter the input.
  • a ternary expression to create the decimal value as parseInt will return NaN for empty strings. The ternary checks if the string is empty evaluating to "" or if there is a number the ternary evaluates to the decimal value.
  • HTMLInputElement.focus (inherited from HTMLElement) to focus the input element when loaded.

I have modified the HTML and CSS to fit the snippet window a little better.

"use strict";
const binaryInput = document.getElementById("binaryInput");
const decimalOutput = document.getElementById("decimalOutput");
const inputWarning = document.getElementById("inputWarning");
binaryInput.addEventListener('change', update);
binaryInput.addEventListener('keyup', update);
binaryInput.focus();

const WARNING_TIME = 2000; // in ms
var warningTimer;
function hidWarning() {
   inputWarning.classList.add("hideWarning");
}
function showWarning() {
    clearTimeout(warningTimer);
    warningTimer = setTimeout(hidWarning, WARNING_TIME);
    inputWarning.classList.remove("hideWarning");
}
function update() {
    var value = binaryInput.value;
    if (/[^01]/g.test(value)){
        binaryInput.value = value = value.replace(/[^01]/g, "");
        showWarning();
    }
    decimalOutput.textContent = value === "" ? "" : parseInt(value, 2);
}
body {
    background: #2e2e2e;
    color: #fff;
    font-family: Heebo, sans-serif;
}
.form-body {
    display: flex;
    flex-direction: column;
    text-align: center;
}
input {
    margin: 0 auto;
    width: 100%;
    max-width: 300px;
    border: 0.125rem solid rgba(255,255,255,0.25);
    background: rgba(255,255,255,0.05);
    color: #fff;
    font-size: 2rem;
    text-align: center;
    font-weight: 300;    
}

.inputWarning { color: #F88 }
.hideWarning { display: none }
<div class="form-body">
  <h2>Binary to Decimal Calculator</h2>
  <label for="binaryInput">Enter a binary value<span id="inputWarning" class="inputWarning hideWarning"> [Only valid digits 0 and 1 permitted]</span></label>
  <input id="binaryInput" type="text" size="8" maxlength="8">
  <h3>Decimal Value: <span id="decimalOutput"></span></h3>
</div>

\$\endgroup\$
  • \$\begingroup\$ Thank you, that was the type of in-depth feedback I was hoping for. It's a much smoother operation. I think the only area I am not too familiar with is the RegExp but this will come with time \$\endgroup\$ – Bagseye May 7 at 22:37
  • \$\begingroup\$ When you give an element an id, it already exists as a property on the window object. You don't need to use getElementById. Since you are using the same name you can just remove those 3 lines entirely. By the way, your opening and closing tag doesn't match for the headings. \$\endgroup\$ – Kruga May 8 at 8:13
  • \$\begingroup\$ @Kruga If I don't add a detailed explanation regarding direct referencing I get down-voted for using them eg codereview.stackexchange.com/a/184295/120556 I volunteer answers and time is not always available to provide a defense of direct referencing as in the following codereview.stackexchange.com/a/217497/120556 \$\endgroup\$ – Blindman67 May 8 at 15:28

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.