Mth to last element

Question

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 ] );
}

Answer 1

• 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.