Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

First I tried to fetch this program as close to pseudo code where I must follow all imported items, all method definitions, and all defined variables; anything else was my choice.

After I created working program that best represent the sample run of output we were provided in my homework documentation I decided to create checks for data input such as correct number input.

Now, is my code (sorry for expression) idiot proof? Meaning that whatever user inputs will not effect program's outputs?

What to do to ensure that this code will not fail because of user input?

In my program I am trying to use while loops as much as possible to let user to re-enter data if data that was entered is not appropriate. But is this enough?

Thank you in advance!

import java.io.*;

public class CalculatePayroll
{
    public static final double TAX_SOCIAL = 0.10;
    public static final double TAX_STATE = 0.10;
    public static final double TAX_FEDERAL = 0.10;

    public static void main(String[] args)
    {
        String inputName = null, mustBeNumeric = "[-+]?\\d+(\\.\\d+)?";
        char inputStatus = 0, inputAskRepeat = 0;
        double inputSalary = 0, inputRate = 0, inputHours = 0;
        boolean boolRepeat = true;

        BufferedReader input = new BufferedReader(new InputStreamReader(System.in));

        System.out.println("Payroll Calculator");

        while (boolRepeat == true)
        {
            boolean boolAskRepeat = true;
            boolean boolAskStatus = true;

            System.out.print("\nLast Name: ");

            try {inputName = input.readLine(); } 
            catch (IOException e) { e.printStackTrace(); }

            while (boolAskStatus == true)
            {
                System.out.print("Status (F or P): ");

                try { inputStatus = input.readLine().toLowerCase().charAt(0); } 
                catch (IOException e) { e.printStackTrace(); }

                if (inputStatus == "f".charAt(0))
                {
                    boolAskStatus = false;

                    String stringCheckSalary = null;
                    boolean boolCheckSalary = true;
                    while (boolCheckSalary == true)
                    {
                        System.out.print("Salary: ");
                        try { stringCheckSalary = input.readLine(); } 
                        catch (IOException e) { e.printStackTrace(); }
                        if (stringCheckSalary.matches(mustBeNumeric))
                        {
                            boolCheckSalary = false;
                            inputSalary = Double.parseDouble(stringCheckSalary);
                        }
                        else 
                            boolCheckSalary = true;
                    }

                    outputData(inputName, inputStatus, calculateFullTimePay(inputSalary));
                }
                else if (inputStatus == "p".charAt(0))
                {
                    boolAskStatus = false;

                    String stringCheckRate = null;
                    boolean boolCheckRate = true;
                    while (boolCheckRate == true)
                    {
                        System.out.print("Rate: ");
                        try { stringCheckRate = input.readLine(); } 
                        catch (IOException e) { e.printStackTrace(); }
                        if (stringCheckRate.matches(mustBeNumeric))
                        {
                            boolCheckRate = false;
                            inputRate = Double.parseDouble(stringCheckRate);
                        }
                        else 
                            boolCheckRate = true;
                    }

                    String stringCheckHours = null;
                    boolean boolCheckHours = true;
                    while (boolCheckHours == true)
                    {
                        System.out.print("Hours: ");
                        try { stringCheckHours = input.readLine(); } 
                        catch (IOException e) { e.printStackTrace(); }
                        if (stringCheckHours.matches(mustBeNumeric))
                        {
                            boolCheckHours = false;
                            inputHours = Double.parseDouble(stringCheckRate);
                        }
                        else 
                            boolCheckHours = true;
                    }                   

                    outputData(inputName, inputStatus, calculatePartTimePay(inputRate, inputHours));
                }
                else boolAskStatus = true;
            }

            while (boolAskRepeat == true)
            {
                System.out.print("Another payroll? (Y or N): ");

                try { inputAskRepeat = input.readLine().toLowerCase().charAt(0); } 
                catch (IOException e) { e.printStackTrace(); }

                if (inputAskRepeat == "n".charAt(0))
                {
                    boolAskRepeat = false;
                    boolRepeat = false;
                }
                else if (inputAskRepeat == "y".charAt(0))
                {
                    boolAskRepeat = false;
                    boolRepeat = true;
                }
                else boolAskRepeat = true;
            }
        }

        System.out.print("\nProgram terminated...");
    }

    private static double calculateTaxes(double weeklyPay) 
    {
        return weeklyPay * (TAX_SOCIAL + TAX_STATE + TAX_FEDERAL);
    }

