2
\$\begingroup\$

the code is very messy, long and I am looking how to shorten it and/or simplify it. There are a lot of repeated variables, for example; 'hourToDay' and then 'hourToDay1', 'hourToDay2' and so forth. Can you help me find a better way to make this program more efficient?

P.S - I am a new coder and wanting to improve my ability, any suggestions would be grand :)

long lightYear = 6000000000000L;
int hoursInDay = 24;
int daysInYear = 365;
int yearsInCentury = 100;
int yearsInMillenium = 1000;


Scanner lightYearInput = new Scanner(System.in);
System.out.println("Enter light year's you'd like to convert" );

long light = lightYearInput.nextLong();
long totalLightYear = (long) (lightYear * light);



// Different vehicle variables with speed.

double car  = 277.87; //mph koenigseggAgeraRS
double MiG25 = 1919.42; //mph
double parkerSpaceProbe = 213240; //mph


// car conversion
long hourConversion = (long)(totalLightYear / car );
System.out.println("Driving the fastest car in space, would take you " + hourConversion + " hours to travel " + light + " light years."); //hours

long hourToDay = hourConversion / hoursInDay;
System.out.println(hourConversion + " hours is equivalent to " + hourToDay + " days."); //hours to days

long dayToYear = hourToDay / daysInYear;
System.out.println(hourToDay +" is equivalent to " + dayToYear + " years."); //days to years

long yearToCentury = dayToYear / yearsInCentury;
System.out.println(dayToYear +" is equivalent to " + yearToCentury + " Centuries."); //years to centuries

long yearToMillenium = yearToCentury / yearsInMillenium;
System.out.println(yearToCentury +" is equivalent to " + yearToMillenium + " millenium.\n"); // centuries to millenium

//MiG25 Conversion

System.out.println("PLANE");
long hourPlaneConversion = (long)(totalLightYear / MiG25 );
System.out.println("Driving the fastest plane in space, would take you " + hourPlaneConversion + " hours to travel " + light + " light years."); //hours

long hourToDay1 = hourPlaneConversion / hoursInDay;
System.out.println(hourPlaneConversion + " hours is equivalent to " + hourToDay1 + " days."); //hours to days

long dayToYear1 = hourToDay1 / daysInYear;
System.out.println(hourToDay1 +" is equivalent to " + dayToYear1 + " years."); //days to years

long yearToCentury1 = dayToYear1 / yearsInCentury;
System.out.println(dayToYear1 +" is equivalent to " + yearToCentury1 + " Centuries."); //years to centuries

long yearToMillenium1 = yearToCentury1 / yearsInMillenium;
System.out.println(yearToCentury1 +" is equivalent to " + yearToMillenium1 + " millenium.\n"); // centuries to millenium

The output works correctly (unless my maths / code is incorrect), so the program runs fine. I'm just simply looking to make my code more efficient. I'm sure you can see that I need to improve my ability, hopefully I can get some supportive guidance here.

New contributor
Hezekiah Sawyer is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
1
\$\begingroup\$

Comments should state why you've written code, not what it is doing. Let's take examples from your code.

// Different vehicle variables with speed.

double car  = 277.87; //mph koenigseggAgeraRS
double MiG25 = 1919.42; //mph
double parkerSpaceProbe = 213240; //mph

Variable names shouldn't start with an uppercase - I'm referring to MiG25 specifically, because they're reserved for other things like interfaces, classes, so on.

While reading your code I have absolutely no idea what koenigseggAgeraRS means. It would have been better to write:

double car  = 277.87; // Top mph of the car model koenigseggAgeraRS
double MiG25 = 1919.42; // Top mph of the plane MiG25
double parkerSpaceProbe = 213240; //mph

This explains why we wrote this variable and speed. But we can still improve on it by explaining what the variable is.

double carMph  = 277.87; // Top mph of the car model koenigseggAgeraRS
double planeMph = 1919.42; // Top mph of the plane MiG25
double spaceProbeMph = 213240; //mph of the Parker spaceProbe

