2
\$\begingroup\$

It is a code eval challenge question https://www.codeeval.com/open_challenges/14/. My answer was accepted, and I modified my code again to improve it. Please let me know is it possible to improve it more.

#include <iostream>
#include<fstream>
#include<algorithm>
#include<string>

void create_permute( std::string &record )
{
    if( !record.empty() ) 
    {
        std::sort( record.begin(), record.end() );
        bool flag=0;
        do{
            if( flag )
            {
                std::cout<<",";
            }
            flag = 1;
            std::cout << record;
        } while( std::next_permutation( record.begin(), record.end() ) );
        std::cout << "\n";
    }
}
void readInputFile( std::string filename ) 
{

    std::ifstream infile;
    infile.open( filename );
    std::string record;
    while( std::getline( infile,record ) )
    {         
       create_permute( record );
    }
}

int main( int argc, char* argv[] )
{
    if( argc < 2 )
    {
        std::cout << "usage: filesize filename" << "\n";
        exit( 0 );
    }
    std::ios_base::sync_with_stdio( false );
    readInputFile( argv[ 1 ] );

    return 0;
}
\$\endgroup\$
0

4 Answers 4

1
\$\begingroup\$

If you have preconditions return early

if( !record.empty() ) 
{
    // CODE
}

// If the precondition for doing work is that it is not empty
// Then test and return immediately.

if (record.empty()) {
    return;
}

// CODE

Now your code does not suffer lots of indenting and becomes easier to read.

Use constructors

std::ifstream infile;
infile.open( filename );

// Easier to read and write as:
std::ifstream infile( filename );

I think we can simplify that loop:

std::sort( record.begin(), record.end() );
std::cout << record;
while( std::next_permutation( record.begin(), record.end() ))
{
    std::cout << ", " << record
}
std::cout << "\n";
\$\endgroup\$
3
\$\begingroup\$

A boolean tested each time, and firing just once on the first iteration is unaesthetic (and a waste of time). Consider instead

    std::cout << record;
    while (std::next_permutation(record.begin(), record.end())) {
        std::cout << ',' << record;
    }
    std::cout << '\n';

Otherwise looks good. Congratulations for not using namespace std.

\$\endgroup\$
0
1
\$\begingroup\$

It's a good habit to eliminate special cases from your code. I don't see any particular reason to check for if ( !record.empty() ), so don't.

\$\endgroup\$
1
\$\begingroup\$

As long as you know for a fact that std::ifstream automatically at the end of void readInputFile( std::string ), there is nothing to add besides what already @vpn and @200_success said.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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