The code inputs an int, n, which is the number of lines to follow. Then, on each line, the first number goes to array A, the second to B, and the third to C. I then pass these 4 arguments to sub which I have no control over (in an object file where I don't know the implementation). All I know is that it returns an int.

What I do then is write n, the three arrays, and the result of sub into an output file. Pretty simple code.

This code works as expected, but I just want to see if there are any conventions I should be following (especially C++11) and ideas to consider for efficiency.

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <sstream>
#include "sub.h"

int main()
    std::ifstream file;

    if (!file) {
        std::cerr << "Error opening file." << std::endl;
        return EXIT_FAILURE;
    int n;
    std::string firstLine;
    getline(file, firstLine);
    std::stringstream(firstLine) >> n;
    int a,b,c;
    std::vector<int> A1,B1,C1;
    while (file >> a >> b >> c) {
    std::ofstream output;

    output << n << "\n";
    size_t size = A1.size();
    for (size_t i=0; i<size; ++i) {
        output << A1[i] << " " << B1[i] << " " << C1[i] << std::endl;
    //call sub method with pointer to beginning of vectors
    //I have no control over sub method, its signature is (int, int*, int*, int*)
    int n1 = sub(n, &A1[0], &B1[0], &C1[0]);
    //output result and close file
    output << n1;

Example input:

0 1 2 <- This is A[0], B[0], C[0]

2 3 4 <- This is A[1], B[1], C[1]

4 5 6 <- This is A[2], B[2], C[2]

The range-based for-loop would not work in this case. It would, however, if I printed all of A, B, or C on one line.

  • It's a good idea to order your STL #includes either alphabetically or by groups (see this answer for more details). Header files should also precede STL #includes to avoid dependencies.

  • I'd give your IO files more accurate names (such as inFile and outFile).

  • Include <cstdlib> for EXIT_FAILURE.

  • This is C++ and not C, so use std::size_t instead of size_t.

  • Put the std::vector declarations (A1, B1, C1) on separate lines.

  • You don't have to create another size variable for A1.size() just for a loop.

    In that same loop: you're looping through all three arrays at once, but are using one vector's size to do that. That's a bad idea. If they happen to be different sizes, you're going to run into problems.

  • This:

    int n1 = sub(n, &A1[0], &B1[0], &C1[0]);

    is best written as this (with iterators):

    int n1 = sub(n, A1.begin(), B1.begin(), C1.begin());
  • \$\begingroup\$ Thanks for your comments. The reason I cannot do the range-based for loop is that I need to print in the same order as I input. I'll edit my question with an example. \$\endgroup\$ – user473973 Aug 28 '13 at 15:25
  • \$\begingroup\$ Oh, okay. Again, just make sure you're always keeping alert of going out of bounds in any situation. \$\endgroup\$ – Jamal Aug 28 '13 at 15:27
  • \$\begingroup\$ Thanks. I originally had A.at(i) because it had bounds checking. However, I decided against it due to a suggestion I've seen before about how operator[]() is faster than .at(). \$\endgroup\$ – user473973 Aug 28 '13 at 15:28
  • \$\begingroup\$ Right. at() is best if you may go out of bounds and/or if you need to throw exceptions. \$\endgroup\$ – Jamal Aug 28 '13 at 15:30
  • \$\begingroup\$ Anyway, thank you again for all of your comments! \$\endgroup\$ – user473973 Aug 28 '13 at 15:31

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.