    private static double calculatePartTimePay(double rate, double hours) 
    {
        double pay = 0;

        if (hours <= 40) pay = rate * hours;
        else if (hours > 40) pay = (rate * 40) + ((hours - 40) * (rate * 1.5));

        return pay;
    }

    private static double calculateFullTimePay(double salary) 
    {
        return salary / 52;
    }

    private static void outputData(String name, char status, double pay) 
    {
        if (status == "p".charAt(0))
        {
            System.out.println("-------------------------------------------------------");
            System.out.println("Name\tStatus\t\tGross\tTaxes\tNet");
            System.out.printf("%s\tPart Time\t%.2f\t%.2f\t%.2f\n\n", 
                    name, 
                    roundDouble(pay), 
                    roundDouble(calculateTaxes(pay)), 
                    roundDouble(pay - calculateTaxes(pay)));
        }
        else if (status == "f".charAt(0))
        {
            System.out.println("-------------------------------------------------------");
            System.out.println("Name\tStatus\t\tGross\tTaxes\tNet");
            System.out.printf("%s\tFull Time\t%.2f\t%.2f\t%.2f\n\n", 
                    name, 
                    roundDouble(pay), 
                    roundDouble(calculateTaxes(pay)), 
                    roundDouble(pay - calculateTaxes(pay)));
        }
    }

    private static double roundDouble(double number) 
    {
        return Double.parseDouble(String.valueOf((int)(number * 100))) / 100;
    }
}
share|improve this question
7  
I'll put this as a comment as it's not really answering your question of idiot-proofness, but money shouldn't be represented in double or float, because they can't accurately represent most base 10 numbers. You should use either BigDecimal, int or long, with int and long representing pennies, cents or their equivalents. More info here -> javapractices.com/topic/TopicAction.do?Id=13 –  toofarsideways Sep 21 '12 at 10:33
4  
If you build an idiot proof program, they will build a better idiot. –  smartcaveman Sep 21 '12 at 12:58
    
"f" is a number in exadecimal. Silly doesn'it? –  cl-r Sep 21 '12 at 13:38
    
The input statement which asks for character input is not idiot-proof, as I noticed. It cannot handle empty or too long string, but I have fixed that. –  HelpNeeder Sep 22 '12 at 1:21
add comment

4 Answers

up vote 10 down vote accepted

"User Proofing"

  1. Don't use double

    As already mentioned by toofarsideways in the comments, storing currency values as float or double is problematic. Floating point cannot represent all numbers; it will get you close though.
    It is highly recommended that you use another option better suited for accuracy (like BigDecimal)

  2. 0-length input

    • Optional last name

      A user can skip entering a last name by pressing enter.

    • StringIndexOutOfBoundsException

      A user that skips entering any other value will result in a thrown StringIndexOutOfBoundsException. I find myself wondering if it's necessary to only check the first character

      Why not just compare Strings with equalsIgnoreCase(String)?

      if("f".equalsIgnoreCase(inputStatus)) {
          // ... full-time
      }
      
    • NullPointerException

      readLine() can return null if the user presses CTRL+C. Typically CTRL+C will kill your program anyways, but still nice to avoid NullPointerExceptions.

  3. Unchecked numeric values

    A user can submit whatever numeric value they want for salary, rate, or hours without any special checks/sanitzation.

    • Negative values

      A user can submit a negative salary, rate, or hours.

      Instead of using regex to validate the input, I would rely on the built-in Double.parseDouble(String) method to deal with the validation. You only have to worry about handling the NumberFormatException.

      System.out.print("Salary: ");
      try { 
          stringCheckSalary = input.readLine(); 
          inputSalary = Double.parseDouble(stringCheckSalary);
          boolCheckSalary = false;
      } catch (IOException e) {
          e.printStackTrace();
      } catch(NumberFormatException ex) {
          System.out.printf("Please try again. Invalid number: '%s'\n", stringCheckSalary);
      }
      

      The same idea works for BigDecimal as well.

    • Unbounded numbers

      You should validate the submitted values and make sure they make sense. Currently a user can input a salary of 700 trillion dollars, or maybe 400 hours of part time work in a week.

      As an example to validate the number of hours, you might use

      final int HOURS_IN_A_WEEK = 7 * 24; // should be static
      boolCheckHours = (inputHours < 0 || inputHours > HOURS_IN_A_WEEK);
      
  4. Unclear error handling (usability)

    When a user submits an unexpected value, you print the same request again without any indication of what went wrong.
    You should print an error message detailing the problem before reasking for input.

    For example,

    Please try again. Invalid number: 'one hundred'

  5. inputHours Bug

    You accidentally parse checkRate as inputHours. So the rate and hours for part-time will always be the same.

    inputHours = Double.parseDouble(stringCheckRate);
    


