1
\$\begingroup\$

I have created a simple program in which the user is prompted for various details about their employment. In my Employee.java file, I have all the methods.

Employee.java:

import javax.swing.JOptionPane;

public class Employee {

    private static String name;
    private static String jobTitle;
    private static int age;
    private static int salary;
    private static int taxAmount;
    private static int salaryPostTax;

    public static int getSalary() {
        try {
            String inputSalary = JOptionPane.showInputDialog("Enter your salary:");
            salary = Integer.parseInt(inputSalary);

        } catch (Exception ex) {
            JOptionPane.showMessageDialog(null, "ERROR! That's NOT a numeric value.");
        }
        return salary;
    }

    public static String getName() {
        name = JOptionPane.showInputDialog("Enter your name:");
        return name;
    }

    public static String getJobTitle() {
        jobTitle = JOptionPane.showInputDialog("Enter your Job Title:");
        return jobTitle;
    }

    public static int getAge() {
        try {
            String inputAge = JOptionPane.showInputDialog("Enter your age:");
            age = Integer.parseInt(inputAge);

        } catch (Exception ex) {
            JOptionPane.showMessageDialog(null, "ERROR! That's NOT a numeric value.");
        }
        return age;
    }

    public static int calcTaxAmount () {

        if (salary < 10_000)
            taxAmount = 0;

        if ((salary >= 10_000) && (salary < 30_000))
            taxAmount = 28;

        if ((salary >= 30_000) && (salary < 80_000))
            taxAmount = 38;

        if (salary >= 80_000)
            taxAmount = 48;

        return taxAmount;
    }

    public static int calcSalary () {
        salaryPostTax = salary - taxAmount;

        return salaryPostTax;
    }

    public static void showPayment() {
        if (inputValid() == true) {

            JOptionPane.showMessageDialog(null, "Pay to the order of " + name + " (" + jobTitle +
                    ") the sum of £"+ salaryPostTax);
        } else {
            JOptionPane.showMessageDialog(null, "ERROR! Please fill in ALL the required fields correclty.");
        }
    }

    public static boolean inputValid() {
        if (
                (name != null) &&
                (jobTitle != null) &&
                (!(salary <= 0)) &&
                (!(salaryPostTax <= 0)) &&
                (!(age <= 0))) {
            return true;

        } else {
            return false;
        }
    }
}

I have a file (UseIt.java) in which I use the class. My question though, is: Is it a good programming practice to either:

1. Put all your methods into one method, and simply call that one method rather than calling them all individually, which becomes tiresome?

Employee.java: (Example 1)

//Collate ALL Employee details into ONE method
public static void getAllDetails() {
    Employee.getName();
    Employee.getAge();
    Employee.getJobTitle();
    Employee.getSalary();
    Employee.calcTaxAmount();
    Employee.calcSalary();
    Employee.showPayment();
}

UseIt.java: (Example 1)

public class UseIt {

    static Employee foo = new Employee();

    public static void main(String[] args) {
        foo.getAllDetails(); //Calling ONE method, rather than all of them.
    }
}

2. Put all the methods into the constructor, so that the methods run automatically upon the creation of the instance?

Employee.java (Example 2)

//Collate ALL Employee details into the constructor
public Employee() {
    Employee.getName();
    Employee.getAge();
    Employee.getJobTitle();
    Employee.getSalary();
    Employee.calcTaxAmount();
    Employee.calcSalary();
    Employee.showPayment();
}

UseIt.java (Example 2)

public class UseIt {

    public static void main(String[] args) {
        Employee foo = new Employee();
    }
}
\$\endgroup\$
8
  • 4
    \$\begingroup\$ Yikes! NONE of Employee's fields or methods should be static. Seriously, as doing this prevents you from making anything more than one single Employee. \$\endgroup\$ Jan 17, 2015 at 17:35
  • \$\begingroup\$ If you're going object orientated you should have a class that gets the input, then another class validates this input then a class that creates your Employee from the input (imo). Following the single responsibility principle \$\endgroup\$
    – Blundell
    Jan 17, 2015 at 17:35
  • \$\begingroup\$ public static void getAllDetails(); – what is the point of this method? It will return you nothing. \$\endgroup\$
    – 5gon12eder
    Jan 17, 2015 at 17:35
  • \$\begingroup\$ First, data storage should certainly be separate from user input. Second, static is a dangerous and dirty habit. I would go as far as to say beginners shouldn't ever use it. \$\endgroup\$ Jan 17, 2015 at 17:36
  • 1
    \$\begingroup\$ We did. First and foremost make all your Employee fields and methods non-static. Please start there. Making a field static means that it is a field of the class not of the class's instance, and so all instances will share the exact same fields. So if you make 3 Employees, they'll all have the exact same names, salaries, etc... Please read up on the meaning of the static modifier, and then try to avoid using except in certain places where it is needed. If you made things static to fix an error, you're fixing things backwards. Change the code so that static is not needed. \$\endgroup\$ Jan 17, 2015 at 17:41

