5
\$\begingroup\$

I am not sure how to properly make this readable. I have been trying to find examples online but I cannot find any that are specific to this simple program.

import java.util.Scanner;

public class KifahUnit3 {

    public static void main(String[] args) {

        Scanner stdIn = new Scanner(System.in);
        String first;  //first name
        String last;  //last name
        String full;  //full name
        Boolean nameMatch = false;
        int score1;
        int score2;
        int score3;
        int score4;
        int score5;
        int totalScore;
        int x;
        double scoreAverage;
        System.out.print("Please enter your first name: ");
        first = stdIn.next();
        System.out.print("Please enter your last name: ");
        last = stdIn.next();
        System.out.println(first + " " + last);
        nameMatch = first.equals(last);
        if (nameMatch == true) {
            System.out.println(
                    "Hello, " + first + " " + last + " "
                    + "your first and last name are the same.");
        } else {
            System.out.println(
                    "Hello, " + first + " " + last + " "
                    + "your first and last name are different.");
            full = first + last;
            System.out.println("The length of your first name is: " + first.length());
            System.out.println("The length of your full name is: " + full.length());
            System.out.println("Your initials are " + first.charAt(0) + last.charAt(0));

            System.out.println("Please enter your test scores");
            System.out.print("Score 1= ");
            score1 = stdIn.nextInt();
            System.out.print("Score 2= ");
            score2 = stdIn.nextInt();
            System.out.print("Score 3= ");
            score3 = stdIn.nextInt();
            System.out.print("Score 4= ");
            score4 = stdIn.nextInt();
            System.out.print("Score 5= ");
            score5 = stdIn.nextInt();
            scoreAverage = (score1 + score2 + score3 + score4 + score5) / 5;
            System.out.println(
                    "Score 1= " + score1 + "; " + "Score 2= " + score2 + "; " + "Score 3= "
                    + score3 + ";  " + "Score 4= " + score4 + "; " + "Score 5= " + score5);
            totalScore = score1 + score2 + score3 + score4 + score5;
            x = 10;
            System.out.println("Total= " + totalScore);
            System.out.println("Average: " + (scoreAverage + x));
            System.out.println("Ooops! Made a mistake. Your average is:  " + scoreAverage);

        }
    } // end main
} // end class KifahUnit3
\$\endgroup\$
  • 1
    \$\begingroup\$ (giving it a shot) Welcome to CR! Are you on Java 8? \$\endgroup\$ – h.j.k. Sep 6 '15 at 14:05
  • \$\begingroup\$ No, I am using Java SE 7 \$\endgroup\$ – Kifah Sep 6 '15 at 15:03
  • \$\begingroup\$ Why are you adding 10 to the average before printing it? \$\endgroup\$ – 200_success Sep 6 '15 at 17:10
  • 1
    \$\begingroup\$ Why does the first name being the same as the last name trigger a special case? "Mohammed Mohammed" is a rather common name. \$\endgroup\$ – 200_success Sep 6 '15 at 17:15
12
\$\begingroup\$

Firstly - Java Code Convention

Read the Java code convention, see this cheatsheet

The Java Code convention has a more detailed description -

Why use code conventions (From the link) -

Code conventions are important to programmers for a number of reasons:

  • 80% of the lifetime cost of a piece of software goes to maintenance.
  • Hardly any software is maintained for its whole life by the original author.
  • Code conventions improve the readability of the software, allowing engineers to understand new code more quickly and thoroughly.

Secondly - Refactoring

Start by using arrays - this will really reduce the amount of code you wrote. For example:

// Declare this constant at the top of the class 
private static final int NUMBER_OF_SCORES = 5;

int[] score = new int[NUMBER_OF_SCORES];

System.out.println("Please enter your test scores");

// Read all Scores
for (int i = 0; i < NUMBER_OF_SCORES; i++) {
  score[i] = stdIn.nextInt();
}

With arrays, you can also easily calculate the total score and output each score -

// Total Score
int totalScore = 0;
for (int i = 0; i < NUMBER_OF_SCORES; i++) {
  totalScore += score[i];
}

// Output All the scores
for (int i = 0; i < NUMBER_OF_SCORES; i++) {
  System.out.print("score " + i + " = " + score[i]);
}

You can also introduce a Person class - example. (Take note of the getInitials function)

public class Person {

    private final String name;
    private final String surname;

    public Person(String name, String surname) {
        this.name = name;
        this.surname = surname;
    }

    public String getName() {
        return name;
    }

    public String getSurname() {
        return surname;
    }

    public String getInitials() {
        return "" + name.charAt(0) + surname.charAt(0));
    }
}

With the introduction of a Person class, the main function can transform into the for example. (Also splitting up the main function into smaller parts)

public static final int NUMBER_OF_SCORES = 5;

public static void main(String[] args) {
    Scanner stdIn = new Scanner(System.in);
    System.out.print("Please enter your first name: ");
    // Read First Name
    String firstName = stdIn.next();
    System.out.print("Please enter your last name: ");
    // Read Last Name
    String lastName = stdIn.next();
    System.out.println("Hello " + firstName + " " + lastName);
    if (firstName.equals(lastName)) {
        System.out.println("Your first and last name are the same.");
    } else {
        // Now you know you have a valid person.
        Person person = new Person(firstName, lastName);
        System.out.println("Your first and last name are different.");
        processName(person);
        readAndProcessScores();
    }
}

