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

Computing the number of permutations of n consecutive numbers

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

Problem

I'm just starting to learn C++ and have been experimenting with various functions. Here is one that computes the number of permutations of n consecutive numbers as well as all possible permutations provided n. I'd love to receive any form of tips and criticisms on improving this function.

#include 
#include 
#include 
#include 
#include 

using namespace std;

string rotateRt(string input);
vector permute(int num);

int main(){
    cout > n;
    vector permutations(permute(n));
    ofstream myfile;
    myfile.open("rosalind_perm.txt");
    myfile  permute(int num){
    vector permutations;
    ostringstream convert;
    if(num == 1){
        permutations.push_back("1");
    }
    else{
        vector prevPermutations(permute(num - 1));
        int size = prevPermutations.size();
        string prefix, suffix, word;
        convert << num;
        prefix = convert.str();
        convert.str("");
        for(int i = 0; i < size; i++){
            suffix = prevPermutations.at(i);
            word = prefix +  " " + suffix;
            int length = word.length();
            for(int j = 0; j < length / 2 + 1; j++){
                permutations.push_back(word);
                word = rotateRt(word);
            }
        }
    }
    return permutations;
}

Solution

In addition to Jamal's good points:

1: Consider sorting your #includes alphabetically. It's not a big issue, but I think it looks more tidy, and it's easier to see if something is already included or not.

2: Consider typedef vector StringCollection (or some other meaningful name). This makes it easier to change the underlying data structure later, and makes the code look cleaner.

3: If you are using C++11, I'd write the for loop like this:

for (auto const& number : permutations) {
    myfile << number << endl;
}


Since you are learning, I see no reason not to get into C++11 right away.

4: Tutorials and the like often use mySomething as identifiers. my is a bad identifier prefix. Instead, write what role it has or what it represents. outputFile is much better than myFile.

5: Initialize variables right away. The std::ofstream constructor takes a filename, so there is no need to first initialize and then call open().

6: Instantiate variables as late as possible. In your rotateRt() (which should be called rotateRight(), by the way), you don't have to create your strings unless the last else triggers. Change it to this:

else {
    string a = input.substr(0, len - 2);
    string b = input.substr(len - 1) + " ";
    return b + a;
}


The same applies to for example ostringstream convert.

When you instantiate a variable as late as possible, you avoid constructing it unless you really have to. "As late as possible" implies as close as possible to the relevant context, which makes the code easier to read.

7: Consider rewriting permute() to reduce nesting.

8: Your code would normally be better suited as a class. You don't have to think too much about this yet, since you just started with C++. It's a good exercise to refactor this to use classes later.

9: In the case of int len = input.length();, you should use std::size_t instead of int.

10: Prefer C++-style comments: // (Effective C++ explains why, so I won't repeat it here.)

11: Use more whitespace. Especially more vertical whitespace. Insert an empty line in logical places to make your code much more readable.

12: You don't modify or copy the string you pass to rotateRt, so it should be reference-to-const instead: rotateRt(string const& input). (If you prefer, you can write const string&, it's the same thing.)

13: Consider rewriting permute() so that it's not recursive. Recursive functions that create a couple of objects can quickly chew up a lot of memory, and are often not very efficient. In short, your permute() function can be improved a great deal.

14: Consider refactoring the file name out into a constant:

const string filename = "rosalind_perm.txt";


This way it's easier to find if you have to change it, and by using the constant you are guaranteed you will only have to change it in one location.

Code Snippets

for (auto const& number : permutations) {
    myfile << number << endl;
}
else {
    string a = input.substr(0, len - 2);
    string b = input.substr(len - 1) + " ";
    return b + a;
}
const string filename = "rosalind_perm.txt";

Context

StackExchange Code Review Q#27859, answer score: 5

Revisions (0)

No revisions yet.