5
\$\begingroup\$

For a basic program I am writing to help learn, I am at the step where I should be cleaning up the messy code and trying to condense my "main" to 5-20 lines by recalling other methods that repeat themselves. I simplified one section perfectly where the user enters their score and the score is recalled from another method, but now I can't quite figure out how to go from here for another section.

This is a program I made for a golfer entering his score into a computer to calculate his single round handicap. The code prompts the user for his/her name, what color tees were played, and the score for those tees. It will then output the handicap differential for the round.

Welcome to Medford Village CC Single Round Handicap Calculator!
Please type your Name: Sam
Thank you Sam. Did you play the White, Blue or Black tees?
Tees Played: white
Please enter your white tee round score: 89

Sam, Your Handicap Differential for this round is: 16.64. This rounds to 16.6!

Thank you, Goodbye!

ISSUE - I know to look for repetitive sections of code which means they can probably be simplified to another method to be recalled. I ask the user to enter which color tees they played from. So I have a while loop and different sections of if's within them for depending on which color tee the played. If they don't enter a correct color tee, it loops back and asks again. My issue is that each color tee has its own "final" data for course rating and course slope, and this makes the calculation differ. If anyone has any insight on how to clean up the individual if statements, without getting too complicated, I would really appreciate it. The code runs exactly how I want it to with the code below, I am just looking to simplify it.

public static void main(String[] args) {
    Scanner scanner = new Scanner(System.in);

    final short blackTeeSlope = 146;
    final float blackTeeRating = 74.6F;
    final short blueTeeSlope = 139;
    final float blueTeeRating = 72.4F;
    final short whiteTeeSlope = 129;
    final float whiteTeeRating = 70.0F;
    final short goldTeeSlope = 133;
    final float goldTeeRating = 71.3F;
    String input = "";

    System.out.println("Welcome to Medford Village CC Single Round Handicap Calculator!");


    System.out.print("Please type your Name: ");
    String name = scanner.nextLine().trim();

    System.out.println("Thank you " + name + ". Did you play the White, Blue or Black tees?");

    while (true) {
        System.out.print("Tees Played: ");
        String teesPlayed = scanner.nextLine().trim().toLowerCase();
        if (teesPlayed.equals("black")) {
            short userScore = (short)readNumber("Please enter your black tee round score: ", 55, 300);

            double handicapDifferential = (userScore - blackTeeRating) * 113 / blackTeeSlope;

            double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

            String formattedDifferential = String.format("%.02f", handicapDifferential);

            System.out.println();
            System.out.println(name + ", Your Handicap Differential for this round is: " + formattedDifferential + ". This rounds to " + rounded + "!");
            break;
        }
        if (teesPlayed.equals("blue")) {
            short userScore = (short)readNumber("Please enter your blue tee round score: ", 55, 300);

            double handicapDifferential = (userScore - blueTeeRating) * 113 / blueTeeSlope;

            double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

            String formattedDifferential = String.format("%.02f", handicapDifferential);

            System.out.println();
            System.out.println(name + ", Your Handicap Differential for this round is: " + formattedDifferential + ". This rounds to " + rounded + "!");
            break;
        }
        if (teesPlayed.equals("white")) {
            short userScore = (short)readNumber("Please enter your white tee round score: ", 55, 300);

            double handicapDifferential = (userScore - whiteTeeRating) * 113 / whiteTeeSlope;

            double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

            String formattedDifferential = String.format("%.02f", handicapDifferential);

            System.out.println();
            System.out.println(name + ", Your Handicap Differential for this round is: " + formattedDifferential + ". This rounds to " + rounded + "!");
            break;
        }
        if (teesPlayed.equals("gold")) {
            short userScore = (short)readNumber("Please enter your gold tee round score: ", 55, 300);

            double handicapDifferential = (userScore - goldTeeRating) * 113 / goldTeeSlope;

            double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

            String formattedDifferential = String.format("%.02f", handicapDifferential);

            System.out.println();
            System.out.println(name + ", Your Handicap Differential for this round is: " + formattedDifferential + ". This rounds to " + rounded + "!");
            break;
        }
        System.out.println("Please Enter Black, Blue, Gold or White.");
    }

    System.out.println();
    System.out.println("Thank you, Goodbye!");
}


