Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
    
Just a suggestion - might want to put a link to Euler Problem #22 in your question. Anyone can google it, but why not make it as easy as possible for those helping you? –  Dan Mar 27 at 17:06
    
@Dan Thanks Dan, that's a great suggestion. From next question I will follow it. –  pranitkothari Mar 27 at 17:11
add comment

2 Answers 2

up vote 10 down vote accepted

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).

share|improve this answer
    
+1. Really good advice. –  pranitkothari Mar 26 at 8:14
1  
You forgot to use the path argument in read_file. :) –  João Portela Mar 26 at 11:37
    
@JoãoPortela Whoops, good catch. Fixed. –  Yuushi Mar 26 at 11:48
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.