1
\$\begingroup\$

I'm pretty new to Java and programming in general and I'm doing practice problems from my textbook over the summer to try to get ahead. This is a simple program that finds the largest value in a 2D array. I'm just looking for feedback on the organization of my code as well as any improvements I can make so that I can build solid habits going forward.

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

    Scanner input = new Scanner(System.in);
    System.out.println("Enter the number of rows and columns in the array: ");

    //Creates a temporary two-dimensional array whose size is the next two inputed integers
    double[][] temp = new double[input.nextInt()][input.nextInt()];

    //Populates the temp array with inputed values
    System.out.println("Enter the array: ");
    for(int i=0; i < temp.length; i++) {
        for(int j=0; j < temp[0].length; j++) {
            temp[i][j] = input.nextDouble();
        }
    }
    input.close();

    //Creates a new Location object that is equal to the object returned from the locateLargest() method call
    Location location = Location.locateLargest(temp);               

    System.out.println("The location of the largest element is " + location.maxValue + " at (" + location.row + ", " + location.column + ")");



    }
}

public class Location {
    public int row=0, column=0;
    public double maxValue = 0;

    public Location() {
    }

    /*
     * Accepts a two-dimensional array as the parameter.
     * Creates a new Location object.
     * Returns a Location object with maxValue at position (0,0) unless a larger value is found.
     */
        public static Location locateLargest(double[][] a) {
        Location location = new Location();        
        location.maxValue=a[0][0];

            for(int i = 0; i < a.length; i++) {
                for(int j=0; j < a[i].length; j++) {
                    if(a[i][j] > location.maxValue) {
                        location.maxValue = a[i][j];
                        location.row = i;
                        location.column = j;
                    }
                }

            }

            return location;
       }

    }
\$\endgroup\$
0
3
\$\begingroup\$

Obligatory disclaimer: My opinion.


I can see that you are a beginner - it's a little hard to explain what you should work on, because the path you're taking toward finding the highest value in an array is a little hard to understand.

Normally, you'd just need to do something like

public static double findHighestValue(double[][] doubles) {
    double currentHighestValue = Double.MIN_VALUE;
    for (int row = 0; row < doubles.length; row++) {
        for (int col = 0; col < doubles[row].length; col++) {
            double value = doubles[row][col];
            if (value > currentHighestValue) {
                currentHighestValue = value;
            }
        }
    }
    return currentHighestValue; 
}

to find the maximum value of a 2D array (which is the question), but instead, you're returning an object Location.

I can understand where you got the class name idea -- locating a value -- however, it's counter-intuitive. Instead, a class name such as Case or Square would be a bit more descriptive due to the fact that the noun location has multiple meanings.

I'll show you what I would do to answer the question (finds the largest value in a 2D array), and then, to help you learn, I'll give you some tips on the code you already wrote.

As this question is extremely wide, I won't go as far as to explain everything, but I'll give you a few pointers.

What I would do

This isn't a really complex problem per se, so there's not a lot to explain.

Since you want to know the coordinates of the maximum value (column and row), we still need to create a custom class.

First, you must determine what the class represents. In this case, it represents an element in a multidimensional array, so like I said earlier, I'll just use something a little bit more intuitive like a Case.

Why is Case a better name?

If you visualize the 2D array, it looks like this:

 _ _ _
|_|_|_|
|_|_|_|
|_|_|_|

And each square looks like a case.

Each case contains a value, and a position determined by the row number and the column number of the case, so those 3 parameters will be our fields:

public class Case {

    private int row;
    private int col;
    private double value;

    public Case(int row, int col, double value) {
        this.row = row;
        this.col = col;
        this.value = value;
    }

    public int getRow() {
        return row;
    }

    public void setRow(int row) {
        this.row = row;
    }

    public int getCol() {
        return col;
    }

    public void setCol(int col) {
        this.col = col;
    }

    public double getValue() {
        return value;
    }

    public void setValue(double value) {
        this.value = value;
    }

}

With this new class in mind, the method we would use to find the case with the highest value would be something like this:

public static Case findHighestValue(double[][] doubles) {
    Case highestCase = new Case(-1, -1, Double.MIN_VALUE);
    for (int row = 0; row < doubles.length; row++) {
        for (int col = 0; col < doubles[row].length; col++) {
            double value = doubles[row][col];
            if (value > highestCase.getValue()) {
                highestCase = new Case(row, col, value);
            }
        }
    }
    return highestCase;
}

I would put that in an interface named something like CaseUtils (unless you have another class related to cases, e.g. CaseService, but since you initially only had a Main class and a Location class, I assume there isn't any), which would look like this:

public interface CaseUtils {

    static Case findHighestValue(double[][] doubles) {
        Case highestCase = new Case(-1, -1, Double.MIN_VALUE);
        for (int row = 0; row < doubles.length; row++) {
            for (int col = 0; col < doubles[row].length; col++) {
                double value = doubles[row][col];
                if (value > highestCase.getValue()) {
                    highestCase = new Case(row, col, value);
                }
            }
        }
        return highestCase;
    }

}

That way, if you want to add more methods of the same type (e.g. findLowestValue), you'll be able to add them there too.

As for how to implement it in your main, you just need to do something like this:

Case c = CaseUtils.findHighestValue(array);
System.out.println("The position of the case with the highest value is " + c.getValue()
        + " at (" + c.getRow() + ", " + c.getCol() + ")");

Tips on your current code

Your comments on the method locateLargest do not conform with the javadoc convention:

/*
 * Accepts a two-dimensional array as the parameter.
 * Creates a new Location object.
 * Returns a Location object with maxValue at position (0,0) unless a larger value is found.
 */

should instead be something like:

/**
 * Locates the largest double
 * @param a two-dimensional array as the parameter.
 * @return Location object with maxValue at position (0,0) unless a larger value is found.
 */

If you want to read more about Javadoc, I suggest you read this article.

The class Location should not have public fields, but private fields with public getters and setters instead. As for why, you can search online, but this is also a good explanation: Why do we need private variables?

If you have any questions, feel free to ask.

NOTE

I used the name Case to highlight that the class name should clearly explain what it is. However, I wouldn't personally use that class name and instead, I would use Cell because the keyword case is reserved (switch case).

\$\endgroup\$
2
  • \$\begingroup\$ personally I would not use Square as it is related to the display and graphics terminology. perhaps Cell is more appropriate (think of the 2d array as a spreadsheet containing cells) \$\endgroup\$ – Sharon Ben Asher May 30 '18 at 6:56
  • \$\begingroup\$ You're right, I had totally forgotten about the existence of that word \$\endgroup\$ – TwiN May 30 '18 at 11:47

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.