public static double readNumber (String prompt, int min, int max){
    Scanner scanner = new Scanner(System.in);
    short value;
    while (true){
        System.out.print(prompt);
        value = scanner.nextShort();
        if (value >=min && value <=max)
            break;
        System.out.println("Please enter an amount between " + min +" and " + max + ".");
    }
    return value;
}
\$\endgroup\$
5
\$\begingroup\$

I have some suggestions for your code.

1) The input variable is unused.

2) Since the tees variables are hard-coded and have two values, I suggest that you use an Enum to hold the values. It will make the code easier to read and remove the variables from the method.

Tees.java

public enum Tees {
    BLACK(146, 74.6F),
    BLUE(139, 72.4F),
    WHITE(129, 70.0F),
    GOLD(133, 71.3F);

    private final int slope;
    private final float rating;

    Tees(int slope, float rating) {
        this.slope = slope;
        this.rating = rating;
    }

    public int getSlope() {
        return slope;
    }

    public float getRating() {
        return rating;
    }
}

Main.java


//[...]
double handicapDifferential = (userScore - Tees.BLACK.getRating()) * 113 / Tees.BLACK.getSlope();
//[...]

3) Since there are multiple instances of java.util.Scanner, I suggest that you create a constant for it; so you can use it everywhere.

    public static final Scanner SCANNER = new Scanner(System.in);

4) For the method readNumber, I suggest that you rename it to readAnswersAsNumber; since this method to more than read.

5) In the same idea of the method readNumber, I suggest that you make a new method to read java.lang.String answers. This will save 2 lines per questions / answers.

Before

System.out.print("Please type your Name: ");
String name = SCANNER.nextLine().trim();

After

public static void main(String[] args) {
        //[...]
    String name = readAnswersAsString("Please type your Name: ");
        //[...]
}

private static String readAnswersAsString(String question) {
    System.out.print(question);
    return SCANNER.nextLine().trim();
}

6) Instead of concatenating a string in the java.io.PrintStream#println, you can use the java.io.PrintStream#printf and use the java string templates. But, the only downside, it has the same effect as the java.io.PrintStream#print method, it doesn't add a new line, so you have to add it to the template ('\n' or '%n').

'\n' vs '%n' on stackoverflow

System.out.printf("Thank you %s. Did you play the White, Blue or Black tees?%n", name);

7) When checking the color of the tees, since we used an Enum earlier, we can use it instead of the string.

String teesPlayed = readAnswersAsString("Tees Played: ").toUpperCase();

if(Tees.BLACK.name().equals(teesPlayed)) {
    //[...]
}

8) Instead of using only if, I suggest that you use the if-else-if, since there's only one color each time.


if (Tees.BLACK.name().equals(teesPlayed)) {
    //[...]
} else if(Tees.BLUE.name().equals(teesPlayed)) {
    //[...]
}

9) Since the score logic is similar in all colors, I suggest that you extract it in a method.

public static void main(String[] args) {
    if (Tees.BLACK.name().equals(teesPlayed)) {
        handleScore(name, "Please enter your black tee round score: ", Tees.BLACK);
        break;
    } else if (Tees.BLUE.name().equals(teesPlayed)) {
        handleScore(name, "Please enter your blue tee round score: ", Tees.BLUE);
        break;
    } else if (Tees.WHITE.name().equals(teesPlayed)) {
        handleScore(name, "Please enter your white tee round score: ", Tees.WHITE);
        break;
    } else if (Tees.GOLD.name().equals("gold")) {
        handleScore(name, "Please enter your gold tee round score: ", Tees.GOLD);
        break;
    }
}

private static void handleScore(String name, String question, Tees tees) {
    short userScore = (short) readAnswersAsNumber(question, 55, 300);

    double handicapDifferential = (userScore - tees.getRating()) * 113 / tees.getSlope();
    double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

    System.out.printf("%n%s, Your Handicap Differential for this round is: %.02f. This rounds to %.2f!", name, handicapDifferential, rounded);
}

Refactored code

public static final Scanner SCANNER = new Scanner(System.in);

