4
\$\begingroup\$

Here is my final build of the Age Distance Calculator - Any feedback on the code would be great - This is my 2nd day of java programming and thought this was a good little project.

package dev.swift.atc;

import java.util.Scanner;

import java.util.concurrent.TimeUnit;

public class AgeCalc {

   private int personAge;
   private int age;
   private int d_age;
   private String name;
   static Scanner s = new Scanner(System.in);

   public static void main(String [] args) throws InterruptedException
   {
       AgeCalc ac = new AgeCalc();
       ac.getInputs();
       boolean result = ac.calculateAge();
       if (result == true) {
          ac.showResults();
          s.close();
       }
       else {
        ac.errorResults();
        TimeUnit.SECONDS.sleep(3);
        System.out.println("Restarting Program");
        TimeUnit.SECONDS.sleep(2);
        ac.ageCalculateTwo();
       } 
   }
   //The constructor 
   public AgeCalc() throws InterruptedException {
       System.out.println("Welcome To Java Coded Age Differnce Calculator");
       TimeUnit.MILLISECONDS.sleep(100);
   }

   //Handle inputs 
   public void getInputs() throws InterruptedException {
        System.out.println("Input Your Name Before We Start");
        this.name = s.nextLine();
        TimeUnit.MILLISECONDS.sleep(100);
        System.out.println("Input Your Age");
        this.age = s.nextInt();
        TimeUnit.MILLISECONDS.sleep(100);
        System.out.println("Your Age Is " + this.age + " Years Old.");
   }

   //Make the calculation 
   public boolean calculateAge() throws InterruptedException {
        TimeUnit.MILLISECONDS.sleep(500);
        System.out.println("Now Input The Desired Age To Work Out The Distance To");
        this.d_age = s.nextInt();
        if (this.d_age < this.age){
            System.out.println("You Cannot Set The Target To Lower Then Your Current Age");
            return false;
        }
        else {
         this.personAge = this.d_age - this.age;
         return true;
        }
    }

   //Show the results
   public void showResults() throws InterruptedException {
       System.out.println("Working - Coded By Swift, Hope You Will Use This Program Again " + this.name);
       TimeUnit.SECONDS.sleep(3);
       System.out.println("You Will Be " + this.d_age + " In " + this.personAge +" Years");
       TimeUnit.SECONDS.sleep(3);
       System.out.println("Hope You Enjoyed The Program " + this.name);
       System.exit(0);
       System.out.println("Line Never Reached - Test");
   }
   public void errorResults() throws InterruptedException {
       System.out.println("You Have Entered An Error " + this.name);
   }
   public void ageCalculateTwo() throws InterruptedException {
        TimeUnit.MILLISECONDS.sleep(500);
        System.out.println("Now Input The Desired Age To Work Out The Distance To - Remember Not To Be Lower Than " + this.age);
        this.d_age = s.nextInt();
        if (this.d_age < this.age){
            System.out.println("You Have Already Been Told Not To Do This");
            TimeUnit.SECONDS.sleep(3);
            System.out.println("Program Shutting Down " + this.name + "Due To Multiple Errors You Have Caused ");
            TimeUnit.SECONDS.sleep(2);
            System.out.println("Closed - Coded By Swift, Hope You Will Use This Program Again " + this.name);
            TimeUnit.SECONDS.sleep(2);
            System.exit(0);
            System.out.println("Line Never Reached - Test");
        }
        else {
         this.personAge = this.d_age - this.age;
         System.out.println("Working - Coded By Swift, Hope You Will Use This Program Again " + this.name);
         TimeUnit.SECONDS.sleep(3);
         System.out.println("You Will Be " + this.d_age + " In " + this.personAge +" Years");
         TimeUnit.SECONDS.sleep(3);
         System.out.println("Hope You Enjoyed The Program " + this.name);
         System.exit(0);
         System.out.println("Line Never Reached - Test");
        }
    }
   }
\$\endgroup\$
3
\$\begingroup\$

I would use more legible names for variables and classes, they make it easier to read code as plain text (which is useful for other people to understand your code).

For example: naming your class AgeCalculator instead of AgeCalc, or ageCalculator instead of ac when instantiating it.

It is not that important on this example because it's not an extensive code, but if it where, it would be very difficult even for yourself (as the code owner) to remember what all variables mean, for further explanation I would suggest reading the disadvantages paragraph of https://en.wikipedia.org/wiki/Hungarian_notation

Another thing I could suggest you is to have your main method outside AgeCalculator, you should only keep inside methods that represent its behaviour.

I would start practising those, good luck!

EDIT

I found an example at http://www.tutorialspoint.com/java/java_object_classes.htm I modified it a bit to also apply what I understand are more legible names:

import java.io.*;

public class Employee{

