1

This question serves as a follow-up to a previous question.

I am trying to create a program that converts from one currency to another.

First, I'll post and explain the relevant code, then describe the desired outcome and some problems I came across.

do {
    try {
        invalidInput = false;

        String line = input.nextLine();
        Scanner lineScan = new Scanner(line);
        BigDecimal moneyInput = lineScan.nextBigDecimal();

        // second do-while loop that tries to emulate the outermost do-while loop
        // introduces a second set of nested scanners
        // re-prompts user for valid currency if they try a blank input
        do {
            try {
                String line2 = input.nextLine();
                Scanner lineScan2 = new Scanner(line2);
                String currency = lineScan2.next();
                if (currency.equals("USD")) {
                    // convert USD to CNY
                } else if (currency.equals("CNY")) {
                    // convert CNY to USD
                }
            } catch (NoSuchElementException e) {
                invalidInput = true;
                System.err.print("Please enter a valid CURRENCY: ");  // prompts user for currency ONLY
            }
        } while (invalidInput);

    } catch (NoSuchElementException e) {
        invalidInput = true;
        System.err.print("Please enter the VALUE followed by the CURRENCY: ");
        }
} while (invalidInput);

The outer do-while loop runs as long as invalidInput is true. At the beginning of the first try block, invalidInput is false, and it remains false unless the user enters an invalid input (e.g. blank input or non-numeric). Here, invalidInput becomes true, and the program loops back to the top of the try block after re-prompting the user for both moneyInput and currency.

Next, I wanted to find someway to check the validity of moneyInput and currency separately. The first set of nested Scanners is meant to process moneyInput. If the user inputs something invalid, it will re-prompt the user Please enter the VALUE followed by the CURRENCY:, and it will continue to do so until something valid is entered.

I then wanted to add the exact same functionality to check currency exclusively. This is the intended purpose of the second set of nested scanners (lineScan2). If a valid input is entered for moneyInput but not for currency, e.g. 100.00 abc, I'd like the program to retain the value of moneyInput, re-prompt the user for only currency, and continue to do so until a valid currency is entered (including if the user enters a blank input).

Here are some problems I'm running into:

enter image description here

The program is only reading moneyInput in the first line, instead of reading both moneyInput and currency. Secondly, for each input, the user must press [return] twice (note each of the empty lines in between each input).

The program also terminates inconsistently. In the image example above, after it finally accepts a valid moneyInput and currency and converts the value, the program does not terminate. It terminates prematurely if the moneyInput is entered first, followed by an invalid currency on a second line, like so:

enter image description here

