4
\$\begingroup\$

Can I get feedback on my code?

Problem:

A zero-indexed array A consisting of N integers is given. Rotation of the array means that each element is shifted right by one index, and the last element of the array is also moved to the first place.

For example, the rotation of array A = [3, 8, 9, 7, 6] is [6, 3, 8, 9, 7]. The goal is to rotate array A K times; that is, each element of A will be shifted to the right by K indexes.

This is one of the lessons on Codility

Here is my solution:

public int[] solution(int[] A, int K) {
    // write your code in Java SE 8
    int size = A.length;
    int[] cyclic = new int [size];
    for (int x=0; x<size; x++) {
        cyclic[(x + K) % size] = A[x]; 
    }

    return cyclic;
}
\$\endgroup\$
3
\$\begingroup\$

You have to do a modulo division for each element of your array. An alternative would be to split it into two subsequent loops:

int size = A.length;
int[] cyclic = new int[size];
K %= size; // thanks to @Josiah for pointing on the case K>sze
for (int i=0; i<size-K; ++i) 
{
    cyclic[K + i] = A[i]; 
}

for (int i=0; i<K; ++i)
{
    cyclic[i] = A[size - K + i]; 
}

Regarding naming see the answer of @Timothy Truckle

\$\endgroup\$
  • 1
    \$\begingroup\$ I must say your approach is pretty different to what a normal person would have come up with. It confused me at first until I sat down and did a 'dry run' on it. +1 for thinking from a different angle and letting me see it. \$\endgroup\$ – a-ina Jul 31 '17 at 11:00
  • 1
    \$\begingroup\$ Please note that this as written has a bug. You could be passed a value of K which is greater than the size of the array, and crash by going out of bounds. (The fix is simply to do one modulo division of K by size before starting.) \$\endgroup\$ – Josiah May 5 '18 at 8:25
  • 1
    \$\begingroup\$ That said, this is preferable as an approach from a runtime efficiency perspective. If developing this for real I would start by thinking like this to see how the control flow fits together. Then I'd replace both loops with the appropriate toSystem.arraycopy() for further speed up. \$\endgroup\$ – Josiah May 5 '18 at 8:27
  • 1
    \$\begingroup\$ @Josiah Thanks for point on the case K>size. Sure, arraycopy would be an improvement but I wanted to stay next to the original solution. As it's the solution of a lesson, "keep it simple". \$\endgroup\$ – milbrandt May 5 '18 at 19:52
2
\$\begingroup\$

Thanks for sharing your code.

The big issue with it is Naming

Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.

Java Naming Conventions.

Please read (and follow) the Java Naming Conventions. The parameter names should start with lowercase letters.

avoid single letter names

In Java the length of identifier names names is virtually unlimited. There is no penalty in any way for long identifier names. So don't be stingy with letters when choosing names.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the quick feedback. At Codility, you're given code skeleton and you just have to write the main code snippet. I understand your point and I am a strong advocate of using proper variable names, but in this case, Codility provided me with the variable name 'A' and 'K'. My code is the one that's written inside the function \$\endgroup\$ – a-ina Jul 17 '17 at 20:06
  • 1
    \$\begingroup\$ @a-ina "At Codility, you're given code skeleton" But as this is java after all you are free to change the parameter names as they are local to your code... \$\endgroup\$ – Timothy Truckle Jul 17 '17 at 20:09
  • \$\begingroup\$ yes, you're right. From now on, I'll change these bits too \$\endgroup\$ – a-ina Jul 18 '17 at 4:12
  • \$\begingroup\$ @a-ina but the name of your targte array isn't that good either, should better be rotatedArray or similar. \$\endgroup\$ – Timothy Truckle Jul 18 '17 at 7:47
-1
\$\begingroup\$

Your code looks nice though I'd change a few small bits related to code formatting.

I'd change:

  • int[] cyclic = new int [size] remove space in int [size] so it'll become int[size]
  • add a new line under int[] cyclic = ... so you have a bit of separation so it's just a bit better structured
  • remove the braces near the for-loop so it doesn't add unnecessary noise to the code.

These are my personal recommendations only though

\$\endgroup\$
  • \$\begingroup\$ Removing braces from loops and conditionals is at best a very contentious recommended. It is a very common source of bugs, especially the sort of bugs that appear as you edit the code. For example Google's Style guide for Java says "Braces are used with if, else, for, do and while statements, even when the body is empty or contains only a single statement." (google.github.io/styleguide/javaguide.html#s4.1-braces) \$\endgroup\$ – Josiah May 5 '18 at 8:18

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.