1
\$\begingroup\$

I made a Binary Calculator that gives the user's the decimal value of their input.

I tried to make my code as small as possible but I feel like there's still too much lines of code for such a simple program.

Did I overcomplicate this too much? Is there a way I could improve?

BinaryCalc class

import java.util.Scanner;
import java.math.*;

public class BinaryCalc {

    public static void main(String[] args) {

        Scanner input = new Scanner(System.in);
        boolean keepGoing = false;

        /*
         * Ask the user to input a binary
         */
        System.out.print("Please enter a binary number : ");
        String str = input.nextLine();
        int size = str.length();

        /*
         * Check if the user's input doesn't go over '1'
         */
        for (int s = 0; s < size; s++) {
            if (str.charAt(s) != '0' && str.charAt(s) != '1') {
                System.out.println("You didn't enter a '0' or '1'");
                size = 0;
                main(args);
                break;
            } else {
                keepGoing = true;
            }

        }

        /*
         * Use the formula [(bit * base) ^ position]
         */
        if (keepGoing = true) {
            int total = 0;
            int result = 0;
            int position = 0;

            for (int i = size - 1; i >= 0; i--) {

                int bit = Character.getNumericValue(str.charAt(i));
                result = (int) (bit * (Math.pow(2, position)));
                total += result;
                position++;
            }
            System.out.println(total);
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Fix a bug in the second condition

First of all, you have a bug in the second condition:

if (keepGoing = true) {

this expression assigns true to keepGoing and then returns true as a result. Thus, if always executes which leads to extra results showing up for invalid inputs.

You probably wanted to check the value of a keepGoing with ==:

if(keepGoing==true) {

this will work, but you should never compare boolean values with true or false. Using the value itself is more convenient:

if(keepGoing) {

Also, the name keepGoing doesn't say anything about the actual condition. It's unclear in which cases the code is going forward. I suggest renaming it to isBinary:

if(isBinary) {

Fix a bug in the control flow

You're setting isBinary to true on each valid character and you're not setting it back to false when the character is invalid:

if (str.charAt(s) != '0' && str.charAt(s) != '1') {
    System.out.println("You didn't enter a '0' or '1'");
    size = 0;
    main(args);
    break;
} else {
    isBinary = true;
}

So, if the input string starts with a valid character (e.g. 119) the code will set isBinary to true which will again lead to extra results.

Instead, you can set it to false on an invalid character:

boolean isBinary = true;
if (str.charAt(s) != '0' && str.charAt(s) != '1') {
    System.out.println("You didn't enter a '0' or '1'");
    size = 0;
    main(args);
    isBinary = false;
    break;
} 

The rest of this method executes only if isBinary is true, so, at this point, you can just return from the method:

if (str.charAt(s) != '0' && str.charAt(s) != '1') {
    System.out.println("You didn't enter a '0' or '1'");
    main(args);
    return;
}

after that, you don't need to check for isBinary and set size to zero. The code will get to calculation only if the input doesn't contain invalid characters

Code so far:

/*
* Check if the user's input doesn't go over '1'
*/
for (int s = 0; s < size; s++) {
    if (str.charAt(s) != '0' && str.charAt(s) != '1') {
        System.out.println("You didn't enter a '0' or '1'");
        main(args);
        return;
    }
}

/*
* Use the formula [(bit * base) ^ position]
*/
int total = 0;
int result = 0;
int position = 0;

for (int i = size - 1; i >= 0; i--) {
    int bit = Character.getNumericValue(str.charAt(i));
    result = (int) (bit * (Math.pow(2, position)));
    total += result;
    position++;
} 

Additional clean-up

result is only used once, let's move it closer to usage:

int result = (int) (bit * (Math.pow(2, position)));
total += result;

The size is not updated anymore and seems unnecessary: let's replace it with actual length:

for (int s = 0; s < str.length(); s++) {
...
for (int i = str.length() - 1; i >= 0; i--) {

Introduce helper methods

You have two independent blocks of code: the first one reads and validates the input, the second converts it to decimal. You can move validation and calculation into separate methods:

/**
 * Check if the user's input doesn't go over '1'
 */
private static boolean isBinary(String str) {
    for (int i = 0; i < str.length(); i++) {
        if (str.charAt(i) != '0' && str.charAt(i) != '1') {
            return false;
        }
    }
    return true;
}

/**
 * Use the formula [(bit * base) ^ position]
 */
private static int toDecimal(String str) {

    int total = 0;
    int position = 0;
    for (int i = str.length() - 1; i >= 0; i--) {
        int bit = Character.getNumericValue(str.charAt(i));
        int result = (int) (bit * (Math.pow(2, position)));
        total += result;
        position++;
    }
    return total;
}

As you can see, I moved comments as well and turned them into javadoc format.

Now, the main method control flow is easier to read and understand:

if (!isBinary(str)) {
   System.out.println("You didn't enter a '0' or '1'");
   main(args);
   return;
}
System.out.println(toDecimal(str));

Check for an empty string

It seems like new isBinary method introduced a bug: it will return true for an empty string. We can go ahead and add an extra condition:

if (str == null || str.isEmpty()) {
    return false;
}  

Get rid of recursion

The recursive call to a main breaks the flow and is hard to read and understand. Recursion is usually used to change the scope of a method. If you just want to repeat the same action multiple times, you can use a loop:

System.out.println("Please enter a binary number : ");
String str = input.nextLine();
while (!isBinary(str)) {
    System.out.println("You didn't enter a '0' or '1'");
    System.out.println("Please enter a binary number : ");
    str = input.nextLine();
}

and move the duplicated lines into a separate method:

public static void main(String[] args) {
    Scanner input = new Scanner(System.in);
    String str = promptInput(input);
    while (!isBinary(str)) {
        System.out.println("You didn't enter a '0' or '1'");
        str = promptInput(input);
    }
    System.out.println(toDecimal(str));
}

/**
 * Ask the user to input a binary
 */
private static String promptInput(Scanner input) {
    System.out.println("Please enter a binary number : ");
    return input.nextLine();
}

Consider alternatives

Now the control flow is relatively clear we can think about improving the actual calculation in toDecimal and isBinary

A quick research on StackOverflow returns multiple questions on this topic, to name a few:

We can use suggestions from there and replace the calculation with shorter alternatives:

/**
 * Check if the user's input doesn't go over '1'
 */
private static boolean isBinary(String str) {
    return str.matches("[01]+");
}

/**
 * Use the formula [(bit * base) ^ position]
 */
private static int toDecimal(String str) {
    return Integer.parseInt(str, 2);
}

Now, both methods are self-explanatory one-liners and each of them is used only once. You can keep them or you can inline both of them:

String str = promptInput(input);
while (!str.matches("[01]+")) {
    System.out.println("You didn't enter a '0' or '1'");
    str = promptInput(input);
}
System.out.println(Integer.parseInt(str, 2));

One can argue that the code is easier to read with method calls because they show the intent. But at the end, it's a matter of taste/experience.

Integer overflow

The code doesn't handle the case when the input number doesn't fit into the range of Integer. Original code will silently return a wrong value. Integer.parseInt will throw an exception. You'll need to think about that.

\$\endgroup\$
  • 1
    \$\begingroup\$ Wow @default locale! These are some amazing advices, thank you! I'm currently a beginner with Java and I've learned quite some things from your well written advices. Thanks again! \$\endgroup\$ – Crypto Aug 25 '17 at 5:57
  • \$\begingroup\$ there's only one thing that I didn't understand. What do you mean by "As you can see, I moved comments as well and turned them into javadoc format."? \$\endgroup\$ – Crypto Aug 25 '17 at 6:05
  • 1
    \$\begingroup\$ @Crypto I moved inline comments (e.g. Check if the user's input doesn't go over) to respective methods and added an extra asterisk /** required by javadoc. Javadoc is a documentation generator for Java. IDEs use it to display help information. \$\endgroup\$ – default locale Aug 25 '17 at 6:09
  • 1
    \$\begingroup\$ Okay I see, I'll try to read more and learn how to make my code look more professional! \$\endgroup\$ – Crypto Aug 25 '17 at 6:12

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.