just made!

Felt kinda happy bout it so i thought that i would post


#include <cstdlib>

#include <iostream>

#include <fstream>


using namespace std;

void convert_to_upper(char *file_input);


int main(int argc, char *argv[])

{

    char filename[81];

    int i, c, j;

    char input_line[81];

    char j_FTW[81];

    cout << "Enter file name and press enter: ";

    cin.getline(filename, 80);


    

    ifstream file_in(filename);

    

    if (!file_in) {

              cout << "file error...\n";

              system("pause");

              return -1;

    }

    while (1){

          cout << "enter lines of text to read: ";

          cin.getline(j_FTW, 80);

          j = atoi(j_FTW);

          for (i = 0; i <= j  && ! file_in.eof(); i++){

              file_in.getline(input_line, 80);

              convert_to_upper(input_line);

              cout << input_line << endl;

          }

          if (file_in.eof())

             break;

          cout << "more? (press 'Q' and enter to quit.)";

          cin.getline(input_line, 80);

          c = input_line[0];

          if (c == 'Q' || c == 'q')

             break;

          }

    

    

    

    system("PAUSE");

    return EXIT_SUCCESS;

}

void convert_to_upper(char *file_input){

     int m;

for ( m = 1; m < 81; m++){

file_input[m] = toupper(file_input[m]);

}

}

>Felt kinda happy bout it so i thought that i would post
Since you posted it, I can make you considerably less happy by ripping it to shreds. Nothing personal, just odd things I noticed and ways to improve your code. :)

>int main(int argc, char *argv[])
You don't use argc or argv, so they just clutter up your code and make it more confusing.

>char filename[81];
I'm curious where you got 81. Was it just an arbitrary number? If you want a buffer size, I'd recommend BUFSIZ from cstdio. I'm also curious why you use C-style strings instead of C++ string objects. They're much easier to use and cleaner.

>char j_FTW[81];
j for the win? :p I have no idea what this variable is supposed to mean, but it's clearly used to read an integer value for determining how many lines to process from the file. A better name would be...well, better. ;)

>cin.getline(filename, 80);
You're wasting the extra thought that you put into adding an extra space for the null character to the array. getline reads n - 1 characters and stores a null character in that last place. This is a good thing because you can avoid magic numbers entirely and just use sizeof with the array:


cin.getline ( filename, sizeof filename );

>system("pause");
I hate this. system is so tempting to abuse, and so many people do. The problem is that this program will be run from the command line, but what if it's automated? You've forced users to interact with the program when they could otherwise just redirect stdin to a file containing instructions for the program to use. It's also non-portable because you limit yourself to shells that support a pause program. It's also unsafe because I could write a malicious program, call it pause, give it priority higher than the shell's pause, and you're helping me do bad things.

How do you fix it? Write your own pause function that doesn't need to defer to the system. Since all you need is something to block the program while you read the messages, you can use any input method in C++ to cause a blocking read. The only problem is that now instead of pressing any key, you have to press enter because input in C++ is buffered and line oriented.

>return -1;
Why return -1 when EXIT_FAILURE is already in cstdlib? That seems inconsistent with how you use EXIT_SUCCESS, but I like symmetry in my returns.