   String name;
   int age;
   String designation;
   double salary;

   // This is the constructor of the class Employee
   public Employee(String name){
      this.name = name;
   }
   // Assign the age of the Employee  to the variable age.
   public void setAge(int newAge){
      age =  newAge;
   }
   /* Assign the designation to the variable designation.*/
   public void setDesignation(String newDesignation){
      designation = newDesignation;
   }
   /* Assign the salary to the variable salary.*/
   public void setSalary(double newSalary){
      salary = newSalary;
   }
   /* Print the Employee details */
   public void printEmployee(){
      System.out.println("Name:"+ name );
      System.out.println("Age:" + age );
      System.out.println("Designation:" + designation );
      System.out.println("Salary:" + salary);
   }
}

As you can see, variable names get a bit longer, but they also state what they are for, just by reading their names, so you don't have to remember "What did 'asdf' mean??" xD get me?

Also take into account that in complex code, you probably would have to be remembering many variables, and sometimes it won't be your code at all! So you won't be familiar with it.

Now, the main method, should be in another class (probably another file):

import java.io.*;

public class TheClassThatHasMainMethod{

   public static void main(String args[]){
      /* Create two objects using constructor */
      Employee firstEmployee = new Employee("James Smith");
      Employee secondEmployee = new Employee("Mary Anne");

      // Invoking methods for each object created
      firstEmployee.setAge(26);
      firstEmployee.setDesignation("Senior Software Engineer");
      firstEmployee.setSalary(1000);
      firstEmployee.printEmployee();

      secondEmployee.setAge(21);
      secondEmployee.setDesignation("Software Engineer");
      secondEmployee.setSalary(500);
      secondEmployee.printEmployee();
   }
}

Because as I said, you should only keep inside a class, methods that represent it's behaviour. At least in Object Oriented Programming, like this case, because each time you initialise a new Object (new Employee), you want it to behave like an Employee and expect it to have Employee methods, like calculating it's salary for example, or getting it's name. But a method called "main" doesn't make sense at all, an employee could give you his salary or name, not his... main?

Also remember that main is a special method that represents the beginning of the execution of your code, it should be separated from the rest of your objects and it must be unique.

Well hope I've helped some, I've also much to learn myself, good luck!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply and the advice - Like i said above i am very new to Java coding and wonder what you meant by another class outside AgeCalculator - how would I implement it as code and what would I use it for - like i said not to sure what you mean by "keep inside methods that represent it's behaviour." \$\endgroup\$ – SwiftYT Aug 10 '16 at 21:03
  • \$\begingroup\$ @SwiftYT see EDIT in my answer \$\endgroup\$ – Loaderon Aug 11 '16 at 1:11
  • \$\begingroup\$ Alot of this is hard for me to still understand but I really understand your talk about naming variables which is great - Secondly the idea of object orientated programming was the structure was shown to me by someone - I was just putting parts of my code into methods and calling them to main - I understand now with the idea of running the code in another class - How do you link the information to one another? \$\endgroup\$ – SwiftYT Aug 11 '16 at 10:56
  • \$\begingroup\$ Object Orientated Programming (OOP, or POO in Spanish) takes some time to get along with, but the idea is to think code as objects, instead of only mathematical functions, purpose is: We humans are much more familiarised with objects, so is easier to think of code as objects. You next step should be focusing on studying OOP in any Language that supports it, Java would be an excellent idea because you already started with it (and I starting my career with that language too!). That would also give you a good general idea on programming (on things like linking code from different files). \$\endgroup\$ – Loaderon Aug 11 '16 at 20:26
1
\$\begingroup\$

While I was skimming through your program I wrote few things to address. These tips may help you to improve yourself.

  • Pick a consistent naming convention in your programming life.
  • Give meaningful names for your variables
  • Use appropiate Exceptions only when needed
  • Method shouldn't know anything beyond its scope (You are directly accessing the scanner in many methods)
  • More accessible variables/ methods write at the top of the class. For e.g.: public methods appear above the private methods
  • After System.exit(0) nothing will be executed. You are just simply shutting down the JVM.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply - I understand the idea of now having more meaningful names - Now what do you mean by appropiate exceptions when needed - If you are talking about the InterruptedException that is always there for the time delay between dialog I believe - Eclipse generates it. You also talk about how the method is accessing the scanner - Should I have scanners per method and they write the data to for instance this.age and this.name - And also you mean just write public methods above private just because? and the system.exit(0) is there to end the program I have no idea how to loop it back \$\endgroup\$ – SwiftYT Aug 11 '16 at 10:59
  • \$\begingroup\$ I used the code to try loop back to main(args) or something along that line but when I did this it - when it got to the user inputs bit it just printed out the questions and did not wait for any input - Maybe its because it already has the inputs - But thats why i went for a method looping it back allowing them to try do it correctly and if false to end program - I see no way of restarting the program inside of it self \$\endgroup\$ – SwiftYT Aug 11 '16 at 11:01
