3
\$\begingroup\$

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;
}
\$\endgroup\$
1
\$\begingroup\$

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).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ without len added the result might be negative on lower i. If I'm not mistaken, modulus on negative values is implementation defined. \$\endgroup\$ – Incomputable Dec 19 '16 at 6:39
  • \$\begingroup\$ 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. \$\endgroup\$ – Martin York Dec 19 '16 at 7:02
0
\$\begingroup\$
  • 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.

| improve this answer | |
\$\endgroup\$

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.