4
\$\begingroup\$

I am starting out with Java programming and get limited feedback from my professor. Following is an assignment that I handed in, and just re-factored. I wanted to see if people with Java experience could give some pointers on improving how it looks. Last semester I took a C++ course so splitting sections up into public/private methods/classes/objects is pretty foreign to me. Feedback on commenting is also welcome; I'm not sure about how much I should be commenting.

Note: All user input is taken is as a string as per the Professor's request. Hence the parseDoubles flying around.

    import java.util.Scanner;

public class ReportBuilder {

    static Scanner keyboard = new Scanner(System.in);

    public static int numericalInputs = 7;

    private static String firstName;
    private static String lastName;
    static String reportName;

    public static void main(String[] args) {

        firstName = "Empty";
        lastName = "Empty";
        reportName = "Empty";

        System.out.println("This program will generate a report based on the input");
        System.out.println("If you wish to exit enter \"-1\"");

        System.out.println("Please enter your first name:");
        firstName = FirstInput.main();

        System.out.println(firstName + ", please enter your last name one character at a time");
        System.out.println("When you are finished, type 'Stop'");
        lastName = SecondInput.main();

        System.out.println("Hi, " + firstName + " " + lastName + ".");
        System.out.println("Enter the name of the report:");
        reportName = keyboard.next();

        if (reportName.equals("-1")) {
            exit();
        }
        else {
        System.out.println("Please enter seven numbers one at a time");
        ThirdInputv2.main();
        }

        keyboard.close();   
    }

    public static void exit() {
        System.out.println("Good bye!");
        System.exit(0);
    }
}

public class FirstInput {

    public static String main() {

        String ownerName = "Empty";

        ownerName = ReportBuilder.keyboard.next();
        if (ownerName.equals("-1")) {
            ReportBuilder.exit();
        }
        else {
            ownerName = ownerName.replaceAll("[0-9]+ ?","");
        }

        return ownerName;
    }
}

public class SecondInput {

    private static String lastName;
    private static int stringLength;

    public static String main() {
        lastName = "Empty";

        return characterInput();
    }

    private static String characterInput() {
        /* This method takes in the last name one character at a time then stops when instructed.
         */

        String[] charArray = new String[20];
        String lastInput = "None";

        stringLength = -1;

        for (int i = 0; i < charArray.length; i++) {    
            /* This loop takes in individual strings and creates an array of strings of length 1.
             */
            lastInput = ReportBuilder.keyboard.next();
            charArray[i] = lastInput.substring(0,1);
            if (lastInput.equals("-1")) {
                ReportBuilder.exit();
            }

            /* The validateLast method of CheckNum is called to see if the user input contains a number.
             */
            else if (CheckNum.validateLast(charArray[i])) {
                System.out.println("Please do not enter any numbers");
                i--;
            }

            /* When the user enters "Stop" no more input is accepted
             */
            else if (lastInput.equalsIgnoreCase("Stop")) {
                stringLength = i;
                break;
            }
        }

        lastName = stringCharacters(charArray, stringLength);

        return lastName;
    }

    private static String stringCharacters(String[] characters, int length) {
        /* This method concatenates all characters 
         * then turns them into a single string, containing the last name.
         */
        String lastName = "";

        for (int i = 0; i < length; i++) {
            lastName = lastName + characters[i];
        }
        return lastName;
    }
}

public class ThirdInputv2 {

    private static int numberOfInputs = ReportBuilder.numericalInputs;
    private static String[] userInputString = new String[numberOfInputs];

//  The following arrays are created so that each one can be fed into the grandTotal method
    private static double[] arrayOfInputs = new double[numberOfInputs];
    private static double[] arrayOfMaxValues = new double[numberOfInputs];
    private static double[] arrayOfMinValues = new double[numberOfInputs];
    private static double[] arrayOfAccumulators = new double[numberOfInputs];
    private static double[] arrayOfAverages = new double[numberOfInputs];

    public static void main() {
        numArray();
        System.out.println("");
        printReport();
    }

    private static void numArray() {
        /* This method collects each number entered by the user then stores it in an array
         * After each number is entered: the max value, minimum value, average, and total are displayed
         */ 

        int elementCounter = 1;

        for(int i = 0; i < numberOfInputs; i++) {           
            userInputString[i] = ReportBuilder.keyboard.next();

            // Program exit condition of -1:
            if (userInputString[i].equals("-1")) {
                ReportBuilder.exit();
            }

            if (i == 0); {
                arrayOfAccumulators[i] = Double.parseDouble(userInputString[i]);
                arrayOfAverages[i] = arrayOfAccumulators[i];

            }

            arrayOfInputs[i] = Double.parseDouble(userInputString[i]);

            if (i > 0) {
                arrayOfAccumulators[i] = arrayOfAccumulators[i-1] + arrayOfInputs[i];
                arrayOfAverages[i] = arrayOfAccumulators[i-1]/elementCounter;
            }

            arrayOfMaxValues[i] = maxCalc(arrayOfInputs, elementCounter);
            arrayOfMinValues[i] = minCalc(arrayOfInputs, elementCounter);

            elementCounter++;

            printMetrics(arrayOfInputs[i], arrayOfMaxValues[i], arrayOfMinValues[i], 
                         arrayOfAccumulators[i], arrayOfAverages[i], elementCounter);
            }
        }

