9
\$\begingroup\$

For full disclosure: this is an assignment for my programming class and I just want tips or advice on some of the code.

Assignment details just for background information purposes:

Write a program that will read a number 1-100 from the user, and the name of a data file, and will tell the user what word is in the file and how many times the numbers shows up in the data file. Validate input number (keep asking until valid) and validate the file was successfully open.

text file contents: Darling 10 20 21 19 20


#include <iostream>
#include <fstream>
#include <string>
#include <cstdlib>  // Needed for exit()
using namespace std;

int main()
{
    ifstream inputFile;
    string fileName;
    int value;

    cout << "Enter the file name: ";
    getline(cin, fileName);

    // Open the file
    inputFile.open(fileName.c_str());

    // Check for successful opening
    if(inputFile.fail())
    {
       cerr << "Error Opening File" << endl;
       exit(1);
    }

    cout << "\nThe file has been successfully opened for reading.\n";

    cout << "Pick a number between 1 through 100. ";
    cin >> value;

    if (value >= 1 && value <= 100)
    {
     cout << value << " is in the acceptable range.\n";
    }
    else
    {
     cout << "Please enter a number from 1 to 100. ";
     cin >> value;
    }

    string word;
    int number;
    int count = 0;

    infile >> number;

    // Read a file until you've reached the end
    while (!inputFile.eof())
    {
        if (number == x)
        {
            sum1++
        }
        infile >> number;
    }
    cout << sum1;

    while (!inputFile.eof())
    {
       inputFile >> word;
        if (word == "input")
        {
            count++;
        }
    }

    // Close the file
    inputFile.close();
    return 0;
}

Any tips or advice would be greatly appreciated since I'm lost when it comes to the fstream portion of this assignment right now.

UPDATED

#include <iostream>
#include <fstream>
#include <string>
#include <cstdlib>  // Needed for exit()
using namespace std;

int main()
{
    ifstream inputFile;
    string fileName;
    int value, x;

    cout << "Enter the file name: ";
    getline(cin, fileName);

    // Open the file
    inputFile.open(fileName.c_str());

    // Check for successful opening
    if(inputFile.fail())
    {
       cerr << "Error Opening File" << endl;
       return(1);
    }

    cout << "Pick a number between 1 through 100. ";
    cin >> value;

    do
{
    cout << "Pick a number between 1 through 100. ";
    int value;
    cin >> value;

} while (value < 1 || value > 100);

    string word;
    int number;
    int count = 0;

    infile >> number;

    // Read a file until you've reached the end
    while (!inputFile.eof())
    {
        if (number == x)
        {
            sum1++
        }
        inputFile >> number;
    }
    cout << sum1;

    while (!inputFile.eof())
    {
       inputFile >> word;
        if (word == "input")
        {
            count++;
        }
    }

    // Close the file
    inputFile.close();
    return 0;
}
\$\endgroup\$
5
\$\begingroup\$
  • Despite what you may have been taught, you should not get into the habit of using using namespace std, especially when you start writing larger programs.

  • Since you're in main(), you should just return 1 instead of calling exit(1). The latter is only necessary in other places from which you will need to terminate early.

  • You don't need to display an output if the file was successfully opened. You'll know it failed if the program terminates after asking for a filename.

  • Do not use std::eof() for detecting the end of the file. It will look for the eof bit after reading the end of the stream (and will not even consider errors in reading (i.e. bad bit)). Read this for more information.

    Instead, read from the file and check for the bad bit using std::getline():

    while (std::getline(std::cin, line))
    {
        // ...
    }
    
  • You ask the user for input, but you only validate it once. If the user enters an invalid value a second time, the program will continue, causing problems. All of that should be in a loop that will terminate once the user provides valid input.

    do
    {
        std::cout << "Pick a number between 1 through 100. ";
        std::cin >> value;
    
    } while (value < 1 || value > 100);
    
  • You don't need return 0 at the end of main(). Reaching the end already implies a successful termination, so the compiler will do this return for you.

\$\endgroup\$
  • \$\begingroup\$ By changing the return 0 to return 1 in place of the exit() the loop would return a true value. Is that a correct conclusion from one of your bullet points? \$\endgroup\$ – jal3524 Apr 11 '14 at 19:21
  • \$\begingroup\$ @jal3524: I mean changing the exit(1) to return 1. The former is for cases where you must exit from a different function (such as a void function). Other than that, you don't have any loops that return at the end. \$\endgroup\$ – Jamal Apr 11 '14 at 19:27
  • 1
    \$\begingroup\$ do { std::cin >> value; } while (value < 1 || value > 100); can/will loop indefinitely if the user enters something other than a digit. \$\endgroup\$ – Jerry Coffin Apr 12 '14 at 3:33
  • \$\begingroup\$ @JerryCoffin: Yeah, I just gave a shorter example of this. I could've accounted for all input. I think Loki's answer is much better in regards to this anyway. \$\endgroup\$ – Jamal Apr 12 '14 at 3:36
  • 2
    \$\begingroup\$ @Jamal: You have chosen...wisely. \$\endgroup\$ – Jerry Coffin Apr 12 '14 at 3:38
