Code fragment where I am repeating myself

Question

I have two methods here that does the same thing. get the user input and validate them. if they are enter a letter program will prompt him again to enter a number after that if its a number it will compare it to the bound given by the program or the user. it is working fine. but can anybody help me refactor this code? since it would be really helpful if I could disect the validations into smaller pieces of code or method

public void addToQuestionToQbank(){
        this.askForCategory();
        System.out.println("Add Questions[1]Multiple Choice[2] Identification: ");
        int choice;
        boolean notValid = true;
        do{
            while(!input.hasNextInt()){
                System.out.println("Numbers Only!");
                System.out.print("Try again: ");
                input.nextLine();
            }
                notValid = false;
                choice = input.nextInt();
        }while(notValid);

        while(choice > 2){
            System.out.println("Out of bounds");
            choice = input.nextInt();
            input.nextLine();
        }
    }


public Difficulty askForDifficulty(){
        System.out.println("Difficulty For This Question:\n1)Easy\n2)Medium\n3)Hard\nChoice: ");
        int choice = 0;

        boolean notValid = true;
        do{
            while(!input.hasNextInt()){
                System.out.println("Numbers Only!");
                System.out.print("Try again: ");
                input.nextLine();
            }
                notValid = false;
                choice = input.nextInt();
        }while(notValid);

        while(choice > diff.length){
            System.out.println("Out of bounds");
            choice = input.nextInt();
            input.nextLine();
        }
        System.out.println(diff[choice -1]);
       return diff[choice -1];
    }

Answer 1

There are 2 problems that must be addressed first. Where did diff.choice come from, what is that? Second. What is the this.askForCategory() call? It sounds like exactly what addToQuestionToQbank() is doing. addToQuestionToQbank() does not do what it says, so I wonder what the real adding method looks like ....

Anyway, I would expect the calling code to look something like this:

public void newQuestion () {
    Question myQuestion = new Question();

    myQuestion.Question = this.askForQuestion();
    myQuestion.Answer = this.askForAnswer ();

    // here is where we call your code, refactored.
    myQuestion.category = this.multiChoicePrompt("Add Questions[1]Multiple Choice[2] Identification: ", 2);
    myQuestion.Difficulty = this.multiChoicePrompt("Difficulty For This Question:\n1)Easy\n2)Medium\n3)Hard\nChoice: " ,3);

    this.addQuestionToQbank(myQuestion);
}

By passing the prompt as a parameter we can now refactor as you asked:

// this prompt designed specifically to pick from multiple choices
// note how the method name makes that comment irrelevant.
public int multiChoicePrompt(String askUser, int responseMaxValue) {
    System.out.println(askUser);

    int choice;
    boolean notValid = true;
    do{
        while(!input.hasNextInt()){
            System.out.println("Numbers Only!");
            System.out.print("Try again: ");
            input.nextLine();
        }
            notValid = false;
            choice = input.nextInt();
    }while(notValid);

    // assume numbered choices always start with "1"
    while(choice > responseMaxValue || choice < 1){
        System.out.println("Out of bounds");
        choice = input.nextInt();
        input.nextLine();
    }
    return choice;
} 

The easiest way to "extract method" might be to physically move all that code to the new method and then recompile and let the compiler errors tell you what variables it doesn't "know" - these are what you pass in as parameters.

Answer 2

it is working fine.

Are you sure about that?

Your validation will fail miserably in the following scenario:

Add Questions[1]Multiple Choice[2] Identification:
10
Out of bounds
asdf

Exception in thread "main" java.util.InputMismatchException

You need to check if the input is a number every time you process it; not just the first time. Note that your do-while loop will run for a single iteration every single time, so it is completely redundant and can be deleted (keeping the loop body).

Take a look at this:

Improved Version

Usage

askQuestion is called with a question and a valid range of answers (refactored from your two similar methods). Note that this flexible scheme allows for arbitrary ranges to be passed in.

String categoryQuestion = "Add Questions[1] Multiple Choice[2] Identification: ";
String difficultyQuestion = "Difficulty For This Question:\n1)Easy\n2)Medium\n3)Hard\nChoice: ";
int category = askQuestion(categoryQuestion, Range.between(1, 2));
int difficulty = askQuestion(difficultyQuestion, Range.between(1, 3));
// it's not clear what you are actually doing with category and difficulty, 
// so I left that up to you

Input Processing

This will properly validate the input, and I made it a bit more readable by extracting a method for getting the next integer. I have to admit I'm not a big fan of while (true) but it was the simplest and most readable thing I could come up with. Note that each method has its own, clearly defined task: the askQuestion method is all about validating the correct bounds (and doesn't know anything about your Scanner input) while the getNextInt method is all about retrieving the next integer from input.

public int askQuestion(String question, Range<Integer> responseRange) {
    System.out.println(question);
    while (true) {
        int choice = getNextInt();
        if (responseRange.contains(choice)){
            return choice;
        }
        System.out.println("Out of bounds");
    }
}

private int getNextInt(){
    while (!input.hasNextInt()) {
        System.out.println("Numbers Only!");
        System.out.println("Try again: ");
        input.nextLine();
    }
    int nextInt = input.nextInt();
    input.nextLine();
    return nextInt;
}

Range Helper Class

This class makes heavy use of generics to allow for flexible ranges to be defined (ranges are always inclusive in this implementation, i.e. from and to are included in the range). You might want to extend this class later on, but currently all we need is the convenient factory method Range.between and the check for Range.contains.

public class Range<T extends Comparable<? super T>> {
    private final T from;
    private final T to;

    public static <T extends Comparable<? super T>> Range<T> between(T start, T end){
        return new Range<T>(start, end);
    }

    private Range(T start, T end) {
        from = start;
        to = end;
    }

    public boolean contains(T value) {
        return from.compareTo(value) <= 0 && to.compareTo(value) >= 0;
    }
}

---

It's important to study your code carefully and step through it in the debugger to verify that your validation is actually working (it was not). This is a lot easier if you have written readable, clean code.