Binary string to decimal conversion

Question

Problem Statement:

Write a program that will convert a binary number, represented as a string (e.g. '101010'), to its decimal equivalent using first principles

Implement binary to decimal conversion. Given a binary input string, your program should produce a decimal output. The program should handle invalid inputs.

Code:

public class Binary {
 private final String binaryString;
 public Binary(String binaryString) {
 this.binaryString = binaryString;
 }
 public int getDecimal() {
 return toDecimal(binaryString);
 }
 public static int toDecimal(String str) {
 int decimal = 0;
 int base0 = (int)'0';
 char ch;
 for (int i = 0; i < str.length(); i++) {
 ch = str.charAt(i);
 if (charIsNotBinary(ch)) {
 return 0;
 }
 decimal = decimal * 2 + ((int)str.charAt(i) - base0);
 }
 return decimal;
 }
 private static boolean charIsNotBinary(char ch) {
 return '1' < ch || ch < '0';
 }
}

Test Suite:

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
import java.util.Arrays;
import java.util.Collection;
import static org.junit.Assert.assertEquals;
@RunWith(Parameterized.class)
public class BinaryTest {
 private String input;
 private int expectedOutput;
 @Parameters
 public static Collection<Object[]> data() {
 return Arrays.asList(new Object[][]{
 {"1", 1},
 {"10", 2},
 {"11", 3},
 {"100", 4},
 {"1001", 9},
 {"11010", 26},
 {"10001101000", 1128},
 {"2", 0},
 {"5", 0},
 {"9", 0},
 {"134678", 0},
 {"abc10z", 0},
 {"011", 3}
 });
 }
 public BinaryTest(String input, int expectedOutput) {
 this.input = input;
 this.expectedOutput = expectedOutput;
 }
 @Test
 public void test() {
 Binary binary = new Binary(input);
 assertEquals(expectedOutput, binary.getDecimal());
 }
}

Reference:

http://exercism.io/exercises/java/binary/readme

• Tunaki: I assume you don't want to use Integer.parseInt(..., 2) but want to do it yourself :).
• CodeYogi: @Tunaki yes absolutely!

Answer 1

Usability

The Binary class has two kinds of usages:

• Create an instance from a binary string, to get the decimal value later
• A utility class with a utility method to convert a binary string to decimal value

Having two usages is confusing. In general, an API that lets you accomplish the same thing in multiple ways raises questions like "which way is better", which is not a very productive discussion. Just provide one way, and there will be less questions asked, less confusion.

In this particular example, it might make sense to move the converter logic to a BinaryUtils utility class. If you still want a class that contains a binary string and decimal value, then you can keep the Binary class for that purpose.

Pre-calculate when it makes sense

Getters should not calculation. When I see a method named "getSomething", I assume it simply returns the value of a field (or sometimes a defensive copy). But in this class, it calculates the decimal value. Every time it's called, again and again. Since the method always returns the same value, it would be better to calculate that value once, at construction time. And that way, the getter will naturally really return just a simple field value.

Avoid negation in names

Negation as the "Not" in charIsNotBinary may lead to double-negation in code like if (!charIsNotBinary(...)) { ... }, which can be confusing. It's better to rewrite as isBinary, and use ! where you need to negate it.

Using < and > operators is also slightly odd, feels kind of overkill for a range of size 2. This way seems simpler and more natural:

 private static boolean isBinary(char ch) {
 return ch == '0' || ch == '1';
 }

• CodeYogi: I tried to experiment a bit this time like the static class was private as expected but I exposed it as public so that the client should have choice to make an instance or not. Regarding negative method name it was also by intention I needed to see what happens if I try to avoid ! sign because it seems annoying, considering my use case I would never need the !charIsNotBinary. Overall I need to try YAGNI approach.

Answer 2

All of these are kind of nit-picky. Overall it looks pretty good.

1. It's not clear to me that the static vs instance strategy is. The entire class could just be a few static methods, or the whole thing could be instance methods based on binaryString, but I don't get the mix or the motivations that led you there.

2. Strategy on access modifiers also isn't clear. Why is charIsNotBinary private but toDecimal public?

3. The variable names could be clearer. If you end up with variables 2 letters or less you might want to reconsider.

4. charIsNotBinary reads a little strangely to me, but maybe there's a reason you did it that way instead of just straight equality comparison?

5. base0 could be a constant.

6. Seems like you could fail faster (instead of doing work then giving up in the loop if you encounter a offending char)... that said, anything I can think if would be less performant (but maybe a little more transparent). Also, I'd probably throw an Exception here rather than returning 0.

• CodeYogi: Can you put up some example? also remember my code has some contract guided by the test case given.

Answer 3

Numeric \$\neq\$ Decimal

Your code (and perhaps the exercise description) seems to confuse decimal with numeric. An integer is an abstract mathematical concept. A binary representation of an integer is one of many possible ways to represent its value. Decimal representation is just another way. So is spelling out the English word (eg "thirteen").

Talking about an int as "decimal" is wrong. Internally, it is not represented in decimal but binary form. But what you probably care about is not the internal representation but the value. That is, what you want is not a function toDecimal but toNumber that gives you the numeric value of an integer's string representation in binary format.

On the other hand, if the exercise is to be taken literally and you're really supposed to produce a "decimal" form, then you should probably return a string of digits from 0 to 9 that represents the same value as the input string. I'm not sure whether this is expected from you and having a "binary → decimal" function would also be poor design as you would then need \$n^2\$ functions to convert between \$n\$ representations when you could have done with \2ドルn\$ using two families of "some representation → numeric value" and "numeric value → some representation" functions. (And \$n\$ could quickly grow considerably if you think about binary, octal, decimal, hexadecimal, ...)

Error Handling

If your function is given invalid input, it returns 0. While there is infamous precedence for this in the C standard library, it is nothing to feel good about. For the users of your function, it probably matters whether your function successfully parsed "0" to 0 or failed to parse nonsensical input. Using a plausible result to indicate error is a bad idea in general. You have a few options to report errors in a more useful way. (In order of my personal preference.)

• throw an exception (eg NumberFormatException) on error.

  Good because it cannot be ignored and doesn't clutter client code where failure is not expected. Not so good if you expect many failures because throwing and catching exceptions is relatively expensive and verbose.

• Change the return type to Optional<Integer>.

  Good if you expect many failures because it better communicates that failure is a normal result. Not so good for performance as you have to allocate two objects and the function becomes a little more verbose to use in cases where failure is not expected.

• Since you're not supporting negative numbers, you could use –1 to unambiguously indicate error.

  Best option for performance. Poor option for usability as errors can easily be ignored by accident. It also paints you into a corner where you cannot enhance your function later to also support negative numbers.

• Change the return type to Integer and use null to indicate error.

  Poor man's version of using an Optional. Less performance overhead as only a single object has to be allocated (and that might not be needed since the JVM caches Integers) but also less clear about your intent.

Unit Tests for Corner Cases

I would add unit tests for the corner case "0" (you already have "1") as well as "0000" and "1111". (The four digits are arbitrary, the point is to test a few repetitions of the same digit.)

If you're serious about the quality of your implementation, also test Integer.MAX_VALUE (that is, a string of 31 consecutive 1s preceded by zero or more leading 0s). And as we are at it, think about what your function should do if it is given inputs that are too large to be represented in an int.