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 just implemented code to add two ints together as strings and I'm sure my method is inferior. I've looked up other methods too but I'd appreciate criticisms specific to my code. I really want to make sure I don't develop bad habits in C++ and I think leveraging the knowledge of brighter individuals than myself on this site will help with that :). Am I relying too much on iterators btw?

Anyhow, the code is below and be as harsh as you want. Thanks!

//This program intends to read very large integers from a text file
//the format is two integers per line. It will then add them together and output
//the sums one each line

//first, succesfully read from the text file and store into two vectors
//each digit being an element
#include<iostream>
#include<fstream>
#include<string>
#include<vector>
using namespace std;

int main()
{

    string line,s1,s2;
    vector<string> vec1, vec2; //string 1 and string 2 store each of the 2 numbers per line as individual digits
    vector<string> final;      //create vector of strings that holds the sum of each pair of strings per line
    ifstream myfile("bignum.txt"); //this opens the file bignum.txt
    if (myfile.is_open())
    {
       int  count=0; //count lines
       while (!myfile.eof())
       {
           getline(myfile,line);
           bool flag=true; //bool to see if integer 2 should be stored now
           for (int i=0; i<line.length(); i++)
           {
               if (line[i]==' ')
                   flag=false;
               else
               {
                   if (flag)
                       s1+=line[i];
                   else
                       s2+=line[i];
               }


                //by the end of this loop we have two strings representing each of the numbers
           }

           //will now append 0's to the front of the smaller string in order to make the strings the same length
          if (s1.length()<s2.length())
              s1=string(s2.length()-s1.length(),'0')+s1;
          else
              s2=string(s1.length()-s2.length(),'0')+s2;

           //create a variable that acts as the surplus/carry for each single-digit addition
           int carry=0;           

           //create iterators allowing for iteration over a vector
           string::reverse_iterator it1 = s1.rbegin(),it2=s2.rbegin();
           string sum_string="";

           for (; it1!=s1.rend(); it1++, it2++)
           {
               //convert chars to ints
               int k1=*it1-'0';
               int k2=*it2-'0';

               //intermediate sum
               int sum_inter=k1+k2+carry;
               carry=0;

               sum_string.insert(0,to_string(sum_inter%10)); 
               carry+=int((sum_inter-sum_inter%10)/10);

           };

           //insert the last carry into the highest digit position
           sum_string.insert(0,to_string(carry));

           //go through sum_string and omit leading 0's
           bool lead_zero=true;
           string::iterator leadit=sum_string.begin();
           while(lead_zero)
           {
               if (int(*leadit-'0')==0)
                  sum_string.erase(0,1);
               else
                 lead_zero=false;
           }

           final.push_back(sum_string);

           //clear strings 1 and 2 for reuse

           s1.clear();
           s2.clear();
       }

        vector<string>::iterator i1=final.begin();
        int count3=0;
        for (; i1!=final.end(); i1++)
        {
            count3++;
            cout<<"The sum of the numbers on line "<<count3<<" is "<<*i1<<"\n";
        }

    }
    else cout <<"file could not be opened";
    return 0;
}
share|improve this question
    
Surely C++ has a real big-int library... –  Daniel Wagner May 15 at 18:23
    
It's a good excercise nonetheless –  Borat.sagdiyev May 15 at 18:24

3 Answers 3

Some basic C++ advice:

  • Don't use using namespace std;. It leads to namespace pollution: it you use another library, you may import names in the global namespace that may clash with those in the std namespace. Anyway, adding std:: in front of the standard library components does not hinder readability.

  • return 0; at the end of main is useless. If the program reaches the end of main without having encountered any return statement, it will automagically return 0.

  • You compare signed and unsigned integer values in this line:

    for (int i=0; i<line.length(); i++)
    

    That means that you didn't turn on the right compiler flags (-Wall, -pedantic and -Wextra flags that you should almost always use) or that you ignored the warnings. Try never to ignore warnings, they help to write good code. To fix this warning, simply turn int i=0 into std::size_t i=0. Ideally, you should even write typename std::string::size_type i=0 but people generally agree that it is too long, and the main standard library implementations use std::size_t anyway for the length of a string.

    The rationale for this warning is that in some cases, the comparison will happen, but small negative integers may be compared to big unsigned integers. On theo ther hand, if you compare unsigned integers with different sizes, you will never have such a problem.

  • It's really no more than a tidbit, but post-incrementation (++i) may be faster than pre-incrementation (i++). We all know that premature optimization is the root of all evils, but this one is simple enough to write as you go. In other words, you should remember this useful guideline:

    Use ++i if you don't have a specific reason to use i++.

  • The variable count is never used, you can remove it. You compiler should have told you that it was unused.

  • Try to organize your headers in alphabetical order:

    #include<fstream>
    #include<iotream>
    #include<string>
    #include<vector>
    

    It will help you when you want to check whether a header is already included or not.

  • If you want to check whether an ifstream was successfully openen, don't use is_open, use operator!:

    ifstream myfile("bignum.txt"); //this opens the file bignum.txt
    if (not myfile)
    {
        // An error occured
        // handle the error
    }
    // else use myfile
    
  • Also, this it not the idiomatic way to read a file line by line:

    while (!myfile.eof())
    {
        getline(myfile,line);
        // ...
    }
    

    Here is how you should do it, std::getline has been designed to be used in such a loop:

    while (getline(myfile, line))
    {
        // ...
    }
    

    Actually, using eof can read past end of file which makes it unsafe to read a file line by line until the end of the file is reached.

