2
\$\begingroup\$

I have two similar methods for setting dimensions of a rectangle. At first, I had them combined into one longer method that set width and length. I read about keeping code modular that performs one task at a time. So split it into two methods. Also, I have separate methods for displaying messages. Am I going overboard breaking methods into smaller ones?

private void userInputWidth() {
    Scanner reader = new Scanner(System.in);
    double loopCondition = 0;
    while (loopCondition <= 0) {
        displayEnterWidth();
        double nextDouble = reader.nextDouble();
        loopCondition = nextDouble;
        if (nextDouble <= 0) {
            displayErrorMessage();
        } else {
            this.rectangleWidth = nextDouble;
            break;
        }
    }
}

private void displayEnterWidth() {
    System.out.println("Please enter the width of the rectangle ");
}

2nd Method

private void userInputLength() {
    Scanner reader = new Scanner(System.in);
    double loopCondition = 0;
    while (loopCondition <= 0) {
        displayEnterLength();
        double nextDouble = reader.nextDouble();
        loopCondition = nextDouble;
        if (nextDouble <= 0) {
            displayErrorMessage();
        } else {
            this.rectangleLength = nextDouble;
            break;
        }
    }
}

private void displayEnterLength() {
    System.out.println("Please enter the length of the rectangle");
}
\$\endgroup\$
2
\$\begingroup\$

Breaking methods down into less than five lines is generally a sign that you have gone a bit too far (But not always!). This Software Engineering post is a good overview.

Instead of breaking your methods into userInputWidth and userInputHeight, I would recommend creating a helper method named getPositiveInput that accepts a prompt string and using that instead.

For example:

private double getPositiveInput(String prompt) {
    Scanner reader = new Scanner(System.in);
    double result;
    do {
        System.out.println(prompt);
        result = reader.nextDouble();
        if (result <= 0) {
            System.out.println("Enter a number greater than zero.");
        }
    } while (result <= 0)
    return result;
}

This makes it possible to reduce the duplication of your code without breaking your methods down so far that you have to have one for every property on your class.

The trick is what the definition of "one task" is. Deciding what each method should do to keep the code modular is made much easier by experience. I've found the following guidelines help.

  1. Describe the method out loud, if you say "and" or "then" more than once, the method is probably doing too much and should be split up.
  2. If you say the same thing for more than one method, extract the code that does that thing into it's own method.
  3. If you catch yourself copy/pasting code stop and put it in a method.

Here's an example:

Given the following code (combined your methods to get what I presume was the original method), here's how I would describe the method.

private void userInputDimensions() {
    Scanner reader = new Scanner(System.in);
    double loopCondition = 0;
    while (loopCondition <= 0) {
        System.out.println("Please enter the width of the rectangle ");
        double nextDouble = reader.nextDouble();
        loopCondition = nextDouble;
        if (nextDouble <= 0) {
            System.out.println("Enter a number greater than zero.");
        } else {
            this.rectangleWidth = nextDouble;
            break;
        }
    }

    loopCondition = 0;
    while (loopCondition <= 0) {
        System.out.println("Please enter the length of the rectangle");
        double nextDouble = reader.nextDouble();
        loopCondition = nextDouble;
        if (nextDouble <= 0) {
            System.out.println("Enter a number greater than zero.");
        } else {
            this.rectangleLength = nextDouble;
            break;
        }
    }
}
  • Prompt the user for the width of the rectangle until they enter a number greater than zero
  • Set the rectangleWidth to the number
  • Prompt the user for the length of the rectangle until they enter a number greater than zero
  • Set the rectangleLength to the number

Notice that the duplicate "code" here is the prompt and validation input. By contrast, consider the following code using the getPositiveInput method.

private void userInputDimensions() {
    this.rectangleWidth = this.getPositiveInput("Please enter the width of the rectangle");
    this.rectangleLength = this.getPositiveInput("Please enter the length of the rectangle");
}

This method can be described by "set the width and length of the rectangle to the user input" and does not need to be broken down further.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

I think you're going overboard with this. Moreover, you have duplicate code right there. Here's how I think it should be done:

Main

public class ShapeDriver{
    public static void main(String[] args){
        System.out.println("Enter length of rectangle: ");
        float length = getInputsFromUserUntilAValidInput();
        System.out.println("Enter width of rectangle: ");
        float width = getInputsFromUserUntilAValidInput();
        Rectangle rectangle = new Rectangle(length, width);

        //Do whatever you want with the rectangle here lol
    }

    private float getInputsFromUserUntilAValidInput(){
        Scanner inputReader = new Scanner(System.in);
        float input = -1;
        while(isInvalid(input)){
            input = inputReader.nextFloat();
            if(isInvalid(input))
                displayErrorMessage();
        }
        return input;
    }

    private boolean isInvalid(float input){
        return input <= 0;
    }
}

Rectangle

public class Rectangle{
    private float length;
    private float width;

    public Rectangle(int length, int width){
        this.length = length;
        this.width = width;
    }

    public float getLength(){
        return length;
    }

    public float getWidth(){
        return width;
    }
}
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Your reworked code abandons the error handling the OP includes presumably for good reason. This probably isn't a good idea. \$\endgroup\$ – Gerrit0 Oct 13 '17 at 3:44
  • \$\begingroup\$ Oh wait, I didn't see that part. Lemme rework my code. \$\endgroup\$ – Rocket Pingu Oct 13 '17 at 3:52
  • \$\begingroup\$ while(isInvalid(input)) is very clumsy. Not only do you artificially initialize input to an invalid value to force it to enter the loop the first time, you also have a redundant isInvalid(input) rest inside the loop. You would be better off with a do-while loop. \$\endgroup\$ – 200_success Oct 13 '17 at 5:24
  • \$\begingroup\$ Either way, you'd have to check if the input is valid twice: one for inside the loop; one for the loop condition just like the accepted answer. \$\endgroup\$ – Rocket Pingu Oct 13 '17 at 5:32

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.