This is a challenge from codeeval

Write a program which determines the Mth to the last element in a list.

Input sample:

The first argument is a path to a file. The file contains the series of space delimited characters followed by an integer. The integer represents an index in the list (1-based), one per line.

For example:
a b c d 4

e f g h 2

Output sample:

Print to stdout the Mth element from the end of the list, one per line. If the index is larger than the number of elements in the list, ignore that input.

For example:
a

g

Please let me know can I improve it.

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

void processRecords( const std::string& record )
{
     auto splitLoc= record.rfind(" ");
     std::string::size_type index = std::stoi( record.substr( splitLoc ) );
     std::string str( std::begin( record ), std::begin( record ) + splitLoc );
     str.erase( std::remove_if( str.begin(), str.end(), []( const char& val )
                                  { return std::isspace( val );}), str.end() );

     if( str.size() >= index )
     {
          std::cout << str[ str.size() - index ] << "\n";
     }
}

void readInputFile( const std::string fileName )
{
     std::ifstream infile( fileName );
     std::string record;

     while( std::getline( infile, record ) )
     {
         processRecords( record );
     }
     infile.close();
}

int main( int argc, char* argv[] )
{
    if( argc < 2 )
    {
        std::cout << "Usage: " << argv[0] << " input_file_name\n";
        exit( 0 );
    }

    std::ios_base::sync_with_stdio( false );
    readInputFile( argv[ 1 ] );
}
  • Your readProcessInputFile violates both Single Responsibility and Open/Closed principles. You should really split it into 3 functions: read, findMth, display – Daniel Sokolov Jul 27 '15 at 1:23
  • @DanielSokolov All suggestions for improvements belong in answers, not comments, even if it's that short. – 200_success Jul 28 '15 at 20:56
up vote 2 down vote accepted
  • processRecords() is a very generic name and doesn't say what is being done. A better name would be displayMthLastItem() (assuming you don't follow Daniel Sokolov's advice and split the function out further into the find and display operations).

  • Spacing around function arguments is a little inconsistent:

    auto splitLoc= record.rfind(" ");  // NO SPACE
    std::string::size_type index = std::stoi( record.substr( splitLoc ) );  // HAS SPACE
    
  • index has type std::string::size_type. This is generally only needed when indexing into a std::string. The problem definition says that it's an integer, so either declare it as an int or, since you're using C++11, as auto (which will be deduced to int as the return type of std::stoi.

  • As presented, the input data set has a blank line; your code will crash because the string::rfind() call will return string::npos if it can't find the search string. Add a check for that value:

    void processRecords( const std::string& record )
    {
         auto splitLoc= record.rfind(" ");
         if( splitLoc != std::string::npos )
         {
              // ... the rest of the function goes here...
         }
    }
    
  • Function readInputFile() takes its argument by value; change it to a reference-to-const to prevent copies being made of its argument:

    void readInputFile( const std::string& fileName )  // "string&", not "string"
    
  • You don't need to call std::ifstream::close() at the end of readInputFile(): the destructor will close the file for you as soon as infile goes out of scope when the function returns.

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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