More Efficient Way? Project in Java non related to CodeCademy


#1

I have a project from school that is finished and graded. I got a perfect score, but the teacher cares a lot more about the output than the efficiency. I know that efficiency is not always important. In my case, the job is done, and I could move on, but I just feel like my code is unsightly, and I have SO MANY VARIABLES that are for my for loops to count and what not that I just have names everywhere, and I'm wondering if there is an easier way to do it. This is in Java, but if there are better examples of certain aspects in other languages, I can read them too.
Additionally, this is no priority, but I know that some people like to refactor code and it would be cool to see a better way to do this.
Last note, I know that I could have used the built in functions of min and max, but part of the project was to not use them. If however, you want to include them refactored, that would be awesome, as I would be able to see how they work.

/* Kylea Watkins
 * November 9, 2016
 */


import java.util.Scanner;
import java.io.File;
import java.io.IOException;

public class Hurricanes2 {
    public static void main(String[] args)throws IOException {
    	  //declare and initialize variables
      
        int arrayLength = 59;
        int [] year = new int[arrayLength];
        String [] month = new String[arrayLength];
        int [] pressure = new int[arrayLength];
        int [] knots = new int[arrayLength];
        String [] name = new String[arrayLength];

        File fileName = new File("hurcdata2.txt");
        Scanner inFile = new Scanner(fileName);

        //INPUT  - read data in from the file
        int index = 0;
        while (inFile.hasNext())
        {
            year[index] = inFile.nextInt();
            month[index] = inFile.next();
            pressure[index] = inFile.nextInt();
            knots[index] = inFile.nextInt();
            name[index] = inFile.next();
            index++;
        }
        inFile.close();
      
      int a = 0;
      int b = 0;
      int c = 0;
      int d = 0;
      int e = 0;
      int [] category = new int[arrayLength];
      
		//PROCESSING - calculate and convert values
		// convert windspeed from knots to MPH
      for(int numberss = 0; numberss < arrayLength; numberss++) {
         knots[numberss] = (int)(knots[numberss] * 1.15);
      }
      int x = 0;
		// determine category
      for(double speed : knots) {
         if (speed <= 95) {
            category[x] = 1;
            a++;
         }
         else if (speed <= 110) {
            category[x] = 2;
            b++;
         }
         else if (speed <= 129) {
            category[x] = 3;
            c++;
         }
         else if (speed <= 156) {
            category[x] = 4;
            d++;
         }
         else {
            category[x] = 5;
            e++;
         }
         x++;        
      }
		// count number of each category
      
      //Find min, max and average for category, windspeed and pressure
      int sumCat = 0;
      int avgCat = 0;
      int minCat = 1;
      int maxCat = 5;
      sumCat = a + b*2 + c*3 + d*4 + e*5;
      avgCat = sumCat/59;
      
      int sumWind = 0;
      int avgWind;
      Integer minWind = Integer.MAX_VALUE;
      for(int i = 0; i < arrayLength; i++) {
         if (knots[i] < minWind) {
            minWind = knots[i];
         }
      }
      Integer maxWind = Integer.MIN_VALUE;
      for(int numb = 0; numb < arrayLength; numb++) {
         if (knots[numb] > maxWind) {
            maxWind = knots[numb];
         }
      }
      for (int windSpeed : knots) {
         sumWind += windSpeed;
      }
      avgWind = (int)((double)sumWind/59);
      
      int sumPressure = 0;
      int avgPressure;
      Integer minPressure = Integer.MAX_VALUE;
      for(int counter = 0; counter < arrayLength; counter++) {
         if (pressure[counter] < minPressure) {
            minPressure = pressure[counter];
         }
      }
      Integer maxPressure = Integer.MIN_VALUE;
      for(int count = 0; count < arrayLength; count++) {
         if (pressure[count] > maxPressure) {
            maxPressure = pressure[count];
         }
      }
      for(int nums : pressure) {
         sumPressure += nums;
      }
      avgPressure = (int)((double)sumPressure/59);
      
      //Output - print table using printf to format the columns
      
   
      System.out.println("                      Hurricanes 1980 - 2006");
      System.out.println();
      System.out.println("Year     Hurricane    Category     Pressure (mb)     Wind Speed (mph)");
      System.out.println("=====================================================================");
      for(int counting = 0; counting < arrayLength; counting++) {
         System.out.printf("%4d %12s %8d %15d %18d \n", year[counting], name[counting], category[counting], pressure[counting], knots[counting]);
      }
      System.out.println("=====================================================================");
      System.out.printf("%7s %18d %15d %18d \n", "Average", avgCat, avgPressure, avgWind);
      System.out.printf("%7s %18d %15d %18d \n", "Maximum", maxCat, maxPressure, maxWind);
      System.out.printf("%7s %18d %15d %18d \n", "Minimum", minCat, minPressure, minWind);
      System.out.println();
      System.out.println("Number of Category 1: " + a);
      System.out.println("Number of Category 2: " + b);
      System.out.println("Number of Category 3: " + c);
      System.out.println("Number of Category 4: " + d);
      System.out.println("Number of Category 5: " + e);

    }//end main()
}//end Hurricanes2

