Reading numbers from three text files in C++

Question

This code is supposed to read some text files. What should I do in order to optimize my code. I was confused about writing a user defined function so that I can pass any text to it and it stores it in vectors. I have different types of vectors as you see below.

#include "stdafx.h"
#include <iostream>
#include <vector>
#include <fstream>
using namespace std;

int main()
{
    unsigned short int num1;
    float num2, num3;
    vector<unsigned short int>index;
    vector<float>lenght;
    vector<float>coordinate;
    ifstream InFile1;
    ifstream InFile2;
    ifstream InFile3;
    InFile1.open("E:\\Programming Languages\\HelloC++\\Geodetic\\Index.txt"); 
    InFile2.open("E:\\Programming Languages\\HelloC++\\Geodetic\\Lenght.txt");
    InFile3.open("E:\\Programming Languages\\HelloC++\\Geodetic\\Coordinate.txt");
    if (!InFile1 || !InFile2 || !InFile3){
        cerr << "Sorry Couldn't Open The File\n.";
    }
    while (InFile1 >> num1) {
        index.push_back(num1);
    }
    while (InFile2 >> num2) {
        lenght.push_back(num2);
    }
    while (InFile3 >> num3){
        coordinate.push_back(num3);
    }
    for (size_t i = 0; i < coordinate.size(); i++) {
        cout << coordinate[i]<<endl;
    }
    return 0;
}

Answer 1

I'm going to be cheeky here and make a recommendation that's currently beyond what I suspect your current C++ level is, because I feel like this is a great problem to introduce templates:

Consider splitting the file reading in its own function, so that you don't have to reimplement the file loading logic three times. In this case, this will necessarily be a function template, since you can load different type of values.

Here's what it would look like:

template<typename T>
std::vector<T> read_num_file(const char* file_name) {
  // open ifstream and error check here...

  std::vector<T> result;
  T val;
  while(InFile >> val) {
    result.push_back(val);
  }
  return result;
}

int main() {
  std::vector<unsigned short> index = read_num_file<unsigned short>("Index.txt");

  ...
}

Answer 2

I agree with what @Frank said.
But rather than use your own method I would use a one that is already implemented by the standard library.

An easy way to copy a file into a vector is:

ifstream              inFile3("E:\\Programming Languages\\HelloC++\\Geodetic\\Coordinate.txt");
std::vector<float>    coordinate(std::istream_iterator<float>(inFile3),
                                 std::istream_iterator<float>());

Other things to think about:

Stop doing this:

using namespace std;

Yes all books use this. But it is bad practice in general. They have a good excuse in a book that they only have so much horizontal space to keep things readable.

In reality using it is dangerous and should be avoided (even for short programs, as doing it here becomes a bad habit and bad habits have a tendency to accidentally intruding on production code).

See:: Why is “using namespace std” considered bad practice?

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

    unsigned short int num1;
    float num2, num3;
    vector<unsigned short int>index;
    vector<float>lenght;
    vector<float>coordinate;
    ifstream InFile1;
    ifstream InFile2;
    ifstream InFile3;

Declare variables as you need them at the point where you are about to use them. Note a lot of these object have constructors that run code. You don't want to run code for an object that is never used. So only declare them just before you use them (and then use a good constructor).

Also by declaring them close to the point of usage it is easy to quickly see there types (without having to scroll to the top of the function).

Be specific with error messages.

    if (!InFile1 || !InFile2 || !InFile3){
        cerr << "Sorry Couldn't Open The File\n.";
    }

Which file failed to open?

Learn to use standard algorithms and iterators

    for (size_t i = 0; i < coordinate.size(); i++) {
        cout << coordinate[i]<<endl;
    }

In C++ we normally use iterators to loop over a container:

    for (std::vector<float>::const_iterator loop = coordinate.begin(); loop != coordinate.end(); ++i) {
        std::cout << *loop << "\n";
    }

In C++11 we added auto to remove long type declarations.

    for (auto loop = coordinate.begin(); loop != coordinate.end(); ++i) {
        std::cout << *loop << "\n";
    }

In C++11 we also added lambda's to make std::for_each() easy to use:

    std::for_each(coordinate.begin(), coordinate.end(),
                  [](auto const& item){std::cout << item << "\n";});

In C++14 we added range based for loop:

    for (auto const& item: coordinate.begin()) {
        std::cout << item << "\n";
    }

Prefer to use prefix increment

    for (size_t i = 0; i < coordinate.size(); i++)
                                              ^^^

Usually it does not matter. But sometimes it does. If you get in the habbit of using prefix increment then you always use the optimal version of increment.

Prefer '\n' over std::endl

Don't use return 0 in main when it can never error.

    return 0;