2
\$\begingroup\$

Below function takes an array, an element to be deleted and returns a new array minus the indicated element. Can this be optimized some how? How good is it generally?

public static int[] sbSample(int a[],int i){

    int size=0;
    int b[]=new int[a.length]; // provide length of new array
    for(int k=0;k<a.length;k++){    // iterate over elements of passed array
        if(!(a[k]==i)){    // search for elements other than to be deleted in passed array

            b[size]=a[k];     // store those arrays in new declared array
            size++;    // increment counter
        }
    }
    for(int k=0;k<size;k++){
        System.out.println(b[k]);
    }
    return b;
}
\$\endgroup\$
2
  • \$\begingroup\$ Optimised for what? Speed, memory usage, readability, clarity, computational complexity? \$\endgroup\$
    – Kaz
    Dec 7 '15 at 7:01
  • \$\begingroup\$ @Zak In such case, I'd say, safe to say that everything is up for optimization \$\endgroup\$
    – janos
    Dec 7 '15 at 7:13
3
\$\begingroup\$

The methodname sbSample doesn't tell anything about what the method is doing and this will make debugging the code very hard if not impossible.

You should leave your methodparameters some space to breathe so int a[],int i should be int a[], int i. The same is true for variables and operators.

Comments should tell the reader of the code why something is done in the way it is done. Stating the obvious what is done is only adding noise to the code and should be removed. Let the code itself tell what is done by using meaningful names for classes, methods and variables.

This method is doing to much hence it is breaking the single responsibility principle. It is "deleting" array elements and displaying the resulting array values.

If the source array has 10 elements and 3 of them have the value which you want to delete, the resulting array still has 10 elements which is somehow strange. Deleting means IMO that the size of the array should shrink afterwards. How about using an ArrayList<T> adding the items which aren't satisfying the search condition and later call toArray() to return the resulting array ?

\$\endgroup\$
1
\$\begingroup\$
  1. I recommend to do the System.out.println in a separate method (or simply use Arrays.toString(..)
  2. I recommend to use != instead of !(....=)...)
  3. I recommend to return an array with the size matching the new number of entries.
  4. even if a.length might be cheap to compute, you should avoid computing the same over and over again in the loop-condition.

    public static int[] sbSample(int a[], int i) {
        // Idea: store the not deleted elements in a temporary array
        // and at the end create on with the fitting size and return it.
        int len = a.length;
        int size = 0;
        int temp[] = new int[len];
        for (int k = 0; k < len; k++) {
            if (a[k] != i) {
                temp[size++] = a[k];
            }
        }
        int[] result = new int[size];
        System.arraycopy(temp, 0, result, 0, size);
        return result;
    }
    
\$\endgroup\$

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.