share|improve this answer
    
Thanks. Well aware of using namespace and return 0. I will most certainly use the scope operator to specify namespaces when I start using one's other than std. However, it's time consuming as I'm constantly making small programs now and that's not a habit I'm worried about being stuck on. return 0 is a stylistic thing and I think its useful to specify for readability, but its trivial nonetheless. I appreciate points 3 and 5 though. I was mostly hoping for analysis of the algorithm I used and whether I could have done it in a simpler way as well. Is that a valid purpose for Codereview? –  Borat.sagdiyev May 15 at 13:25
    
@Borat.sagdiyev It is a valid purpose, but I am not currently in a good mindset to make this kind of "semantic review". You could say that I only did a "no-brainer good practice review", but I am pretty sure that somebody will come here and do a proper algorithm review too :) –  Morwenn May 15 at 13:27
    
Thanks for the points though Morwenn! –  Borat.sagdiyev May 15 at 13:28
    
could you explain why you shouldn't use is_open? –  Borat.sagdiyev May 15 at 13:29
    
And could you perhaps direct me to somewhere where I could read about compiler flags (or explain the important ones to me). A simple google search is proving fruitless –  Borat.sagdiyev May 15 at 13:31

I have found a couple of things that could help you improve your code. In no particular order, here they are:

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Eliminate unused variables

This code declares a variable count to count lines (according to the comment) but then does nothing with it. Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

Avoid comparing signed to unsigned numbers

Your code contains the line

for (int i=0; i<line.length(); i++)

but line.length() returns an unsigned value and not an int. This is easily diagnosed by the compiler, and easily fixed by simply declaring i to be unsigned instead of int.

Avoid using language keywords as identifiers