public static void main(String[] args) {
    System.out.println("Welcome to Medford Village CC Single Round Handicap Calculator!");
    String name = readAnswersAsString("Please type your Name: ");
    System.out.printf("Thank you %s. Did you play the White, Blue or Black tees?%n", name);

    while (true) {
        String teesPlayed = readAnswersAsString("Tees Played: ").toUpperCase();

        if (Tees.BLACK.name().equals(teesPlayed)) {
            handleScore(name, "Please enter your black tee round score: ", Tees.BLACK);
            break;
        } else if (Tees.BLUE.name().equals(teesPlayed)) {
            handleScore(name, "Please enter your blue tee round score: ", Tees.BLUE);
            break;
        } else if (Tees.WHITE.name().equals(teesPlayed)) {
            handleScore(name, "Please enter your white tee round score: ", Tees.WHITE);
            break;
        } else if (Tees.GOLD.name().equals(teesPlayed)) {
            handleScore(name, "Please enter your gold tee round score: ", Tees.GOLD);
            break;
        }

        System.out.println("Please Enter Black, Blue, Gold or White.");
    }

    System.out.println();
    System.out.println("Thank you, Goodbye!");
}

private static void handleScore(String name, String question, Tees tees) {
    short userScore = (short) readAnswersAsNumber(question, 55, 300);

    double handicapDifferential = (userScore - tees.getRating()) * 113 / tees.getSlope();
    double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

    System.out.printf("%n%s, Your Handicap Differential for this round is: %.02f. This rounds to %.2f!", name, handicapDifferential, rounded);
}

private static String readAnswersAsString(String question) {
    System.out.print(question);
    return SCANNER.nextLine().trim();
}

public static double readAnswersAsNumber(String prompt, int min, int max) {
    short value;
    while (true) {
        System.out.print(prompt);
        value = SCANNER.nextShort();
        if (value >= min && value <= max)
            break;
        System.out.println("Please enter an amount between " + min + " and " + max + ".");
    }
    return value;
}

Edit - Method to fetch the tee color directly

I suggest that you create a method that return directly the Tees enum.

private static Tees readTeeColor() {
    while (true) {
        String teeColor = readAnswersAsString("Tees Played: ").toUpperCase();

        try {
            return Tees.valueOf(teeColor);
        } catch (IllegalArgumentException ex) {
            System.out.println("Please Enter Black, Blue, Gold or White.");
        }
    }
}

With this modification, you need to change the condition and remove the while loop, since the readTeeColor will now handle the invalid choice by looping indefinitely.

System.out.printf("Thank you %s. Did you play the White, Blue or Black tees?%n", name);

Tees teesPlayed = readTeeColor();

if (Tees.BLACK.equals(teesPlayed)) {
    handleScore(name, Tees.BLACK);
} else if (Tees.BLUE.equals(teesPlayed)) {
    handleScore(name, Tees.BLUE);
} else if (Tees.WHITE.equals(teesPlayed)) {
    handleScore(name, Tees.WHITE);
} else if (Tees.GOLD.equals(teesPlayed)) {
    handleScore(name, Tees.GOLD);
}

System.out.println();

Edit - Code clean-up

Has @roland-illig suggested in the comment, you can remove the similar message very easily.

1) Remove the parameter question of the method handleScore.

private static void handleScore(String name, Tees tees) {
    //[...]
}

2) Use the template in the method handleScore.

private static void handleScore(String name, Tees tees) {
        //[...]
    short userScore = (short) readAnswersAsNumber(String.format("Please enter your %s tee round score: ", tees.name().toLowerCase()), 55, 300);
        //[...]
}

3) Since the handleScore is now generic, you don’t need the if-else checks anymore.

Redacted Code

public static final Scanner SCANNER = new Scanner(System.in);

public static void main(String[] args) {
    System.out.println("Welcome to Medford Village CC Single Round Handicap Calculator!");
    String name = readAnswersAsString("Please type your Name: ");
    System.out.printf("Thank you %s. Did you play the White, Blue or Black tees?%n", name);

    handleScore(name, readTeeColor());

    System.out.println();
    System.out.println("Thank you, Goodbye!");
}

private static void handleScore(String name, Tees tees) {
    short userScore = (short) readAnswersAsNumber(String.format("Please enter your %s tee round score: ", tees.name().toLowerCase()), 55, 300);

    double handicapDifferential = (userScore - tees.getRating()) * 113 / tees.getSlope();
    double rounded = Math.round(handicapDifferential * 10.0) / 10.0;

    System.out.printf("%n%s, Your Handicap Differential for this round is: %.02f. This rounds to %.2f!", name, handicapDifferential, rounded);
}


private static Tees readTeeColor() {
    while (true) {
        String teeColor = readAnswersAsString("Tees Played: ").toUpperCase();

        try {
            return Tees.valueOf(teeColor);
        } catch (IllegalArgumentException ex) {
            System.out.println("Please Enter Black, Blue, Gold or White.");
        }
    }
}