private static void processName(Person person) {
    String full = person.getName() + person.getSurname();
    System.out.println("The length of your first name is: " + person.getName().length());
    System.out.println("The length of your full name is: " + full.length());
    System.out.println("Your initials are " + person.getInitials());
}

public static void readAndProcessScores() {
    Scanner stdIn = new Scanner(System.in);

    int[] score = new int[NUMBER_OF_SCORES];
    System.out.println("Please enter your test scores");
    // Read all Scores
    for (int i = 0; i < NUMBER_OF_SCORES; i++) {
        System.out.print("Score " + i + " = ");
        score[i] = stdIn.nextInt();
        System.out.println();
    }

    // Output All the scores
    for (int i = 0; i < NUMBER_OF_SCORES; i++) {
        System.out.print("score " + i + " = " +score[i] + " ");
    }

    // Total Score
    int totalScore = 0;
    for (int i = 0; i < NUMBER_OF_SCORES; i++) {
        totalScore += score[i];
    }

    double scoreAverage = (((double) totalScore) / NUMBER_OF_SCORES);
    System.out.println("Total = " + totalScore);
    System.out.println("Your average is:  " + scoreAverage);
}

This code itself can be refactored even more, but this I leave to you :) Try doing the following -

  • Introduce one more class.
  • Extract the calculate total score into its own function. Use this declaration:

    public static double calculateAverage(int[] scores)
    
  • Add some comments to the methods :)

General tips

  • Always use good descriptive variable names.
  • Declare variables as needed (Close to where they are used).

Hope this helps!

\$\endgroup\$
  • 1
    \$\begingroup\$ Wow! That was extremely helpful. You made the code so much more cleaner and understandable. I really appreciate it. I hope to have your skills one day =] \$\endgroup\$ – Kifah Sep 6 '15 at 15:02
  • 1
    \$\begingroup\$ @Reg I've some suggestions built upon your answer... \$\endgroup\$ – h.j.k. Sep 6 '15 at 16:46
  • 1
    \$\begingroup\$ Are You sure your person class needs all those getters and setters? \$\endgroup\$ – Caridorc Sep 6 '15 at 19:48
  • \$\begingroup\$ No, you are right. Will update. \$\endgroup\$ – Reg Sep 6 '15 at 19:50
2
\$\begingroup\$

Building upon @Reg's answer...

  1. Avoid new String().

    Since strings are immutable in Java, there's almost never a need to do a new String() unless you're using one the many array-based constructors. This would have given you a String too:

    String initials = "" + name.charAt(0) + surname.charAt(0);
    

    Or if you also want to eliminate relying on the "" entirely:

    String initials = name.substring(0, 1) + surname.substring(0, 1);
    
  2. Validation.

    I'm not sure why you think a valid name is one with a different name from surname, but leaving that strange requirement aside, you can also consider introducing either a validation method, or let the the Person class determine that for you. The latter is slightly more preferred, as the class should know what is a valid requirement and encapsulate that.

    // validation method suggestion
    private static boolean isValidPersonNames(String firstname, String surname) {
        return !firstname.equals(surname);
    }
    
    // inside the Person class
    private static Person createFromInput(Scanner scanner) {
        String firstname = scanner.nextLine();
        String surname = scanner.nextLine();
        return !firstname.equals(surname) ? new Person(firstname, surname) : null;
    }
    

    For example, if you opted for the Person-class suggestion, you wouldn't have a large-ish if-else code-block, and even better, you can keep prompting the user until you get a usable Person object:

    public static void main() {
        ...
        Person person;
        // stdIn is a Scanner instance
        while ((person = Person.createFromInput(stdIn)) == null) {
            System.out.println("Not a valid person's name, please try again.");
        }
        // person will be non-null from here on
        processName(person); // or printName()?
        readAndProcessScores();
    }
    
  3. Read carefully.

    @Reg's answer suggested the following method:

    public static void readAndProcessScores() {
        Scanner stdIn = new Scanner(System.in);
        ...
    }
    

    This is slightly troublesome for three reasons:

    1. It is not evident what this method is reading from.
    2. Since we already have a Scanner instance reading from System.in from the main() method, we can reuse the same instance. That should then be used for the method's argument.
    3. If System.in is closed by accident from another place, such as doing a scanner.close(), where scanner is also wrapping System.in, reading console input in this method will fail.

    There is was also one bug towards the end of this method, when calculating the average...

    int NUMBER_OF_SCORES = 5;
    int totalScore = /* <calculated> */;
    double scoreAverage;
    scoreAverage = totalScore / NUMBER_OF_SCORES;
    System.out.println("Your average is:  " + scoreAverage);
    

    Creating the temporary variable scoreAverage (and doing the assignment on the next line...) is just one minor thing to improve on:

    System.out.println("Your average is:  " + (totalScore / NUMBER_OF_SCORES));
    

    The bug is that we are doing an integer-based division here, which loses precision first before the widening primitive conversion takes place. To remedy this, we just need to convert any of the operand to the intended double primitive type first, before doing the calculation:

    System.out.println("Your average is:  " + (((double) totalScore) / NUMBER_OF_SCORES));
    

    Of course, declaring totalScore as a double in the first place will be the simpler solution still.

  4. try-with-resources.

    Since you are already on Java 7, do consider wrapping your Scanner instance inside a try-with-resources in order to accustom yourself to safe closing of resources.

\$\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.