It isn't technically wrong, and final isn't even technically a keyword in C++, (it's an identifier) but it has a special meaning in the context of class member functions in C++11 and is probably best avoided as an identifier.

Think of the user

Rather than having the file be a fixed name, why not pass it in as a command line argument? Or better still, have it read from std::cin so that the output of some other program could pipe its output to this one. Also, what happens if more than one number is on a line of input? What happens if there are invalid characters on the line?

Make the program modular

Right now everything happens in main, but it really does three different things: it reads in numbers, it adds the numbers and then it emits the sums. Since the bulk of the operations in your program involve the addition of the numbers, it suggests that that should be broken out into a separate function. Alternatively, it might make sense to create a class of string-based numbers and provide an operator+ for them, greatly simplifying the code for the reader.

Use inserters rather than getline

Your program doesn't really need to count input lines, so inputting your strings could be made much simpler:

myfile >> s1 >> s2;

Handle an error near where the problem is detected

Your program contains this construct:

ifstream myfile("bignum.txt");
if (myfile.is_open()) 
{
    // 82 lines of code later...
}
else cout <<"file could not be opened";
return 0;

First, the test could be written more concisely:

if (myfile) { ... }

Second, if you're testing for an "early bailout" it would make the code more clear if you put that error handling close to the test as in:

if (!myfile) {
    cout << "file could not be opened\n";
    return 1;
}
// all of the file handling goes here

Don't use myfile.eof() as a loop termination

It's usually an error for the exit condition be a check for eof(). You could simplify a lot of your input routine by repacing that while loop with this one:

while ( in >> s1 >> s2 )

Name variables carefully

Naming a variable myfile doesn't say much other than it's a file. The "my" doesn't really add anything in this context. I'd be inclined to name it 'in' or 'infile' instead. Also you have variables named lead_zero and carry which are good, descriptive names, but also k1 and k2 which are not. Many coding standards recommend that the variable names i, j and k be reserved for loop variables, so I'd be inclinded to name these something like 'digit1anddigit2or maybe make an array and call themdigit[0]anddigit[1]. Yourcount3` variable is also only used within the loop, so you could declare it there like so:

for (int count3 = 0; i1!=final.end(); i1++)
{
    count3++;
    cout<<"The sum of the numbers on line "<<count3<<" is "<<*i1<<"\n";
}

Avoid needless math

The routine contains these lines:

//intermediate sum
int sum_inter=k1+k2+carry;
carry=0;

sum_string.insert(0,to_string(sum_inter%10)); 
carry+=int((sum_inter-sum_inter%10)/10);

It would be simpler like this:

int sum_inter = k1 + k2 + carry;
carry = sum_inter > 9 ? 1 : 0;
if (carry) sum_inter -= 10;
sum_string.insert(0,to_string(sum_inter));

sum_string doesn't need an initializer

Rather than string sum_string=""; you could omit the =""; and the effect is the same, but without the creation of a temporary string from "", the copy assignment, and then the destruction of the temporary string. (In truth, your compiler may be smart enough to optimize that all away, but why make it do all that work?)

Consider a more efficient algorithm

Rather than padding with leading zeroes, why not just iterate based on the length of the shorter string and then handle in another loop whatever's left from the longer string? Also, why not copy the longer string into the result and add in place? For example, choosing the first algorithm, first assure that s1 is always shorter or equal to the length of s2:

if (s1.length() > s2.length())
    std::swap(s1, s2);

Then you can process the shorter string's worth of digits:

    for (; it1!=s1.rend(); it1++, it2++)
    {
       int sum_inter=*it1-'0'+*it2-'0'+carry;
       carry = sum_inter > 9 ? 1 : 0;
       if (carry) sum_inter -= 10;
       sum_string.insert(0,to_string(sum_inter));
    }

And then rest of the longer one:

    for (; it2!=s2.rend(); it2++)
    {
       int sum_inter=*it2-'0'+carry;
       carry = sum_inter > 9 ? 1 : 0;
       if (carry) sum_inter -= 10;
       sum_string.insert(0,to_string(sum_inter));
    }

Eliminate spurious semicolon

One of your for loops ends with }; which will compile just fine, but the ; is spurious and should be removed.

Use standard library functions and algorithms where appropriate

Your current leading-zero suppression routine could be rewritten in one line

    sum_string.erase(0, sum_string.find_first_not_of("0"));

Even more efficient, however would be not to generate leading zeroes in the first place. That's the next item:

Avoid creating work

You can avoid needing to suppress leading zeroes by not creating them in the first place. First, only insert the final carry value if it isn't zero, and second, precondition the input strings so that they do not contain leading zeroes.

Use auto if you can

If your compiler has support for C++11, it should support the new auto syntax and range-for. Your code currently has this:

vector<string>::iterator i1=final.begin();
int count3 = 0;
for ( ; i1!=final.end(); i1++)
{
    count3++;
    cout<<"The sum of the numbers on line "<<count3<<" is "<<*i1<<"\n";
}

This could be simplified to:

int count3 = 0;
for (auto const &sum : final)
    cout<<"The sum of the numbers on line "<< ++count3<<" is "<<sum<<"\n";

Prefer const iterators

Your string iterators could be declared as

std::string::const_reverse_iterator it1 = s1.rbegin(), it2 = s2.rbegin();

This is primarily of benefit if you create a separate function which takes two const std::string & arguments, but it may help the compiler generate better code in any case. This could also be written as

auto it1 = s1.rbegin(), it2 = s2.rbegin();
share|improve this answer
    
Thanks a ton. I have a bunch of questions I'll try to post one by one –  Borat.sagdiyev May 15 at 13:48
    
Once again, our answers overlap x) –  Morwenn May 15 at 13:49
    
1. Why use auto? It's certainly shorter but is it explicitly defining the iterator type not practice for clarity? –  Borat.sagdiyev May 15 at 13:49
    
2. I can't use that math for carry because carry could be greater than 1. –  Borat.sagdiyev May 15 at 13:51
    
@Borat.sagdiyev: In the range-for, auto actually refers to a std::string rather than an iterator. This often makes it easier to use. –  Edward May 15 at 13:52

Rather than going back and forth about which syntax it's best to use for your loop to read from the file, I'd suggest that it's best to simply not write that loop yourself at all.

Instead, start by encapsulating your big-number into a class that supports operator>> to read a big number from a file, operator<< to insert a number to a file, and operator+ to add to big numbers together.

Then, use a standard algorithm to apply that to a file full of big numbers. The result should look something like this:

std::transform(std::istream_iterator<Bignum>(infile), std::istream_iterator<Bignum>(),
               std::ostream_iterator<Bignum>(outfile),
               std::plus<Bignum>());

As to your big-number code itself: right now, you seem to be carrying out operations one digit at a time. It'll typically be faster to store more digits per unit of storage, so you have fewer operations to carry out. A typical computer now has a 64-bit CPU. So, instead of storing one digit in an 8-bit number, you could store several digits in a 64-bit number. A 64-bit unsigned integer can store numbers up to 18446744073709551614. It's probably most convenient to use that for numbers from 0 to 999999999999999999 in each 64-bit integer. We then have a digit left over for any carry. This let's the CPU add 18 digits per operation instead of only one digit per operation.

That does require that we convert incoming numbers to that format as we read them, and convert outgoing numbers back to individual digits as we write them. The standard library already has code for both of those operations though, so about all we have to do is break the input into "chunks" of 18 or fewer digits, and use the library to handle the conversion itself.

Whether that's worthwhile or not will depend. If you're really just reading a file, adding two numbers, and writing out the result, it's probably not going to make any noticeable difference. The code will spend virtually all its time reading and writing files, so the time to do the addition probably won't make any difference at all. This being practical would hinge on whether you're likely to ever need to do more with the numbers (or in a different situation) so reading and writing files didn't completely dominate the situation.

share|improve this answer
    
This is exactly the kind of insightful thing I'm looking for. Thank you very much for your suggestion. Eventually I hope to expand this to a general equations parser of sorts, reading operands and values and storing results per line. Any OOD suggestions? –  Borat.sagdiyev May 15 at 23:52

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.