Project Euler #22 - Name scores

Question

I have written code in C++ 11 and check output with Project Euler site, and it is correct. I am not showing output, just to keep it secret, at least from my end.

Please review my C++ 11 code.

#include <fstream>
#include <string>
#include <algorithm>
#include <iostream>
#include <vector>
#include <sstream>

using namespace std;


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

    ifstream file("E:\\names.txt");

    if (!file.is_open())
    {
        cout << "Unable to open file";
        return -1;
    }

    /* Get all file data with ',' delimiter*/
    vector<string> words;
    string token;

    while (getline(file, token, ','))
    {
        words.push_back(token);
    }

    //sort all the words 
    sort(words.begin(), words.end());

    int counter = 0;
    unsigned int grand_total = 0;

    //get total of each word and multiply with it's position
    for (auto it = words.begin(); it != words.end(); it++)
    {
        counter++;
        //copy word from token excluding " "
        string word((*it).begin() + 1, (*it).end() - 1);

        int sub_total = 0;
        for (string::iterator it = word.begin(); it != word.end(); it++)
        {
            //ascii value of A is 65
            //A is 1, B is 2, so *it - 64
            sub_total += *it - 64;
        }

        //grand total will be final answer
        grand_total += sub_total*counter; 
    }

    return 0;
}

Answer 1

The code is generally pretty clean, however, the major sticking point is that everything here is just shoved into main. Just breaking this up with a few functions that do one thing would be a good start:

std::vector<std::string> read_file(const std::string& path)
{
    std::ifstream file(path);

    if (!file.is_open())
    {
        std::cerr << "Unable to open file" << "\n";
        std::exit(-1);
    }

    std::vector<string> words;
    std::string token;

    while (std::getline(file, token, ','))
    {
        words.push_back(token);
    }

    return words;
}

I've also put back all the std:: namespace qualifier. Using namespace std is something that is ok for very short programs like this, but is something you shouldn't get into the habit of doing.

How about a function to sum the values in a word?

int sum_word(const std::string& word)
{
    constexpr static int ascii_to_integer = 64;
    int total = 0;
    for(auto c : word) {
        total += (c - ascii_to_integer);
    }
    return total;
}

In fact, if you want to be fancy, this can be even shorter:

int sum_word(const std::string& word)
{
    constexpr static int ascii_to_integer = 64;
    return std::accumulate(word.begin(), word.end(), 0, [](int i) { return i - ascii_to_integer; });
}

Finally, we can have a function to encompass the outer loop (processing each word in the vector):

int total_word_position(const std::vector<std::string>& v)
{
    int counter = 1;
    int total = 0;

    for(const std::string& s : v) {
        // Exclude " "
        std::string word(s.begin() + 1, s.end() - 1);
        total += (sum_word(word) * counter);
        ++counter;
    }
    return total;
}

Your main method would then become:

int main()
{
    std::vector<std::string> all_words = read_file("E:\\names.txt");
    std::sort(all_words.begin(), all_words.end());
    int grand_total = total_word_position(all_words);
}

This has a number of benefits:

• Variable scope is minimized to a single smaller function, instead of a large main function.

• It's generally easier to read and debug, as there is less to keep in your head.

• It separates parts of the program that do very different things (reading a file vs sorting vs summing numbers).

Answer 2

Yuushi's advice is pretty good. Here's what I have to add:

ifstream file("E:\\names.txt");

Would be nice if the path wasn't hardcoded. You could pass in the input through stdin or open a file name passed in argv[1].

if (!file.is_open())

I believe if (!file) is a better way to do this since it checks if error bits are set.