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.
