Produce the sum of all white space separated integers in a given file

Question

How's my code? Works perfectly for what I'm trying to do :)

#include<iostream>
#include<string>
#include<vector>
#include<algorithm>
#include<cmath>
#include<fstream>
#include<assert.h>
using namespace std;
inline void keep_window_open() { char ch; cin >> ch; }



int main() {
    int sum{ 0 }, number;
    vector<int> numbers;
    string word;
    fstream file;
    file.open("file1.txt", fstream::in);

    while (true) {
        file >> number;
        if (file.eof()) {
            numbers.push_back(number);
            break;
        }
        else if (file.fail()) { 
            file.clear();
            file >> word;
        }
        else if (file.bad()) exit(1);
        else numbers.push_back(number);
    }

    for (int x : numbers) sum += x;

    cout << sum << "\n";


}

Here's the file I'm reading from:

words 32 jd: 5 alkj 3 12 fjkl: 23 / 32
hey 332 2 jk 23 k 23 ksdl 3
32 kjd 3 klj 332 c 32

Answer 1

You collect all numbers but then use the collection only for the sum. Better just update the sum. And I find the whole trying int and if fails use word a bit complicated. How about just always reading words and then reading numbers from them? (The >> writes zero if there's no number.)

#include <fstream>
#include <iostream>
#include <sstream>
#include <string>
using namespace std;

int main() {
    int sum = 0;
    ifstream file("file1.txt");
    string word;
    while (file >> word) {
        int number;
        istringstream(word) >> number;
        sum += number;
    }
    cout << sum << endl;
}

Answer 2

Here's an answer with less complex while loop and no use of stringstreams.

I've found std::stoi/std::stof faster than >> operator to convert strings to numbers. Also, we have a clear indication of failure in the form of exceptions.

---

If you only need the sum, don't store all numbers. If the file is big, it's just memory waste.

---

#include<cmath>
#include<assert.h>

Unused headers.

• https://github.com/include-what-you-use/include-what-you-use

---

#include <fstream>
#include <iostream>
#include <string>

int main() {
    int sum = 0;
    std::ifstream file_s("file1.txt");
    if (file_s.bad()) {
      return -1;
    }

    std::string word;
    while (std::getline(file_s, word, ' ')) {
        try {
           sum += std::stoi(word);
        }
        catch(const std::invalid_argument& ex) {
            std::cout << ex.what() << ":" << word << std::endl;
        }
    }

    std::cout << sum << std::endl;
    return 0;
}

Answer 3

#include<iostream>
#include<string>
#include<vector>
#include<algorithm>
#include<cmath>
#include<fstream>
#include<assert.h>

This would be easier to read with some spaces between #include and the header name.

<assert.h> is a C standard library header file. C++ supplies its own version of the C headers which put their content into the std namespace, and we should use that version instead. These headers have c added to the front, and no extension, e.g. #include <cassert>.

It's nice to arrange includes in alphabetical order.

Note also that we don't need all these headers - include only what you need.

---

using namespace std;

This is a bad habit to get into. It can lead to name collisions for large projects. It's best to avoid it and type the namespace where you need to (e.g. std::vector).

---

inline void keep_window_open() { char ch; cin >> ch; }

This function isn't used! If you're using visual studio (or the microsoft compilers in general), you can set the linker subsystem to CONSOLE, which keeps the console window open after the program finishes running.

---

int sum{ 0 }, number;
vector<int> numbers;
string word;

Variables should be declared as close to the point of use as possible:

• We don't need sum until much later.
• number can be declared inside the loop.
• numbers is only needed after we've opened the file (just before the loop).
• word isn't needed until an inner scope of the loop.

Variables should not be reused (unless there is a significant performance hit - e.g. allocating large objects in memory), as it makes it harder to spot mistakes.

---

fstream file;
file.open("file1.txt", fstream::in);

We can use the fstream constructor to open the file, instead of opening the file in a separate step.

We could use ifstream, since we're only interested in file input.

---

while (true) {
    file >> number;
    if (file.eof()) {
        numbers.push_back(number);
        break;
    }
    else if (file.fail()) { 
        file.clear();
        file >> word;
    }
    else if (file.bad()) exit(1);
    else numbers.push_back(number);
}

There's nothing wrong with while (true). It's often much clearer than trying to cram lots of logic into the loop condition.

There's also nothing wrong with multiple exit points, or return statements. We should return (or break or continue) as soon as possible in our code to simplify the logic and avoid unnecessary branching and nesting.

There are a couple of things we can simplify here:

• We should only have one point where we add a number to the vector (duplicate code is harder to maintain).
• If we exit the loop with break or return (or start a new loop iteration with continue), we don't need to use an else if because the else is implicit. We can just use a separate if statement.

There's also a subtle bug here. When the bad bit on a stream is set, the fail() function also returns true! So instead of exit()ing, we'll try to clear the (unrecoverable) error, the bad bit will be set again, and we end up in an infinite loop. So we need to check bad() before we check fail().

---

It would be nice to print some error messages to std::cerr if something goes wrong (the file doesn't open, or reading from it fails).

It would be nice to have a comment or two where the purpose of the code isn't immediately clear (e.g. what word is used for).

---

Applying this we might end up with the following:

#include <cstdlib>
#include <fstream>
#include <iostream>
#include <string>
#include <vector>

int main() {
    
    std::string filename = "file1.txt";
    std::ifstream file(filename);
    
    if (!file.is_open()) {
        std::cerr << "failed to open file: " << filename << "\n";
        return EXIT_FAILURE;
    }

    std::vector<int> numbers;
    
    while (true) {
        
        int number;
        file >> number;
        
        if (file.bad()) {
            std::cerr << "unrecoverable error while reading from file: " << filename << "\n";
            return EXIT_FAILURE;
        }
        
        if (file.fail()) {
            // input was not an integer! 
            // clear the error, and discard input until the next whitespace
            file.clear();
            std::string word;
            file >> word;
            continue;
        }
        
        numbers.push_back(number);
        
        if (file.eof()) {
            break;
        }
    }

    int sum = 0;
    for (int x : numbers) sum += x;
    
    std::cout << sum << "\n";
}