patterncppMinor
Arbitary Precision Addition
Viewed 0 times
precisionadditionarbitary
Problem
I made a bigint class that can add unsigned unlimited precision integers by splitting number into chunks less than 9999, and if a part is bigger than 9999 I take mod 10000 and carry 1 to the next part.
The adding function looks a bit messy as I had to make adjustments for the algorithm if rhs has more/less digits than lhs. Is there a way to make this programm more compact and readable?
The adding function looks a bit messy as I had to make adjustments for the algorithm if rhs has more/less digits than lhs. Is there a way to make this programm more compact and readable?
class bigint
{
std::vector parts;
public:
bigint(unsigned long long n);
friend std::ostream& operator 9999)
{
*a = *a%10000;
carry = 1;
}
++a; ++b;
}
// If rhs has more digits than lhs
if(a == parts.end() && b != n.parts.end())
{
while(b != n.parts.end())
{
parts.push_back(0);
a = --parts.end();
*a+=*b+carry;
carry = 0;
if(*a > 9999)
{
*a = *a%10000;
carry = 1;
}
++a; ++b;
}
}
// If lhs has more digits than rhs
if(b == n.parts.end() && a != parts.end())
{
while(a != n.parts.end() && carry == 1)
{
*a+=carry;
carry = 0;
if(*a > 9999)
{
*a = *a%10000;
carry = 1;
}
++a;
}
}
if(carry == 1)parts.push_back(1);
return *this;
}Solution
I have reviewed your code and here is what I've found.
Use const references where practical
The code currently declares its addition function like so:
This has two problems. First it passes by value, so a new
Eliminate "magic values"
The values of
(I'm using the C++11
Also use the same constant everywhere. That is, instead of writing this:
write this:
The advantage is that the compiler can probably generate more efficient code because it only has a single constant instead of two.
Be wary of signed versus unsigned
The individual
Avoid computationally costly operations if practical
If we have two "digits" (digits in quotation marks because they're really base 10000 rather than base 10), each with a maximum value of 9999, then the maximum sum is 19998. This means that the carry can only be 1 or 0 and that the code can be rewritten to eliminate the computationally costly
Eliminate redundant condition checks
In the addition code, the first loop only ends if one of the iterators is at the end. This means that the next couple of lines of code:
Can be simplified to just the enclosed
Extract identical code into a single place
The code for the addition of digits is repeated three places, which is a very strong hit that it should probably be made into its own member function. One way to do it:
Use
With the function listed above, the first loop can be rewritten as:
It's already much simpler.
Make sure your constructor always works
If the value passed is
Make sure your
Consider the case in which the value stored is
Use const references where practical
The code currently declares its addition function like so:
bigint& bigint::operator+=(bigint n)This has two problems. First it passes by value, so a new
bigint is created on every call. This is quite wasteful of both time and memory. Second, it should actually be a const reference.bigint& bigint::operator+=(const bigint &n)Eliminate "magic values"
The values of
10000 and 9999 are sprinkled through the code, but they really ought to be a named constant instead, and specifically a named constant static member.constexpr static int MAXVAL = 10000;(I'm using the C++11
constexpr here, but it could be const if you're using some old compiler.)Also use the same constant everywhere. That is, instead of writing this:
if (*a > 9999)write this:
if (*a >= MAXVAL)The advantage is that the compiler can probably generate more efficient code because it only has a single constant instead of two.
Be wary of signed versus unsigned
The individual
parts should probably be unsigned rather than signed. Since the sole defined constructor only takes unsigned numbers, it's reasonable to assume that the constituent parts are also unsigned.Avoid computationally costly operations if practical
If we have two "digits" (digits in quotation marks because they're really base 10000 rather than base 10), each with a maximum value of 9999, then the maximum sum is 19998. This means that the carry can only be 1 or 0 and that the code can be rewritten to eliminate the computationally costly
% operator:{
*a += *b + carry;
if (*a >= MAXVAL)
{
*a -= MAXVAL;
carry = 1;
} else {
carry = 0;
}
++a; ++b;
}Eliminate redundant condition checks
In the addition code, the first loop only ends if one of the iterators is at the end. This means that the next couple of lines of code:
// If rhs has more digits than lhs
if(a == parts.end() && b != n.parts.cend())
{
while(b != n.parts.cend())
{Can be simplified to just the enclosed
while statement.Extract identical code into a single place
The code for the addition of digits is repeated three places, which is a very strong hit that it should probably be made into its own member function. One way to do it:
int bigint::digitAdd(int &a, int b, int carry)
{
a+=b+carry;
if(a >= MAXVAL)
{
a -= MAXVAL;
carry = 1;
} else {
carry = 0;
}
return carry;
}Use
for rather than while where appropriateWith the function listed above, the first loop can be rewritten as:
for(carry = 0; a != parts.end() && b != n.parts.cend(); ++a, ++b)
{
carry = digitAdd(*a, *b, carry);
}It's already much simpler.
Make sure your constructor always works
If the value passed is
0 the constructor will create no members in the parts vector. You might want to reconsider your logic for that case.Make sure your
operator<< does what you really wantConsider the case in which the value stored is
10007. The current operator<< will print "17" which is probably not what is desired.Code Snippets
bigint& bigint::operator+=(bigint n)bigint& bigint::operator+=(const bigint &n)constexpr static int MAXVAL = 10000;if (*a > 9999)if (*a >= MAXVAL)Context
StackExchange Code Review Q#92399, answer score: 4
Revisions (0)
No revisions yet.