    private static double maxCalc(double[] arrayNums, int counter) {
        /* This method finds the greatest element in the incoming array.
         */

        double highest = -1;
        highest = arrayNums[0];

        for(int i = 0; i<counter; i++) {
            if(arrayNums[i] > highest) {
                highest = arrayNums[i];
            }       
        }
        return highest;
    }

    private static double minCalc(double[] arrayNums, int counter) {
        /* This method finds the lowest element in the incoming array.
         */

        double lowest = -1;
        lowest = arrayNums[0];

        for (int i = 0; i<counter; i++) {
            if(arrayNums[i] < lowest) {
                lowest = arrayNums[i];
            }
        }
        return lowest;
    }

    private static void printMetrics(double input, double max, double min, double accumulator, double average, int counter) {
        /* This method prints the metrics for the latest user input value.
         */
        System.out.println("The running total is " + accumulator);
        System.out.println("The max value so far is " + max
                            + " and the min value is " + min);
        System.out.println("The average value is " + average);

        if(counter < numberOfInputs) {
            System.out.println("Please enter the next number:");
        }
    }

    private static void printReport() {
        /* This method prints out the final report to screen.
         */

        String name = ReportBuilder.reportName;

        // Header
        System.out.println("Report ID: " + name);
        System.out.println("Input" + "\t" + "Max" + "\t" + "Min" + "\t" + "Total" + "\t" + "Average");
        System.out.println("----------------------------------------");

        // Body
        for (int row = 0; row < numberOfInputs; row++) {
            // This loop prints out the contents of each column, row by row

            System.out.println(arrayOfInputs[row] + "\t" + arrayOfMaxValues[row] + "\t" + arrayOfMinValues[row]
                               + "\t" + arrayOfAccumulators[row] + "\t" + arrayOfAverages[row]);
        }

        // Grand Total row containing the sum of each column
        System.out.println("----------------------------------------");
        System.out.println(grandTotal(arrayOfInputs) + "\t" + grandTotal(arrayOfMaxValues) + "\t"
                           + grandTotal(arrayOfMinValues) + "\t" + grandTotal(arrayOfAccumulators) + "\t"
                           + grandTotal(arrayOfAverages) + "  = Grand Totals");
    }

    private static double grandTotal(double[] columnArray) {
        /* This method calculates the sum of all values for a given column in the report.
         */

        double gTotal = columnArray[0];
        for (int i = 1; i < 7; i++) {
            gTotal = gTotal+ columnArray[i];
        }

        return gTotal;
    }
}

public class CheckNum {

    public static boolean validateLast(String testString) {
        /* The variable isNumeric returns true if the input string contains an integer between 0 and 9
         */
        boolean isNumeric;

        if(testString.matches("[0-9]+")) {
            isNumeric = true;
        }
        else {
            isNumeric = false;
        }

        return isNumeric;
    }
}
\$\endgroup\$
  • \$\begingroup\$ You can read this and this for advice about using comments. These are pretty good. \$\endgroup\$ – Aseem Bansal Jul 15 '13 at 6:16
3
\$\begingroup\$

Just a few comments:

  • in Java, comments describing methods are ususally placed right before the method declaration, not after it. Also, use /** ... */, so they are turned into Javadoc comments.
  • Its odd how you create separate classes for each of the input tasks, and how those classes define a main method. In Java, the main method describes the entry point to your program, like the main method in your first class. Calling other methods main may lead to confusion.
  • Rather than having several classes with lots of static variables, try to convert those classes to simple methods of your main class, and make those variables local to those methods.

Actually I did not plan on doing this, but whatever. Here's my version of the code.

import java.util.Scanner;

/**
 * [describe what the program is doing]
 */
public class ReportBuilder {

    static Scanner keyboard = null;

    /**
     * main program flow
     */
    public static void main(String[] args) {
        keyboard = new Scanner(System.in);

        System.out.println("This program will generate a report based on the input");
        System.out.println("If you wish to exit enter \"-1\"");

        System.out.println("Please enter your first name:");
        String firstName = getFirstName();

        System.out.println(firstName + ", please enter your last name one character at a time");
        System.out.println("When you are finished, type 'Stop'");
        String lastName = getLastName();

        System.out.println("Hi, " + firstName + " " + lastName + ".");
        System.out.println("Enter the name of the report:");
        String reportName = readString();

        System.out.println("Please enter seven numbers one at a time");
        double[] numbers = getNumberArray(7);
        System.out.println("");
        printReport(reportName, numbers);

        keyboard.close();   
    }