1
\$\begingroup\$

(Some comments may seem harsh, especially for your second day of programming. This is not intended to discourage you.)

The thing that sticks out the most to me is you have three ambiguous variables referring to "age".

personAge, age, d_age have nothing to distinguish them in a meaningful way. E.g is "age" not the age of a person? Is there some magical meaning of "d_" that I should be aware of? My first guess while reading was that it would be age in days. I had to guess until i saw exactly how it was used because the name did not give enough information. "personAge" is simple named wrong as it is never set to the age of a person.

The second thing that stood out was all the useless calls to "sleep".

The third thing was the redundant use of "this.", it's a style thing so debatable, but I find it makes the code more cluttered and less readable.

Why are you capitalizing every word in the sentences?

Method names ageCalculate and ageCalculateTwo are misleading and ambiguous. Neither of these methods calculates an age.

System.exit should not be called in a "calculate" method, if at all.

It would probably make more sense in this case to return the value calculated rather than the boolean for success/failure. You could use an exception for failure, or simply accept the idea of a negative result as being valid for what the user wanted to do.

--- adding more feedback in response to comments---

I realize that this is just a simple exercise, but if you were making a "real" application, AgeCalc and your main class would be different. You would have a different class handling the main loop and the I/O. All of these methods are really too trivial to justify a new class in this case though. Here is what I would do:

Create a loop in the main method, so as you mentioned in the comment you can loop until the user choose to exit.

Do all of the I/O in the main method. E.g.:

public static void main(String [] args) {
    System.out.println("Input Your Name Before We Start");
    String name = s.nextLine();
    System.out.println("Input Your Age");
    int age = Math.abs(s.nextInt());
    System.out.println("Your Age Is " + age + " Years Old.");
    while (true) {
        System.out.println("Input The Desired Age To Work Out The Distance To (Use -1 to exit):");
        int desiredAge = s.nextInt();
        if (desiredAge < age) {
            System.out.println("You Cannot Set The Target To Lower Then Your Current Age ("+age+")");
            break; // no point in continuing
        }

        // now do the real work
        AgeCalc ac = new AgeCalc();
        ac.setCurrentAge(age);
        ac.setTargetAge(desiredAge);
        System.out.println("You Will Be " + ac.targetAge + " In " + ac.calculateDifference() +" Years");
    }
    System.out.println("Hope You Enjoyed The Program " + this.name);
}

Call instance methods on your AgeCalc object to set the inputs, do the calculation, and retrieve the result. Display the result in the main loop.

AgeCalc.getInputs() - getting inputs is not the job of the age calculator. It should be receiving inputs. Do the I/O in your main loop and pass the inputs to AgeCalc

The AgeCalc constructor should not be printing a message. Constructors should only construct the object and not have side-effects like that.

Name your variables like: currentAge, targetAge, ageDifference as these are the same terms you are using in the messages ("You Cannot Set The Target To Lower Then Your Current Age"). I prefer Difference to Distance, but either works.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I love criticism helps me improve and helps me to learn - So i understand that I have age and person age - d_age is distance age - so thats the target they want to work out to. I understand from most comments that I need to start defining variables clearly and also need to set variable names to proper meanings. I use the sleep calls to just put delay between dialog as Im only running it from console - I do not like text being printed so fast. I guess me doing things like Program Restarting and waiting a total of 6 seconds is a bit stupid. I think I have OCD i think personally capitals is neat. \$\endgroup\$ – SwiftYT Aug 11 '16 at 14:38
  • \$\begingroup\$ Yes see I am not sure how to do this but you see I have ageCalculate and ageCalculateTwo also misleading as they are working out the inputs from d_age and age to give the result. I developed these two because I had no idea how to loop back to the start of the program. I tried putting in the main void - the main(args) to start again but it ended up just printing out all my questions and not asking for user inputs. This is the main reason I added system.exits because I have no way asking the user if they want to start again. I used the boolean option as that was the only way Ik to change my main \$\endgroup\$ – SwiftYT Aug 11 '16 at 14:42
  • \$\begingroup\$ So far as I am very new to it - I was going to accept the idea of a negative result as-well as that was something once I first ran it - I noticed I did not like and really did not meet the purpose of the program "Age To Distant Age" not "Age To Past Age" so thank you a lot for all your feed back maybe you can show me examples or link me to tutorials on how I could loop this program instead - Putting my if and else into loops - I will learn java loops soon. Thanks once again @swpalmer \$\endgroup\$ – SwiftYT Aug 11 '16 at 14:44

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.