Other Code Comments

Since I'm at it I'll throw in a few comments on the code as well

  1. roundDouble(double)

    If you go with BigDecimal this will likely be a moot point.

    roundDouble(double) is unecessarily complicated. In general you need to be careful when converting between double and int values. The loss of precision can cause some pretty serious problems in a real application.

    If you're running Java 6+ then you can use DecimalFormat.setRoundingMode(RoundingMode),

    DecimalFormat formatter = new DecimalFormat("0.00");
    formatter.setRoundingMode(RoundingMode.DOWN);
    System.out.println("-------------------------------------------------------");
    System.out.println("Name\tStatus\t\tGross\tTaxes\tNet");
    System.out.printf("%s\tPart Time\t%s\t%s\t%s\n\n",
            name,
            formatter.format(pay),
            formatter.format(calculateTaxes(pay)),
            formatter.format(pay - calculateTaxes(pay))); 
    

    If you're not running at least Java 6, you can simplify this method very easily as

    private static double roundDouble(double number) {
        return Math.floor(number * 100.0) / 100.0;
    }
    

    Or if you're a fan of reusable methods,

    private static double roundDown(double number, int precision) {
        // floorWithPrecision(2.617, 2) => Math.floor(2.617 * 100.0) / 100.0 => 2.61
        final double shiftValue = Math.pow(10, precision);
        return Math.floor(number * shiftValue) / shiftValue;
    }
    
  2. String.charAt(0)

    Instead of using "f".charAt(0) to retrieve a single character you should use the char primitive 'f'

    if (inputStatus == 'f')
    
share|improve this answer
    
You have posten many things that I agree on. There was few things I coded there and I didn't agree too but part of this program was my homework. I just went further and tried to make this program as fail-proof as I could. I like bit of criticism on my coding :) Thank you! –  HelpNeeder Sep 23 '12 at 21:09
2  
readLine() returns null if the user presses CTRL+D (too). (It works on Linux.) –  palacsint Sep 24 '12 at 16:51
2  
@Palacsint I had a feeling about CTRL+D as well. My Linux box is down so I couldn't try it there. On windows it didn't seem to do anything especially noticeable. –  Jared Paolin Sep 24 '12 at 16:55
add comment

Not related to the idiot-proof aspect of your code is the fact that it is structured badly, more precisely, it is not structured at all. At the very least your main function should be broken down into several smaller functions. Given that this is java, you should probably create one or more nested classes that handle input and/or validation.

Related to idiot proof, you should create a generic method of asking your question and verifying that the answer is acceptable, you do that and it will be idiot proof.

share|improve this answer
    
I agree, the program, in fact, was not planned to be OOP so I just fetched it together and tried to make it fail safe :) –  HelpNeeder Sep 25 '12 at 20:27
add comment

mustBeNumeric is a regular expression that will not change. It is better declared as a static final field, rather than a mutable variable within a method scope.

share|improve this answer
2  
Added for future reference: How to check a String is a numeric type in java –  tanyehzheng Sep 24 '12 at 2:11
add comment

A few random notes:

  1. I'd put the variable declarations to separate lines. From Code Complete, 2nd Edition, p759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

  2. You could remove some duplication from the outputData method:

    private static void outputData(final String name, final char status, final double pay) {
        final String statusString;
        if (status == 'p') {
            statusString = "Part Time";
        } else if (status == 'f') {
            statusString = "Full Time";
        } else {
            throw new IllegalStateException("Invalid status: " + status);
        }
        System.out.println("-------------------------------------------------------");
        System.out.println("Name\tStatus\t\tGross\tTaxes\tNet");
        System.out.printf("%s\t%s\t%.2f\t%.2f\t%.2f\n\n", statusString, name, roundDouble(pay),
                roundDouble(calculateTaxes(pay)), roundDouble(pay - calculateTaxes(pay)));
    }
    

    I guess you could replace the similar if-else structures with polymorphism. Two useful reading:

  3. 52, 40 etc. should be named constant instead of magic numbers.

  4. while (boolAskStatus == true)
    

    could be simply:

    while (boolAskStatus)
    

    From Code Complete, 2nd Edition, p433:

    Compare boolean values to true and false implicitly

    [...]

    Using implicit comparisons reduces the number of terms that someone reading your code has to keep in mind, and the resulting expressions read more like conversational English.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.