0
\$\begingroup\$

This exercise with a test suite is from here. I did not intend to use ES6 features, it's plain old JavaScript. That's why I seek advice more on the good practices, performance side, than modern syntax. Basically anything to make this code more elegant, without ES6.

// Constructor
var Binary = function(binary) {
  this.binary = binary;
  this.base = 2;
};

// Transform the string input in a reversed array
Binary.prototype.reverseInput = function() {
  return this.binary.split('').reverse();
};

// Handle the conversion to decimal
Binary.prototype.toDecimal = function() {
  var output = 0;
      binaryArray = this.reverseInput();

  if (this.invalidInput()) {
    return output;
  }

  for (var i = 0, j = binaryArray.length; i < j; i++) {
    output += binaryArray[i] * Math.pow(this.base, i);
  }

  return output;
}

// Check if any character is not a 1 or a 0
Binary.prototype.invalidInput = function() {
  for (var i = 0, j = this.binary.length; i < j; i++) {
    if (this.binary[i] != '0' && this.binary[i] != '1') {
      return true;
    }
  }
}

module.exports = Binary;
\$\endgroup\$

1 Answer 1

Reset to default
1
\$\begingroup\$

Flawed exercise

Before I begin, I would like to point out that the exercise is flawed:

  • The task is not really converting the input from a binary to a decimal representation. Rather, it is to convert it to a number, whose conventional string representation is rendered in base 10. The test suite included this test:

    xit('11010 is decimal 26', function() {
  expect(new Binary('11010').toDecimal()).toEqual(26);
});

    … which could also have been written as:

    xit('11010 is twenty-six', function() {
  expect(new Binary('11010').toDecimal()).toEqual(0x1a);
});

  • It is bad practice to return 0 for new Binary('junk').toDecimal(). Acceptable results would be to either throw an exception or return Number.NaN.

Review

Your invalidInput function either returns true or falls off the end. Good practice would be to name it isInvalidInput, and have it explicitly return true or false. Better yet, reverse the sense and call it isValidInput. Even better, don't expose the validation function as a method at all — you don't really need one.

this.binary.split('').reverse() and Math.pow(…) seem extravagant.

Suggested solution

// Constructor
var Binary = function(binary) {
  this.binary = binary;
  this.base = 2;
};

Binary.prototype.toDecimal = function() {  // <-- Method name is a misnomer!
  var output = 0;
  for (var i = 0; i < this.binary.length; i++) {
    output *= 2;
    switch (this.binary.charAt(i)) {
      case '0': break;
      case '1': output += 1; break;
      default:  return 0;   // <-- This sucks, but the test suite requires it!
    }
  }
  return output;
}
\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.