private static String readAnswersAsString(String question) {
    System.out.print(question);
    return SCANNER.nextLine().trim();
}

public static double readAnswersAsNumber(String prompt, int min, int max) {
    short value;
    while (true) {
        System.out.print(prompt);
        value = SCANNER.nextShort();
        if (value >= min && value <= max)
            break;
        System.out.println("Please enter an amount between " + min + " and " + max + ".");
    }
    return value;
}
```
\$\endgroup\$
  • 1
    \$\begingroup\$ To make this answer perfect, you could describe how to get rid of the 4 very similar string literals that are passed to handleScore. \$\endgroup\$ – Roland Illig Jan 12 at 8:58
  • \$\begingroup\$ I don't find enum to be a good solution for managing the tee information. The tees are likely to be data that changes, so hard coding them makes maintenance difficult. I would define them as a data structure that is populated at run time. This also prevents you from hard coding the tees to other methods forcing you to write more modular code. \$\endgroup\$ – TorbenPutkonen Jan 12 at 10:22
  • \$\begingroup\$ @roland-illig Done, added in the Edit - Code clean-up section, thanks! \$\endgroup\$ – Doi9t Jan 12 at 17:36
0
\$\begingroup\$

There is already a good answer from @Doi9t. I was too slow to publish mine but can still be useful.


"My issue is that each color tee has its own "final" data for course rating and course slope, and this makes the calculation differ". Okay, do you know the the strategy pattern ?

In computer programming, the strategy pattern is a behavioral software design pattern that enables selecting an algorithm at runtime. [..] Strategy lets the algorithm vary independently from clients that use it [..] Deferring the decision about which algorithm to use until runtime allows the calling code to be more flexible and reusable.

-- https://en.wikipedia.org/wiki/Strategy_pattern

However, from what I see, the calculation is always the same, it differ only on the rating and slope. So this is not really a strategy because there is no different algorithm. But this concept of color is still important. There is no interest to create one abstraction when only the values are different, so let's create one Color(rating, slope) class for it. By doing that you will remove the almost duplicated constants, at this time you can create one enumeration for that[1].

The computed handicap is also an important concept, why not extracting one class for that. This will also remove a lot of duplication. And since, the handicap require a tee color to be computed you can create on factory method on the color:

enum Colors implements Color {
  BLACK(146, 74.6f),
  BLUE(139, 72.4f),
  WHITE(129, 70.0f),
  GOLD(133, 71.3f);

  private final short slope;
  private final float rating;

  Colors(short slope, float rating) {
    this.rating = rating;
    this.slope = slope;
  }

  HandicapDifferential handicap(short score) {
    return new HandicapDifferential(score, rating, slope);
  }
}

Now you have a model that can be tested. But there is still some duplication in the presentation. You can still create one decorator class on Scanner with a couple of method tailored to your needs:

class ConsoleView {
  private final Scanner scanner;
  Presentation(InputStream in) {
    this.scanner = new Scanner(in);
  }

  String getName() {
    System.out.print("Please type your Name: "); 
    return scanner.nextLine().trim();
  }

  String getTeeColor() // ...

  short getScore() // ...

  void print(HandicapDifferential handicap) // ...

}

So you have some classes for the model and one for handling interactions with the user. You just miss one to coordinate both. This is what your main method will do. But most of the time, mainly for testing, you may want to move that "flow" to one instance instead of the main method.

class SingleRoundHandicapCalculator {

    public static void main(String[] args) {
        new SingleRoundHandicapCalculator(new ConsoleView(System.in))
        .run();
    }

    // ~ ----------------------------------------------------------------- ~ //

    private final ConsoleView view;

    SingleRoundHandicapCalculator(final ConsoleView view) {
        this.view = view;
    }

    void run() {
        view.greet();
        String color = view.getColor();
        short score = view.getScore();

        HandicapDifferential handicap = Color.valueOf(color.toUpperCase()).handicap(score);
        view.show(handicap);
    }
}

As you can see the "name" is removed. This is the power of this kind of code, where you can hide some presentation requirement into the view itself.

[1] An enumeration is the ideal candidate to group a set of identical "classes" where only the attributes vary. To have a more flexible model you can create one interface to represents the tee color so that you can "generate" them at runtime later from any datasource if needed.

\$\endgroup\$

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.