    /**
     * handle exit condition each time a keyboard input is read
     */
    private static String readString() {
        String input = keyboard.next();
        if ("-1".equals(input)) {
            System.out.println("Good bye!");
            System.exit(0);
        }
        return input;
    }

    private static String getFirstName() {
        String firstName = readString();
        return firstName.replaceAll("[0-9]+ ?","");
    }

    /** This method takes in the last name one character at a time then stops when instructed.
     */
    private static String getLastName() {
        String lastName = "";
        while (true) {
            String input = readString();
            // When the user enters "Stop" no more input is accepted
            if (input.equalsIgnoreCase("Stop")) {
                break;
            }
            // The validateLast method of CheckNum is called to see if the user input contains a number.
            if (isNumber(input)) {
                System.out.println("Please do not enter any numbers");
            }
            // add character to lastName
            lastName += input.substring(0, 1);
        }
        return lastName;
    }

    /** This method collects each number entered by the user then stores it in an array
     * After each number is entered: the max value, minimum value, average, and total are displayed
     */ 
    private static double[] getNumberArray(int numberOfInputs) {
        double[] inputs = new double[numberOfInputs];
        double min = 9999, max = 0, sum = 0, avg = 0;

        for (int i = 0; i < numberOfInputs; i++) {           
            String userInput = readString();

            if (isNumber(userInput)) {
                inputs[i] = Double.parseDouble(userInput);
                min = Math.min(min, inputs[i]);
                max = Math.max(max, inputs[i]);
                sum = sum + inputs[i];
                avg = sum / (i+1);

                System.out.println("The running total is " + sum);
                System.out.println("The max value so far is " + max
                        + " and the min value is " + min);
                System.out.println("The average value is " + avg);

                if (i < numberOfInputs) {
                    System.out.println("Please enter the next number:");
                }
            } else {
                System.out.println("Please enter a number!");
                i--;
            }
        }
        return inputs;
    }

    /** This method prints out the final report to screen.
     */
    private static void printReport(String name, double[] numbers) {
        double min = 9999, max = 0, sum = 0, avg = 0;
        double[] mins = new double[numbers.length];
        double[] maxs = new double[numbers.length];
        double[] sums = new double[numbers.length];
        double[] avgs = new double[numbers.length];

        // Header
        System.out.println("Report ID: " + name);
        System.out.println("Input" + "\t" + "Max" + "\t" + "Min" + "\t" + "Total" + "\t" + "Average");
        System.out.println("----------------------------------------");

        // Body
        // This loop prints out the contents of each column, row by row
        for (int i = 0; i < numbers.length; i++) {
            // okay, here's a bit of code duplication, but not that critical
            mins[i] = min = Math.min(min, numbers[i]);
            mins[i] = max = Math.max(max, numbers[i]);
            sums[i] = sum = sum + numbers[i];
            avgs[i] = avg = sum / (i+1);

            System.out.println(numbers[i] + "\t" + max + "\t" + min + "\t" + sum + "\t" + avg);
        }

        // Grand Total row containing the sum of each column
        System.out.println("----------------------------------------");
        System.out.println(sum(numbers) + "\t" + sum(maxs) + "\t" + sum(mins) 
                + "\t" + sum(sums) + "\t" + sum(avgs) + "  = Grand Totals");
    }

    /** This method calculates the sum of all values for a given column in the report.
     */
    private static double sum(double[] nums) {
        double sum = 0;
        for (int i = 1; i < nums.length; i++) {
            sum += nums[i];
        }
        return sum;
    }

    /** @return true if the input string contains an integer
     */
    private static boolean isNumber(String testString) {
        return testString.matches("[0-9]+");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments. About how my classes were split up, what is a more intuitive way that I could have split up the program? \$\endgroup\$ – freddygv Jul 15 '13 at 16:35
  • \$\begingroup\$ @freddygv See my edit. \$\endgroup\$ – tobias_k Jul 15 '13 at 20:30
  • \$\begingroup\$ Looks great, I really appreciate it. I'm surprised to only see one class, definitely need to look further into when to split them up. By the way, how come you split up the keyboard scanner by initializing it to null? I hadn't seen that before. \$\endgroup\$ – freddygv Jul 16 '13 at 14:32
  • \$\begingroup\$ @freddygv About the Scanner: No special reason, I just wanted the main method to be "symmetrical". Start with creating the scanner, end with closing it. \$\endgroup\$ – tobias_k Jul 16 '13 at 14:57

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.