Search code examples
javareturnsubstringcomparison

Return proper on first iteration, improper on anything more, java


I'm stil learning java so i could have missed something here, but it does not look like i did, and its doing something funky i can't pin down. I'm prompting the user for an arithmatic operator or 'q' to quit. When i run the prog to test it, it works right if i enter a valid input the first time, but if i try to go 1 or more invalid input, the return always defaults to the last character in the check string even if it stops running and goes to return before it gets to the end.

//gets operation from user
public static String getOperation(Scanner scan) {
//valid input string to check against
    String ops = "+-*/=Q";
    System.out.print("Please enter an operation (+, -, /, *, = or Q to quit): ");
    String op = scan.next();
//makes sure input is only 1 character
    if (op.length() > 1) {
//recurses if not 1 character
        getOperation(scan);
    }
    op = op.toUpperCase();
    String check = "";
//begins substringing the check string one at a time for comparison against valid
    for (int i = 0; i < ops.length(); i++) {
        if (i == ops.length() - 1) {
            check = ops.substring(i);
        } else {
            check = ops.substring(i, i + 1);
        }
//exits with current check if appropriate character
        if (check.equals(op)) {
            return op;
        }
    }
//if not valid input, recurses for proper input
    getOperation(scan);
    return check;
}

When testing if i enter '/' the first time, it returns '/'. When testing if i enter 'a', 's', 'd', '/' it returns 'Q', should be '/'


Solution

  • This is not the correct way to use recursion. Let's assume a very basic invalid input: 'a', '/'.

    When the function reads 'a', it will progress completely through the for loop, and when the for loop ends, check will be equal to "Q" because that's what the last operation in the for loop will always do. Then, it will recurse:

    //if not valid input, recurses for proper input
        getOperation(scan);
        return check;
    

    and read a valid input '/'. The second level of the function will return '/', BUT, this result is discarded because you do not assign the output of the recursion to anything. The function then returns check, which will be equal to 'Q' for all invalid inputs (because it's the last value in the ops string).

    So, the solution is to return getOperation(scan); instead of calling the function then discarding the output.

    A few other notes:

    • Use ops.charAt(i) instead of ops.substring() because you are only interested in a single character at a time. This further means you can:
    • Use the primitive == to compare, which saves you a function call (this requires op to be a char, but you should do this anyways because again, you are only interested in a single character at a time. Or, an even better idea:
    • Instead of your own for loop, use the ops.contains(op) function; if it returns true, then return op directly, otherwise you know that the input is invalid