Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;
}
share|improve this question
    
Optimised for what? Speed, memory usage, readability, clarity, computational complexity? – Zak Dec 7 '15 at 7:01
    
@Zak In such case, I'd say, safe to say that everything is up for optimization – janos Dec 7 '15 at 7:13

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 ?

share|improve this answer
  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;
    }
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.