HackerRank, Cracking the Coding Interview: Arrays - Left Rotation

Question

I just wanted to get some constructive feedback on my solution for this problem taken from the HackerRank Cracking the Coding Interview series.

CTCI Arrays Left Rotation Problem Statement

#include <map>
#include <set>
#include <list>
#include <cmath>
#include <ctime>
#include <deque>
#include <queue>
#include <stack>
#include <string>
#include <bitset>
#include <cstdio>
#include <limits>
#include <vector>
#include <climits>
#include <cstring>
#include <cstdlib>
#include <fstream>
#include <numeric>
#include <sstream>
#include <iostream>
#include <algorithm>
#include <unordered_map>
using namespace std;
vector<int> array_left_rotation(vector<int> a, int n, int k) {
 vector<int> b(a.size());
 for(int i = 0, len = a.size(); i < len; i++) {
 int swapValue = (i + len - k) % len;
 b[swapValue] = a[i];
 }
 return b;
}
int main(){
 int n;
 int k;
 cin >> n >> k;
 vector<int> a(n);
 for(int a_i = 0;a_i < n;a_i++){
 cin >> a[a_i];
 }
 vector<int> output = array_left_rotation(a, n, k);
 for(int i = 0; i < n;i++)
 cout << output[i] << " ";
 cout << endl;
 return 0;
}

Answer 1

Don't use:

using namespace std;

See: Why is "using namespace std" considered bad practice?

Only include the headers you need. Also organizing them by length of name does not seem to have any benefits. Personally I group them by function others sort them alphabetically. Pick something logical.

Pass large objects by const reference rather than value.

vector<int> array_left_rotation(vector<int> const& a, int n, int k) {
 /// ^^^^^^

If you pass by value as in the code above. The compiler needs to add code to copy the object from the main function into array_left_rotation().

Prefer pre-increment:

for(int i = 0, len = a.size(); i < len; i++)
 // ^^^ prefer ++i

When using integers it makes no difference. But for other types it can potentially make a difference. So by using the pre-increment version you guarantee that you always use the most efficient version no matter what the type.

(削除) Not sure why we add len here:

int swapValue = (i + len - k) % len;

This is exactly the same as:

int swapValue = (i - k) % len;

(削除ここまで) Prefer to always use braces: '{' and '}' when you have a sub statements.

for(int i = 0; i < n;i++) 
 cout << output[i] << " ";

I would have used this:

for(int i = 0; i < n;i++) {
 cout << output[i] << " ";
}

Prefer \n over std::endl. THe only difference between the two is that std::endl will flush the stream. Since the code will automatically flush the stream at appropriate points (doing it manually usually results in efficiencies).

• Incomputable: without len added the result might be negative on lower i. If I'm not mistaken, modulus on negative values is implementation defined.
• Loki Astari: Actually modulus is well defined. From: 5.6 Multiplicative operators [expr.mul] if the quotient a/b is representable in the type of the result, (a/b)*b + a%b is equal to a; otherwise, the behavior of both a/b and a%b is undefined. BUT you are correct for negative numbers it will generate negative values.

Answer 2

• Use only necessary #includes.

  #include <vector>
  #include <iostream>
  

  are the only two you are actually using.

• The n argument to array_left_rotation is never used.

• Use correct types. int may not be enough to represent the array size and its indices. Their type is std::vector<int>::size_type.

• The wording of the problem statement suggests that the array shall be rotated in-place.