With this, I know why you've chosen these speeds. In this case these comments are a bit excessive and break the flow of reading the code, but that's something that comes with practice.

long hourToDay1 = hourPlaneConversion / hoursInDay;
System.out.println(hourPlaneConversion + " hours is equivalent to " + hourToDay1 + " days."); //hours to days

Here the comment is irrelevant. The variable names and the code itself clearly show what's happening, I don't need the comment telling me again what its doing, I'd rather have it tell me why, or if its obvious, like here, skip it entirely.


I'm aware you asked for tips on efficiency, but practicing commenting and naming conventions is going to serve you well in the long run. Let's move to some basics in functions.

As you noted, you're repeating code and renaming variables the same. You can make the code much more efficient by using functions. Lets make the car hourConversion into a function, using the renamed carMph.

public void hourConversion(double vehicleMph){
    long LIGHT_YEAR = 6000000000000L;
    long light = lightYearInput.nextLong();
    long speedConverted = lightYear/vehicleMph;
    System.out.println("Driving the fastest car in space, would take you " + 
    speedConverted + " hours to travel " + light + " light years.");
}

You'll notice we moved the two lightYear and light variables to the function, I did this because we never use these variables again. In your method, I would've had to scroll back up and look at what these variables are and what their values are.

We can now call this method like so:

hourConversion(carMph);
hourConversion(planeMph);

From here, I think you can create more functions that make the rest of your code more efficient and clearer.

\$\endgroup\$
  • 2
    \$\begingroup\$ Regarding the variable comments, if you change the variable name to make it better, the comments shouldn't be there anymore. \$\endgroup\$ – IEatBagels Aug 14 at 13:39
  • \$\begingroup\$ Following this advice I have changed (copied) my program to what you have said. Doing so has provided me with some errors I'm unsure how to fix. In the 'public void hourConversion', lightYear couldnt be resolved and fixed this changing lightYear to LIGHT_YEAR, to make LIGHT_YEAR/vehicleMPH work. This provided new error, "cannot convert double to long" thuse changed the function double to long. No errors with this code but getting errors with 'lightYearInput' as it cannot be resolved. I am really unsure how to fix this and would appreciate anymore help you provide. \$\endgroup\$ – Hezekiah Sawyer yesterday
  • \$\begingroup\$ import java.util.Scanner; public class Applicationn { public static void main(String[] args) { double carMph = 277.87; Scanner lightYearInput = new Scanner(System.in); System.out.println("Enter amount of lightyears to convert: "); } public void hourConversion(long vehicleMph) { long LIGHT_YEAR = 6000000000000L; long light = lightYearInput.nextLong(); long speedConverted = LIGHT_YEAR / vehicleMph; System.out.println("Driving the fastest car in space, would take you " +speedConverted+" hours to travel "+light+ "light years."); } } \$\endgroup\$ – Hezekiah Sawyer yesterday
1
\$\begingroup\$

Constants should be Constant

You shouldn't be able to change constants; they should be fixed, non-modifiable values. In Java, this means declaring them final.

If you create two instances of your class, you'll end up with two instances of lightYear, hoursInDay and so on. But since these values are fixed compile-time constants, it doesn't make sense to have multiple copies of them. You just need one copy of these constants, which means declaring them as static.

Things like hours in a day, and days in a year are also fairly general values. They could be of use in other classes, so it makes sense to make these values sharable, which means public.

Finally, convention is to name constants with an UPPERCASE_NAME.

So ...

public final static long MILES_PER_LIGHT_YEAR = 5878625541248L;

public final static int  HOURS_PER_DAY = 24;
public final static int  DAYS_PER_YEAR = 365;
public final static int  YEARS_PER_CENTURY = 100;
public final static int  YEARS_PER_MILLENNIUM = 1000; // Note: 2 N's

Time Conversion

