HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Program to rotate an array

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
arrayrotateprogram

Problem

I wrote this code where I have to rotate an array of 'n' elements by 'd' elements in the anti-clockwise direction.Can it be done in a better and simpler way?

public class RotateArr{
     public static void rotate(int[] arr,int d,int n){

    System.out.println("Array before rotating : ");

    for(int l=0; l<n; l++){
        System.out.print(arr[l]+" ");
    }
        System.out.println();

    int temp = arr[0];

    for(int i=0; i<d; i++){
        for(int j=1; j<n; j++){
            arr[j-1] = arr[j];
            if(j==(n-1)){
                arr[n-1] = temp;
            }
        }
        temp = arr[0];
    }

    System.out.println("Array after rotating");
    for(int k=0; k<n; k++){
        System.out.print(arr[k]+" ");
    }
}
    public static void main(String[] args) {

    int[] arr = {1,2,3,4,5,6,7};

    int d = 3;

    int n = arr.length;

    rotate(arr,d,n);
      }
   }

Solution

-
Separation of concerns. Ideally, one method should do one thing. I would expect the rotate method to actually rotate the array. That's it. Your method also prints something to the standard output. There are two reasons why it's bad. Firstly, it's confusing. Secondly, it introduces more reasons for the method to change. For instance, if you need to change the format of the output or write it to the file, you have to change the rotate method. It's not convenient, is it?

-
The interface is quite strange. The n parameter of the rotate method is redundant (it's expected to be equal to the length of the array, isn't it?). It also makes the interface less clear (what if I pass a different value of n?).

-
The algorithm itself is not efficient. Shifting the array be one d times requires O(n * d) time. There are at least two ways to make it linear. The first one is pretty simple. You can copy the entire array to a buffer and that assign res[i] = buffer[(i + d) % buffer.length] in a loop. However, it requires additional memory. The second one uses a constant amount of space and O(n) operations. It goes like this: to cyclically shift an array by k positions to the left, one can reverse the [0, k - 1] range, then reverse the [k, n - 1] range and finally reverse the entire array.

-
Documenting your code is a good practice. It's not clear what the valid range of values for d is from your code itself. It's also not clear whether it performs a left or a right shift.

-
Your code formatting violates the Java coding conventions. The indentation is kind of messed up. The binary operators are not surrounded with whitespaces. The names of the variables are not meaningful (for example, what does d stand for?). That's why it's hard to read.

Context

StackExchange Code Review Q#147338, answer score: 4

Revisions (0)

No revisions yet.