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

Permutation algorithm

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

Problem

I was asked to write a permutation algorithm to find the permutations of {a,b,c}. Since this is a famous question to which an answer is readily available online, I wanted to do it a little differently, so that it won't look like I copied off the Internet. It was evaluated as OK for the algorithm being correct, but said that the algorithm is inefficient and also there are 'major concerns' in the code.

I would really appreciate if the experts here would advice me on what is wrong about this code, so that I can learn and fix my mistakes. I understand that the use of templates is an overkill, but other than that, what is so wrong about this code? The question was, find the permutations of {a,b,c}.

```
#include
#include
#include
#include
using namespace std;

template vector TruncateVector(vector, int);
template vector > GetPerms(vector);
unsigned GetNumPerms(unsigned);
template
bool ValidatePermutations(vector, vector >);

int main()
{
// Create a vector with the elements required to permute
vector permSet;
permSet.push_back('a');
permSet.push_back('b');
permSet.push_back('c');

// Get the permutations
vector > vectorOfPerms = GetPerms(permSet);

/ Printing the permutations /
for(unsigned x=0, counter=1;x
vector > GetPerms(vector permSet)
{
/* --------------- Permutation algorithm -----------------
* In this algorithm permutations are calculated by sequentially taking each element in the vector,
* and then combining it with 'all' the permutations of the rest of the vector elements.
* Example:
* GetPerms('a,b,c') --> ( {a, GetPerms('b,c')}, {b, GetPerms('a,c')}, {c, GetPerms('a,b')} }
* The resultant permutations are returned as a vector of vectors, with each vector consisting of one permutation.
*
* Vectors were chosen to store the elements because of its ease of handling (inserting, deleting) and also for its ability
* to handle different types of dat

Solution

-
Prefer the preincrement operator (++variable) in place of the postincrement operator (variable++).

-
Prefer range based for loops or std::for_each so that you can allow the compiler to handle container bounds checking for you. The compiler will be more efficient at this than you can, and can potentially perform loop unrolling especially in the case of std::for_each.

-
Prefer to use std algorithms over writing your own for loops. std::find_if or std::any_of are good for finding elements in a container that match a condition. std::generate and std::transform are good for filling a container with values from a function or another container.

-
Instead of:

// Create a vector with the elements required to permute
vector permSet;
permSet.push_back('a');
permSet.push_back('b');
permSet.push_back('c');


-
Prefer to initialize directly using uniform initialization such as:

vector permSet{'a', 'b', 'c'};


Or better yet (if you know that you won't modify permSet):

const vector permSet{'a', 'b', 'c'};

Code Snippets

// Create a vector with the elements required to permute
vector<char> permSet;
permSet.push_back('a');
permSet.push_back('b');
permSet.push_back('c');
vector<char> permSet{'a', 'b', 'c'};
const vector<char> permSet{'a', 'b', 'c'};

Context

StackExchange Code Review Q#36786, answer score: 5

Revisions (0)

No revisions yet.