1 Answer 1

5
\$\begingroup\$

Wrote this on SO so excuse any compilation errors.

As a novice there is a lot to learn, I would start by understanding the SOLID Principles and also looking out for Top 10 errors Java programmers make.

The first thing you want to do is get away from the static main(String[] args) method that starts your program. You do not want to use static methods. Therefore you need instantiate your system (MySystem) so that you are in the world of objects.

You want to keep your responsibilities separate.

Always prefer immutability.

Here I have an immutable Employee object (it cannot be changed once instantiated). From the main method our DetailsRetriever will get the input from the user, this uses the DetailsValidator to ensure the input is correct. If all the details are correct a Factory creates our employee object.

public class UseIt {

    public static void main(String[] args) {
        MySystem foo = new MySystem(new DetailsRetriever(new DetailsValidator()), new EmployeeFactory())
        foo.start();
    }
}

public class MySystem {

   private final DetailsRetreiver detailsRetriever;
   private final EmployeeFactory employeeFactory;

   public MySystem(DetailsRetreiver detailsRetriever, EmployeeFactory employeeFactory) {
         this.detailsRetriever = detailsRetriever;
         this.employeeFactory = employeeFactory;
   }

   public void start() {
         String name = detailsRetriever.retrieveName();
         String jobTitle = detailsRetriever.retrieveJobTitle();
         ..etc

         Employee employee = employeeFactory.create(name, jobTitle, ..etc);
   }

}

public class Employee {

    private final String name;
    private final String jobTitle;
    private final int age;
    private final int salary;

    public Employee(String name, String jobTitle, int age, int salary) {
        this.name = name;
        this.jobTitle = jobTitle;
        this.age = age;
        this.salary = salary;
    }

   public int calcTaxAmount() {
    int calculatedTax;
    if (salary < 10_000)
        calculatedTax = 0;

    if ((salary >= 10_000) && (salary < 30_000))
        calculatedTax = 28;

    if ((salary >= 30_000) && (salary < 80_000))
        calculatedTax = 38;

    if (salary >= 80_000)
        calculatedTax = 48;

    return taxAmount;
}

   public int calcSalary() {
    int calculatedSalaryPostTax = salary - calcTaxAmount();

    return calculatedSalaryPostTax;
   }

}

public class DetailsRetreiver {

    private final DetailsValidator detailsValidator;

    public DetailsRetreiver(DetailsValidator detailsValidator) {
      this.detailsValidator - detailsValidator;
    }

    public String getName() {
        String name = JOptionPane.showInputDialog("Enter your name:");
        if(detailsValidator.validateName(name)) {
          return name;
        } else {
           return getName(); // or throw an exception
        }
    }

    // etc ...

}

public class DetailsValidator {

     public boolean validateName(String input) {
          // add more validation as you need
          return !"".equals(input);
     }

     // etc
}

public class EmployeeFactory {

      public Employee create(String name, String jobTitle, int age, ... etc) {
           return new Employee(name, jobTitle, age .. etc); 
      }

}
\$\endgroup\$
5
  • \$\begingroup\$ This is great, thanks much! So rather than a single class with many static methods, many classes with non-static methods... is that kinda the idea? \$\endgroup\$
    – Mr Chasi
    Jan 17, 2015 at 17:55
  • 2
    \$\begingroup\$ Yes but no, it's a lot more complicated than that. You need to read a lot of programming books to understand all that is in the above :-) but like everyone is saying if you want the short answer: avoid statics, use objects and invest in understanding S.O.L.I.D \$\endgroup\$
    – Blundell
    Jan 17, 2015 at 17:58
  • \$\begingroup\$ I would add that if using Swing, which this is, the whole lot needs to go in an invokeLater. \$\endgroup\$ Jan 17, 2015 at 18:04
  • 2
    \$\begingroup\$ @MrChasi One of the key concepts here is that you want to create instances of the Employee class, one instance per actual, real-world employee that you're modeling. This is the essence of object-oriented programming. Non-static methods and fields belong to those instances. Static methods and variables "belong" to the class as a whole, not an individual instance. And as Blundell said, all of this will become second-nature as you read more programming books. \$\endgroup\$
    – Ben
    Jan 17, 2015 at 18:04
  • \$\begingroup\$ @Ben Oh! I see, that's why every object I made in my example would have had the same values. \$\endgroup\$
    – Mr Chasi
    Jan 17, 2015 at 18:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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