Return to Answer

• 1

• 97.7k
• 5
• 126
• 341

• 97.7k
• 5
• 126
• 341

The only things I am unwilling to change:

• Storing the data as a dynamic char array.
• The attributes of the operators (friend, overloaded, return type, parameters).

Well there is not much point doing more of a review. As both of these things are done incorrectly to start with. There are some major problems here to start with.

Storing

Separation of concerns is basically about a class only doing one thing. It either does resource allocation or business logic. Your class is doing both and as a result doing it badly.

Also there is already a specific class that does exactly this std::vector<char>. This class is highly efficient highly tested and is going to beat your memory management every time (unless you are experimenting you should be using it).

You miss move semantics. Which makes your code less efficient than it could be.

You also don't have the concept of capacity so you can't preallocate and must reallocate the correct size all the time which means sizing all the time which is very inefficient.

Friendship.

Your use of friendship is inconsistent. As a result it behaves differently than you would expect from (if you expect it to work like built in types).

int x = 5; int y = 6; int z = 3;

y = x += z; // This should work.

// Now with your type.

int x = 5; LargeInt y = 6; LargeInt z = 3;

y = x += z; // Not sure why this would not work.

Assignment operator is broken

LargeInt& LargeInt::operator=(const LargeInt& x)
{
 if(this != &x)
 {
 this->sign_ = x.sign_;
 
 // This is wrong.
 // This is the size of the allocated area of memory.
 // If you don't keep this accurate you are going to 
 // overwrite the end of the array if you are not careful
 // You are mixing the concept of size and capacity into a
 // single variable.
 this->length_ = x.length_;
 for(Index i = kFirstIndex; i < this->length_; i++)
 {
 this->data_[i] = x.data_[i];
 }
 }
 return *this;
}

Algorithms.

You spend way to much time re-writing the same loop.

 for(Index i = kFirstIndex; i < this->length_; i++)
 {
 this->data_[i] = x.data_[i];
 }

Much easier to use algorithms to all this kind of work.

 std::copy(x.data + kFirstIndex, x.data + length, data);

DRY

Is the += operator not basically the same as the -= operator? I am sure you can reuse about 90% of that code.

Operator !

If you have this pattern

if (x)
 return true;
else return false;

This can be replaced with:

return x;

The test itself is very ineffecient:

if (*this == LargeInt())

Here you are creating a LargeInt() object each time you do the test. Which means memory allocation and de-allocation every time. It may be worth creating a static function object so that it is only created once.

Enum

The following could all be enum values.

const bool kSignPositive = 0;
const bool kSignNegative = 1;
const bool kCarryTrue = 1;
const bool kCarryFalse = 0;
const bool kBorrowTrue = 1;
const bool kBorrowFalse = 0;

Radix

Using a radix of 10 is very eneffecient.

const Digit kDefaultDigitValue = 0;
const Digit kMinDigitValue = 0;
const Digit kMaxDigitValue = 9;
const Digit kRadix = 10;

This not only causes extra storage but also extra operations when moving through each digit.