7
\$\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;
}
Improve this question
\$\endgroup\$

3 Answers 3

Reset to default
6
\$\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

Improve this answer
\$\endgroup\$
2
  • 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, 2017 at 11:00
  • 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, 2018 at 8:27
3
\$\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.

Improve this answer
\$\endgroup\$
4
  • \$\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, 2017 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, 2017 at 20:09
  • \$\begingroup\$ yes, you're right. From now on, I'll change these bits too \$\endgroup\$
    – a-ina
    Jul 18, 2017 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, 2017 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

Improve this answer
\$\endgroup\$
1
  • 1
    \$\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, 2018 at 8:18

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