But here, it terminates properly after a successful run (although this still isn't exactly right because it only is "successful" if moneyInput and currency are input on separate lines; ideally, the user should be able to enter both on the same line and the program prints the appropriate conversion):

enter image description here

However, one thing the program does do well is responding repeatedly to invalid (specifically, blank inputs):

enter image description here

And actually, in the case above, aside from the fact that [return] had to be entered twice when prompting for moneyInput and that it didn't terminate after a successful run, everything is working exactly as desired:

the user gets to try repeatedly until a valid input, and in the case where moneyInput is valid but currency is not, the user only has to enter a valid input for currency (i.e. they don't have to re-enter moneyInput when only the currency was invalid).

So overall I am looking for ways to modify the code above to achieve the desired results. As mentioned in the comment section of the linked question at the very top of this post, another method I tried was another do-while loop inside (in place of the inner do-while loop) to check currency, and this worked except for when a blank input was entered, which is basically problem I had at the beginning of that post (here is my code: pastebin.com/raw/CT0qjBPk and example screenshots: imgur.com/a/mjfaL).

Sorry if this post is excessively specific and lengthy. I am a beginner programmer and trying to study Java as thoroughly as possible, and this process of improving code has been of great educational value. Thanks so much for reading and providing feedback.

Community
  • 1
  • 1

1 Answers1

1

Your implementation is overly-complex because you're using input in several different places. Here's essentially the pattern I suggested in my answer to your previous question:

try (Scanner in = new Scanner(System.in)) {
  while (in.hasNextLine()) {
    String line = in.nextLine();
    doSomethingWithALineOfInput(line);
  }
}

Here's roughly what your code is doing:

boolean invalidInput = false;
try (Scanner in = new Scanner(System.in)) {
  while (in.hasNextLine()) {
    do {
      String line = input.nextLine();
      invalidInput |= doSomethingWithALineOfInput(line);
      do {
        String line2 = input.nextLine();
        invalidInput |= doSomethingWithASecondLineOfInput(line2);
      } while (invalidInput);
    } while (invalidInput);
  }
}

Notice in particular that you're calling input.nextLine() in two separate places, which makes your code hard to reason about. One of the primary goals when programming is to reduce your problem into smaller subproblems - interleaving input.nextLine() calls everywhere (let alone inside nested do-while loops) forces you to continue dealing with one big problem.

So instead of mixing your line-processing and your token-processing code together, isolate them from each other. The only thing that should interact with input is the very outer while loop. Once you've read a line you're done with input for the remainder of that iteration.

Consider something like this (notice the use of a class to contain the values as we read them in):

class PromptForMoney {
  private BigDecimal amount;
  private String currency;

  public void prompt(Scanner in) {
    System.out.print("Enter an amount of money and currency to convert: ");
    while (in.hasNextLine()) {
      try {
        processLine(in.nextLine());
        return;
      } catch (InputMismatchException | NoSuchElementException e) {
        // we use the exception message to describe the problem to the user
        // if Scanner generates exceptions with unclear messages you can
        // catch them in processLine() and throw your own with a better message.
        System.out.print("Invalid input - " + e.getMessage() + ": ");
      }
    }
    throw new NoSuchElementException(
        "No more input to read, but a valid amount or currency was not entered.");
  }

  private void processLine(String line) {
    Scanner lineScanner = new Scanner(line);
    if (amount == null) {
      // this line will raise an exception if the line is empty
      // or if it doesn't start with numerical token
      amount = lineScanner.nextBigDecimal();
    }
    if (currency == null) {
      // this line will raise an exception if the user didn't specify a currency
      String c = lineScanner.next();
      if (isValidCurrency(c)) {
        currency = c;
      } else {
        throw new InputMismatchException(c + " is not a valid currency");
      }
    }
    // if we get this far without raising an exception we've read a valid
    // amount and currency from the user.
  }
}

Notice how prompt() deals solely with lines, and processLine() deals solely with the contents of a single line. This cleanly separates the problem into smaller, easier-to-reason-about parts.

You'd use PromptForMoney like so:

public static void main(String[] args) throws IOException {
  PromptForMoney prompt = new PromptForMoney();
  try (Scanner in = new Scanner(System.in)) {
    prompt.prompt(in);
  }
  System.out.println(convert(prompt.getAmount(), prompt.getCurrency());
}

There's another separation of concerns - only main() is responsible for directly interacting with System.in. As far as PromptForMoney is concerned its Scanner could be backed by a string or a file and it would work exactly the same.


Caveat: there are some best practices I'm not emphasizing for the sake of space and simplicity (e.g. preferring final instance variables). If you're interested in improving your code quality even further I strongly suggest reading Effective Java which goes into great detail about Java design patterns.

dimo414
  • 47,227
  • 18
  • 148
  • 244
  • If I replace the final line in `main` with my method for converting CNY and USD, everything works great (see: http://pastebin.com/jJ6YdSAt). Now, how would I define that portion separately as another function named `convert`, so that whole chunk could just be replaced with `System.out.println(convert(prompt.amount));`? I tried to follow the examples of `prompt` and `processLine`, but I didn't know what format the argument list should be in. For example, I tried unsuccessfully to define `private void convert(BigDecimal prompt.amount)` – A is for Ambition Jul 24 '16 at 19:26
  • I tried defining `private static void convert( BigDecimal amount, String currency )`, like so http://pastebin.com/Vt8Sx9rV , but I got this result (http://imgur.com/a/95q06) in the IDE. So basically I am having trouble defining this portion as a new function. – A is for Ambition Jul 24 '16 at 19:44
  • 1
    Your `convert()` method is `void` - it should return something (presumably a `BigDecimal`) if you want to be able to print its result. Instead of having `println()` statements inside `convert()` the method should return the converted amount then you can print *that*. If that isn't clear enough I'd suggest posting a new question (feel free to link me to it and I'll try to answer). Comments don't work well for follow up questions. – dimo414 Jul 24 '16 at 20:28
  • oh my god i figured it out! thank you so much for being so patient and responsive, you've really helped me improve so much just from this 1 basic project :) – A is for Ambition Jul 24 '16 at 21:01
  • Hi, I just have one last question. This is inspired by your caveat of "best practices" (http://stackoverflow.com/questions/38557212/returning-null-in-a-method-that-accounts-for-multiple-conditions) – A is for Ambition Jul 24 '16 at 21:56