As noted by @Sixsmith, the Hours to Days, Years, Centuries, and Millennium calculation is being repeated. Unfortunately, they went a little far and added System.out.println("Driving the fastest car in space to their hoursConversion() function, so it is not general enough to use for the plane (or eventually, space probe).

Reducing the conversion code slightly produces a reusable function:

public static void output_hour_equivalents(long hours) {

    long days = hours / HOURS_PER_DAY;
    if (days > 0)
        System.out.println(hours + " hours is equivalent to " + days + " days.");

    long years = days / DAYS_PER_YEAR;
    if (years > 0)
        System.out.println(days + " days is equivalent to " + years + " years.");

    long centuries = years / YEARS_PER_CENTURY;
    if (centuries > 0)
        System.out.println(years + " years is equivalent to " + centuries + " years.");

    long millennia = years / YEARS_PER_MILLENNIUM;
    if (millennia > 0)
        System.out.println(years + " years is equivalent to " + millennia + " millennia.");
}

You could, of course, add "weeks" and "months" to this without loss of generality. I added tests at each conversion to avoid printing out a zero duration if given a small number of hours, like output_hour_equivalents(10000).

Duration for distance at speed

The conversion from speed and lightyears, for the fastest class of an object, to duration can be moved into a general purpose function. But we don't want to hard-code too much text. We want to fly a plane, not drive one, so we should take in the object class, and the action we perform with it, as well as it's maximum speed and the distance to be traversed:

public static void describe_fastest(String object, String action, double max_speed, int light_years) {
    long miles = light_years * MILES_PER_LIGHT_YEAR;
    long hours = (long)(miles / max_speed);

    // Capitalize the first letter of our action, for output. 
    action = action.substring(0, 1).toUpperCase() + action.substring(1);

    System.out.println(action + " the fastest " + object + " in space, would take you " + hours + " hours to travel " + light + " light years.");
    output_hour_equivalents(hours);
}

You can now use this in your method. For example, from main, borrowing @Sixsmith's variable names:

public static void main(String[] args) {

    int light_years = ...

    describe_fastest("car", "driving", carMph, light_years);
    describe_fastest("plane", "flying", planeMph, light_years);
    describe_fastest("space probe", "traveling in", spaceProbeMph, light_years);
}

1 Light Year

If you provide the value 1 for light_years you'll get something like:

Driving the fastest car in space ... to travel 1 light years.

I hate that "1 light years." It should be "1 light year." Singular. MessageFormat can help us here:

private final static String MSG_FORMAT =
    "{0} the fastest {1} in space, would take you {2} hours to travel " +
    "{3} {3,choice,0#light years|1#light year|1<light years}.";

public static void describe_fastest(String object, String action, double max_speed, int light_years) {
    long miles = light_years * MILES_PER_LIGHT_YEAR;
    long hours = (long)(miles / max_speed);

    // Capitalize the first letter of our action, for output. 
    action = action.substring(0, 1).toUpperCase() + action.substring(1);

    System.out.println(MessageFormat.format(MSG_FORMAT, action, object, hours, light_years));
    output_hour_equivalents(hours);
}

The format string constant is private, since it is unlikely that anyone else would re-use that string anywhere.

You can also add commas between groups of 3 digits, and other things to make the output a little more pleasing. See MessageFormat for details.

You can also make it output "hour" or "hours", "day" or "days", "year" or "years", "century" or "centuries", and "millennium" or "millennia" in the other messages.

Exercise left to student.

\$\endgroup\$
  • \$\begingroup\$ I've also attempted this method, thankyou for your time. I understood the content up until the second part in your 'Duration for speed at distance', I had an error with the variable 'light' in your print statement. Then the main method confused me, i'm unsure about the light_years int and I had errors with describe_fastest. "cannot make static reference" and "light_years cannot be resolved to variable". Apologies I am absolute begginer therefore new to Java and OOP but determined to get better, thankyou. \$\endgroup\$ – Hezekiah Sawyer yesterday

Your Answer

Hezekiah Sawyer is a new contributor. Be nice, and check out our Code of Conduct.

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.