>while (1){
This is an annoyance with some compilers, but if you like clean compiles, you're SOL because they warn that the condition is a constant value. You can avoid that by using a for instead:


for ( ; ; ) {

>j = atoi(j_FTW);
atoi is EVIL! Seriously, never use atoi unless you're absolutely, positively, without a doubt, sure that the string contains a valid number within the range of int. Why? Because if the string can't be represented as an integer, the behavior is undefined and there's no way you can avoid it. If the string doesn't represent an integer at all and atoi fails, you get 0 as if it succeeded. atoi is evil. Use strtol instead. At least then you can verify that everything worked and do error handling.

>for (i = 0; i <= j && ! file_in.eof(); i++){
There are two problems with this, both involve an off-by-one error. The first is that you're using the range [0, j] rather than [0, j). That's an inclusive range instead of an exclusive range. If I tell the program to process 1 line, it'll give me 2.

The second problem is using eof as a condition. The problem here is that the eofbit is only set after you try to read from the file and hit end-of-file. That means that under some circumstances the last line will be processed twice. You can avoid that problem by using the result of the input routine as your condition:


for ( i = 0; i < j && file_in.getline ( input_line, sizeof input_line ); i++ ) {

Or handle that condition inside of your loop:


for ( i = 0; i < j; i++ ) {

  if ( !file_in.getline ( input_line, sizeof input_line ) )

    break;

  //...

}

>system("PAUSE");
What's worse is you're not even consistent with your casing. :eek:

>void convert_to_upper(char *file_input){
Okay, problem #1 is that your bracing with this function doesn't match main, that's inconsistent formatting and you should avoid it like the plague. Second, and this is my personal preference, you should return the result string even if the modifications are mirrored back in main. That way you can use the result as an argument to another function.

>for ( m = 1; m < 81; m++){
m is a goofy name. People expect to see i, or sometimes x, but m (and x) suggests a multidimensional array. That could be confusing. Also, you use code omniscience in this function. How do you know what the size of the string is? Worse yet, you're probably walking off the end of the string into uninitialized parts of the array. Take a lesson from strcpy and stop when you find a '\0' character.


for ( int m = 1; file_input[m] != '\0'; m++ ) {

>file_input[m] = toupper(file_input[m]);
As a side note, unless you can guarantee that the character came from a standard input function, you can't be sure that it's in the expected range of toupper. So you should cast the character to unsigned char to force validity.

Lastly, you should, as a good practice, limit the scope of your variables to the blocks that they're used in and always check your input for errors.

Here are my changes (without getting into the C++ library, which would greatly simplify the entire program):


#include <cstdio>

#include <cstdlib>

#include <fstream>

#include <iostream>

#include <ios>

#include <limits>


using namespace std;


char *convert_to_upper ( char *file_input );

void fatal_error ( const char *msg, bool is_dirty );

void my_pause ( bool is_dirty );


int main()

{

  char filename[BUFSIZ];

  bool done = false;

 

  cout<<"Enter file name and press enter: ";

  

  if ( !cin.getline ( filename, sizeof filename ) ) {

    fatal_error ( "Input error...", false );

  }


  ifstream file_in ( filename );


  if ( !file_in ) {

    fatal_error ( "File open error...", false );

  }

  

  while ( !done ) {

    char input_line[BUFSIZ];

    int n;


    cout<<"Number of lines to process: ";

    

    for ( ; ; ) {

      char *end;


      if ( !cin.getline ( input_line, sizeof input_line ) ) {

        fatal_error ( "Input error...", false );

      }

      

      // This is minimal security for strtol

      // Ideally we would also check for range problems

      n = (int)strtol ( input_line, &end, 0 );


      // Assume success if end moved

      if ( end == input_line ) {

        cerr<<"Invalid input, please enter a number: ";

      } else {

        break;

      }

    }

    

    for ( int i = 0; i < n; i++ ) {

      if ( !file_in.getline ( input_line, sizeof input_line ) ) {

        break;

      }


      cout<< convert_to_upper ( input_line ) <<'\n';

    }


    // end-of-file isn't a fatal error here

    if ( file_in.fail() || file_in.bad() ) {

      fatal_error ( "Input error...", false );

    } else if ( file_in.eof() ) {

      // But it is a terminating condition

      done = true;

    } else {

      cout<<"More? (press 'Q' and enter to quit): ";


      if ( !cin.getline ( input_line, sizeof input_line ) ) {

        fatal_error ( "Input error...", false );

      }


      if ( toupper ( input_line[0] ) == 'Q' ) {

        done = true;

      }

    }

  }


  my_pause ( false );


  return EXIT_SUCCESS;

}


char *convert_to_upper ( char *file_input )

{

  for ( int i = 1; file_input[i] != '\0'; i++ ) {

    file_input[i] = (char)toupper ( (unsigned char)file_input[i] );

  }


  return file_input;

}


void fatal_error ( const char *msg, bool is_dirty )

{

  cerr<< msg <<'\n';

  my_pause ( is_dirty );

  exit ( EXIT_FAILURE );

}


void my_pause ( bool is_dirty )

{

  // Make sure we can actually block

  cin.clear();


  if ( is_dirty ) {

    // Clear out any extraneous characters

    cin.ignore ( numeric_limits<streamsize>::max(), '\n' );

  }


  // Make sure the prompt isn't redirected with stdin

  cerr<<"Press [Enter] to continue . . .";


  // Do the block

  cin.get();

}