hurcdata2.txt (1.3 KB)

The attachment is the file that I'm reading from. :slight_smile:


#2

That same information is in your categoy array, these variables (a-e) can be removed
sumCat = a + b*2 + c*3 + d*4 + e*5;


if (speed <= 95) {
That number literal could be a constant instead, so that the above would read:
if (speed < CATEGORY_B_SPEED)
or some variation of that whatever makes more sense


year[index] = inFile.nextInt();
month[index] = inFile.next();
pressure[index] = inFile.nextInt();
knots[index] = inFile.nextInt();
name[index] = inFile.next();

Those could be put in a Hurricane object instead:
hurricanes.add(new Hurricane(year, month, pressure, knots, name));


int arrayLength = 59;
That's out of nowhere. Is that the amount of hurricanes in the data? What if the data is changed? Use a list instead (ArrayList<Hurricane>)



//declare and initialize variables
Explain what action is being carried out, not the language itself


Sometimes you use for-each and sometimes a regular for loop, that could be more consistent. I'd use for-each whenever the index itself isn't needed.


(int)((double)sumPressure/59);
Skip the casting


//end main()
I'm not a fan of doing that


for(int numberss = 0;
Is that a typo or are you creating two variables with almost the same name? There are more places with this, using a few other names. Use i or foreach instead


File fileName = new File("hurcdata2.txt");

Unless it's specifically gotta be file io and with that file in particular, then reading from stdin would make it more flexible.


whitespace - you've got some tab characters at some of the comments, and also some trailing whitespace here and there


String [] name
snuggle the brackets with the type


finding minimum and maximum can be done in the same loop


knots[numberss] = (int)(knots[numberss] * 1.15);
if it's not knots any more then it shouldn't go into that variable, it could be renamed to be more general as an alternative solution


A brief comment at the top of the file about what the program does would be good, both to set expectations about what's in the code and to help decide whether someone would want to use this program.


for(
get a space in there, it's not a function call


#3

honestly I'm super excited about your feedback. This was almost a month ago, so some of my practices have changed, but not all of them.
I really do appreciate this and I'm going to try to implement this into my every day coding.
I'll keep this nearby.
There was a lot that I knew was not only inneficient, but counterproductive and unneeded, but I couldn't point out exactly what I didn't need. This helps tremendously and I also recognize the time that this took.
I do have a question about you saying

I'm wondering if it's a personal preference, or something that is bad. With java, there are just so many brackets that I try to keep up with them sometimes. Additionally some of this was because it was a outline file from my teacher to have a starting point and do the work inside of.
Anyways, thanks again for the feedback, I appreciate your help.
This was exactly what I was looking for.


#4

If by efficiency you mean execution performance, then you've only got two significant costs, startup and IO, the rest takes about a millisecond to run

And I expect that if considering what is being done here:

int sumWind = 0;
int avgWind;
Integer minWind = Integer.MAX_VALUE;
for(int i = 0; i < arrayLength; i++) {
   if (knots[i] < minWind) {
      minWind = knots[i];
   }
}
Integer maxWind = Integer.MIN_VALUE;
for(int numb = 0; numb < arrayLength; numb++) {
   if (knots[numb] > maxWind) {
      maxWind = knots[numb];
   }
}
for (int windSpeed : knots) {
   sumWind += windSpeed;
}

Then it's clear that this could be one loop instead of three. (except that arrayLength is coming out of nowhere, that variable shouldn't exist at all)

It would be reasonable to argue that they are three separate actions and to therefore put them in their own loops instead of cramming them all together to create something complicated. I'd rather do them all at once in this case.


..and:
that avgWind variable is declared too early, it isn't being used until after those loops.
You're mixing 3 and 4 width indentation (your editor settings + habits should make that a non-issue)


  int x = 0;
  for(double speed : knots) {

should be for (int i = 0; i < knots.size(); ++i)


I don't think that solves a problem, to me that looks like some mix of:
- someone's commenting for the sake of it (as opposed to commenting to improve understanding of something)
- someone is struggling with brackets in some way and should probably just learn their editor better
- someone recognises that their code is a mess and puts that comment there and somehow fools them self into thinking that's helpful to somebody else

It's cringe-worthy. It's not there because it's the best solution to a problem that's reasonable to have. It's like a grocery clerk summing up the price by counting on their fingers, yikes, did they even get that right?

That said, perhaps it can be useful at times. Fingers can be a useful memory aid. But you'd rather be able to do it without fingers, or eliminate the problem