4
\$\begingroup\$

Everything @Jamal said:

When reading user input prefer not mix getline() with operator>> as one reads and discards new line while the other does not and this can cause confusion as to where on the input stream you are.

Secondly user input (when done manually) is usually line based (ie the input buffer is not flushed until they hit return). So it is best to read a whole line at a time then validate it.

Remember scope:

int value;   // This is the value being used in the while condition.

// STUFF

do
{
    cout << "Pick a number between 1 through 100. ";

    int value;   // This is the value you are
                 // reading from user input and has nothing to 
                 // do with the variable value in the outer scope.

                 // It is usually a bad idea to shadow variables
                 // in the outer scope like this as it causes confusion.
                 // so turn your compiler warning up so that the compiler
                 // warns you about variable shadowing.

                 // Then tell your compiler to treat all warnings as
                 // errors (as all warnings are really logicall errors
                 // in your code).

    cin >> value;

  // This while
  // is testing the variable `value` that is defined in the outer
  // scope (which is not what you want).
} while (value < 1 || value > 100);

You must also check for invalid input.

int value;
std::cin >> value;

What happens if I type 'fred<enter>'?
Personally I have forgotten if the variable is even changed (its in the standard somewhere). But the stream has its bad bit set. This means any further attempt to read from the stream will result in nothing happening so you can't even do it in a loop.

int value = 0; // make sure it has an initial value so
               // the while() test is valid even if the
               // user enters junk.
do
{
    std::cout << "Enter a value\n";
    std::cin >> value;
}
while (value < 1 || value > 100);

If I type 'fred' then this will enter an infinite loop (even if the user enters 10 next time around). Because the bad bit has been set and nothing will be read from the stream while this bit is set (its set because you are trying to read an int and you got fred (which is not an int)).

Don't declare all your variables at the top of the function.

ifstream inputFile;
string fileName;
int value, x;

Declare them as close to the point where you use them as possible (it makes reading the code easier). Also in C++ we sometimes (quite often) use the side affects of the constructor/destructor to do work. So you don't want those firing until you need them to fire.

std::cout << "What is the file name?\n"; // Don't forget the '\n'
std::string fileName;
std::getline(std::cin, fileName);

When testing a stream state best to use bad rather than any of the other states (as this catches all failures). Also note that when a stream is used in a boolean context (ie in an if statements condition) it converts itself into a value compatible with the context using the bad() method as a test.

std::ifstream file(fileName.c_str());
if (!file)              // don't need to call any functions.
{                      // This calls `bad()` and converts
                       // the result into a value useful for a test
                       // true if it is open and readable or false otherwise
                       // so !file is only true if you can read the file.
     return 1;
}

The same apples for eof() as it does fail(). Don't use it. Prefer bad().

while (!inputFile.eof())
// This will result in an infinite loop if the bad bit is set.

Also note that the stream operator operator>> returns a reference to the stream. Which (as stated above) when used in a boolean context will convert itself to value that is useful using bad()

 while(file >> value)
 {
     // Loop is entered every time the read works.
     // This is because `file >> value` returns a reference to a stream.
     // When a stream is used in a boolean context like while(<stream>)
     // weill test itself using `bad()`
 }

Conclusion:

// Always use std::getline to read user input.
// Always do the read and test in the same condition.
// Always check for bad bit.

std::string line;
int         value;
while(std::getline(std::cin, line))
{
    // Successfully read a line.
    // If we did not enter the loop then we reached eof.

    // So now we want to check what the user entered was a number.
    // So convert the user input line into a stream and read a number
    // from it (or try).
    std::stringstream linestream(line);
    if (linestream  >> value)
    {
         // You have successfully read a user value.
         // This means the read was good.
         // If there was text on the input then this would have failed.
         if (value >=1 && value <= 100)
         {
             break;  // exit the loop
         }
    }

    // If we reach here the user either entered invalid data
    // Which caused the the `bad` bit in `linestream` to be set
    // or the value was out of range.
    // Either way we just tell him to retry.

    // Note: because we create a new linestream each attempt
    //       this effectively resets the `bad` bit. If you want
    //       mess around with the bit flags in the stream that 
    //       is fine but I usually find this easier.

    std::cout << "Hey... Whats up dumb ass you can't read? Number between 1 and 100. Try again:\n"

    // The outer while loop will the try to get another value.

}

if (std::cin.eof())
{
   // We failed to get a get a good value.
   // The loop above exited because std::getline() failed
   // This happens when we reach eof thus the value has
   // not been set correctly.


   return 1; // or exit(1) if appropriate.
}

std::cout << "The value was good: " << value << "\n";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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