Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
  • 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 adviceDaniel 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.

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

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

Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19

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