2
\$\begingroup\$

I was wondering about the Array built in methods in java 8. As there is no specific method other than Arrays.binarySearch. So for multidimensional array I wrote following method.

public int[] search(Integer[][] mat,int key)
    {
        int r=0,c=0;
        for(Integer[] a:mat)
        {
            if(Arrays.asList(a).contains(key)){
                c = Arrays.asList(a).indexOf(key);
                break;
            }
            else r++;
        }
        return new int[]{r+1,c+1};
    }

checking

 Integer[][] mat = {{1,2,3,99},{4,5,6,57},{7,8,9,89},{10,11,12,13}};
int [] rc = new int[2];
 rc = search(mat, 8);
 System.out.println(Arrays.toString(rc));
 rc = search(mat, 9);
 System.out.println(Arrays.toString(rc));
 rc = search(mat,10);
 System.out.println(Arrays.toString(rc));
 rc = search(mat, 1);
 System.out.println(Arrays.toString(rc));
   

output

[3, 2]
[3, 3]
[4, 1]
[1, 1]

I would like a review of efficiency and general code quality. Also any improvements I can do to make it better.

\$\endgroup\$
  • \$\begingroup\$ Hello, have you tested your method ? It will always return a wrong result. \$\endgroup\$ – dariosicily May 5 at 14:51
  • \$\begingroup\$ consider the output \$\endgroup\$ – Aashish Pawar May 5 at 15:08
  • \$\begingroup\$ Ok, but you are returning the couple of indexes in your array adding 1 to every index, so if you want to use the corresponding element you have to subtract 1 from both elements of the returned array before using them. \$\endgroup\$ – dariosicily May 5 at 15:17
  • \$\begingroup\$ see first eg key = 8 ans = [3,2] third row and 2 second column I'm not getting what are you saying \$\endgroup\$ – Aashish Pawar May 5 at 15:29
  • \$\begingroup\$ mat[3, 2] = 12, while mat[2, 1] = 8, you have to subtract 1 from both the elements of the returning array of your method. The array indices always start from 0. \$\endgroup\$ – dariosicily May 5 at 15:35
3
\$\begingroup\$

A few things to mention:

As has been mentioned, you're using 1-based indexing instead of 0-based indexing. 0-based is much more intuitive, since most implementations, of indexing in most programming languages, use 0-based indexing.

You're using the Arrays.asList twice for each iteration of the loop. This increases the inefficiency.

Calling indexOf inside the loop is basically creating another loop and doesn't gain anything a for loop wouldn't give you.

I think the name of your method could be better. search indicates all you want is the value. However you're returning the indexes. Therefore indexesOf might be more appropriate, and more in fitting with similar methods in other collections.

Your method is hard coded for int. This isn't very extensible. A generic version would probably be more fitting.

You're returning an int[]. This doesn't really tell anyone what this is supposed to represent. I would suggest a class(Indexes) to hold the indexes would allow you to represent them with a name that will immediately let the user know what it represents.

You haven't made any provision to handle failed searches.

It looks to me that the 2D array is basically a matrix. Therefore I would suggest labeling the indexes as such(row,column).

The Indexes class would look like this:

public class Indexes {
    int row = 0;
    int col = 0;

    public Indexes(int row, int col) {
        this.row = row;
        this.col = col;
    }

    public Indexes() {
    }

    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;
    }
    @Override
    public String toString(){
        return String.format("[%d, %d]",row,col);
    }
}

A generified version with the points I mentioned could look something like this:

public static<T> Indexes indexesOf(T[][] arr, T value){
    for(int row = 0; row < arr.length; ++row){
        for(int col = 0; col < arr[row].length; ++col){
            if(arr[row][col].equals(value)){
                return new Indexes(row,col);
            }
        }
    }
    return new Indexes(-1,-1);
}

EDIT Alternative code based on OP's comments

static final int DEFAULT_RETURN_VALUE = -1;
public static<T> Indexes indexesOf(T[][] arr, T value){
    for(int row = 0; row < arr.length; ++row){
            int col = indexOf(arr[row],value);
            if(col > DEFAULT_RETURN_VALUE){
                return new Indexes(row,col);
            }
    }
    return new Indexes(DEFAULT_RETURN_VALUE,DEFAULT_RETURN_VALUE);
}
public static<T> int indexOf(T[] arr, T value){
    for(int i = 0; i < arr.length; ++i){
        if(arr[i].equals(value)){
            return i;
        }
    }
    return DEFAULT_RETURN_VALUE;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I think Location might be a better name for Indexes? \$\endgroup\$ – RobAu May 5 at 19:51
  • \$\begingroup\$ There are probably quite a few different names that could be applied. I think the point is having a name that indicates what values it holds. \$\endgroup\$ – tinstaafl May 5 at 20:06
  • \$\begingroup\$ I got your point about repeated use of asList and zero based indexing . any other suggestions. I used asList because I don't wanted to explicit searching code that you mentioned above \$\endgroup\$ – Aashish Pawar May 6 at 6:59
  • \$\begingroup\$ @AashishPawar - As my answer explains indexOf is hiding an internal loop. The effect is exactly the same as my code. The performance with just the loop is actually better, since it doesn't copy the arrays into a list. If you want the same simplicity, extract the inner into a function and call it from the outer loop. \$\endgroup\$ – tinstaafl May 6 at 15:29
  • \$\begingroup\$ Sorry, but saying Arrays.asList() was very inefficient is simply wrong. There will be a new Arrays$ArrayList inner class created for each call, but this is just a simple wrapper around the array. Yes, you could spare a few CPU cycles by reusing the result, but there is only constant (O(1)) runtime for this call. It is especially not creating a copy. \$\endgroup\$ – mtj May 7 at 6:07

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.