FIZZ BUZZ -challenge program with adjustable intervals

This is again a solution to the codeEval challenge #1. It was accepted by codeEval, but please let me know if I can improve it anyway somehow?

#include
#include
#include
#include
#include

struct input_rec{
    int first;
    int second;
    int last;
};

void parseRecord( std::string &record, input_rec& rec )
{
    int first  = 0;
    int second = 0;
    int last   = 0;
    bool good  = 0;

    size_t start = 0;
    size_t end =  record.find_first_of(" ");
    first = std::atoi(record.substr(start,end-start).c_str());
    if( ( 1  & rec )
{

    //std::cout input;
    readInputFile( argv[ 1 ], input );    
    for( auto &x : input)
    {
       checkfuzzbuzz(x);
    }
   auto end      = std::chrono::steady_clock::now();
   auto duration = std::chrono::duration_cast(end-start);
   std::cout<< duration.count() << "\n";
   return 0;
}

Sort your includes.

Your input_rec has poor names. Something like fizz, buzz and length would be better. If you don't want to give it better names, just use a tuple.

parseRecord takes a line and tries to fill a record. It would be more senible to just return a record (out-parameters are largely a bad idea for small types). This does mean you actually need to handle the early-return cases. For the moment, I'll just return {0, 0, 0} - this is at least better than the old arbitrary-data return value. The input should probably be taken by const &.

The constraints

• The number of test cases ≤ 20

• "X" is in range [1, 20]

• "Y" is in range [1, 20]

• "N" is in range [21, 100]

should not be part of your code. They only serve to make it less general.

This gets us to

input_rec parseRecord( std::string const &record )
{
    int fizz = 0;
    int buzz = 0;
    int length = 0;

    size_t start = 0;
    size_t end =  record.find_first_of(" ");
    fizz = std::atoi(record.substr(start,end-start).c_str());

    start = end + 1;
    end = record.find_first_of( " ", start );
    buzz = std::atoi( record.substr( start, end-start ).c_str() );

    start = end + 1;
    length = std::atoi( record.substr( start, std::string::npos ).c_str() );

    return {fizz, buzz, length};
}

The next thing to do is initialize late:

input_rec parseRecord( std::string const &record )
{
    size_t start = 0;
    size_t end =  record.find_first_of(" ");
    int fizz = std::atoi(record.substr(start,end-start).c_str());

    start = end + 1;
    end = record.find_first_of( " ", start );
    int buzz = std::atoi( record.substr( start, end-start ).c_str() );

    start = end + 1;
    int length = std::atoi( record.substr( start, std::string::npos ).c_str() );

    return {fizz, buzz, length};
}

This is much simpler with an istringstream:

#include

input_rec parseRecord( std::string const &record )
{
    std::istringstream stream(record);

    int fizz, buzz, length;
    stream >> fizz >> buzz >> length;
    return {fizz, buzz, length};
}

Your readInputFile isn't needed - one can interleave reading and writing. Letting checkfuzzbuzz take its argument by value, this is just:

std::ifstream infile( argv[ 1 ] );

std::string record;
while( std::getline( infile, record ) )
{
    checkfuzzbuzz(parseRecord( record ));
}

Your exit( 0 ); in main can jus be return;, although you probably want return 1;.

checkfuzzbuzz has a typo. Further, the whole flag setting things is meaningless overhead. Just do

void checkfizzbuzz( input_rec rec )
{ 
    int x = 1;
    while( x < rec.length )
    {
        if( ( x % rec.fizz ) == 0 && ( x % rec.buzz ) == 0 ) {
            std::cout << "FB" << " ";           
        }
        else if( ( x % rec.fizz ) == 0)
        {
            std::cout << "F" << " ";
        }
        else if ( ( x % rec.buzz ) == 0 )
        {
            std::cout << "B" << " ";
        }
        else {
            std::cout << x << " ";
        }
        ++x;
    }
    if( x == rec.length )
    {
        if( ( x % rec.fizz ) == 0 && ( x % rec.buzz ) == 0 ) {
            std::cout << "FB";           
        }
        else if( ( x % rec.fizz ) == 0)
        {
            std::cout << "F";
        }
        else if ( ( x % rec.buzz ) == 0 )
        {
            std::cout << "B";
        }
        else {
            std::cout << x;
        }
    }
    std::cout << "\n";
}

Further, you can combine the two to do

void checkfizzbuzz( input_rec rec )
{ 
    for( int x = 1; x <= rec.length; ++x )
    {
        if( ( x % rec.fizz ) == 0 && ( x % rec.buzz ) == 0 ) {
            std::cout << "FB";           
        }
        else if( ( x % rec.fizz ) == 0)
        {
            std::cout << "F";
        }
        else if ( ( x % rec.buzz ) == 0 )
        {
            std::cout << "B";
        }
        else {
            std::cout << x;
        }

        if( x < rec.length ) {
            std::cout << " ";
        }
    }
    std::cout << "\n";
}

You can drop the return 0;.

You should really find a consistent formatting. Since I'm lazy, I just ran it through clang-format with the default settings (LLVM-style) except with 4-space tabs.

It's probably best not to call std::ios_base::sync_with_stdio(false); - there's no real reason to avoid flushing unless you are actually needing the speed. I also removed the timing code, since it seems to be for debugging purposes.

This all gives:

```
#include
#include
#include

struct input_rec {
int fizz;
int buzz;
int length;
};

input_rec parseRecord(std::string const &record) {
std::istringstream stream(record);

int fizz, buzz, length;
stream >> fizz >> buzz >> length;
return {fizz, buzz, length};
}

void checkfizzbuzz(input_rec rec) {
for (int x = 1; x <= rec.length; ++x) {
if ((x % rec.fizz) == 0 && (x % rec.buzz) == 0) {
std::cout << "FB

StackExchange Code Review Q#90262, answer score: 8