patterncppMinor
Large integer class for storing integer data as a char array
Viewed 0 times
classarraychardatalargeforstoringinteger
Problem
I am working on a
I would like to find a way to further streamline the addition/subtraction. I think it ought to be possible to only use a single loop and manipulate the signs and order to produce the proper result, but all attempts thus far have failed.
The only things I am unwilling to change:
This does not use proper C++ components such as
Everything else is fair game, and I welcome any and all criticisms. This is one of the first C++ classes I have made, so I expect that my coding is sub-par.
LargeInt.h
```
#ifndef LARGEINT_H
#define LARGEINT_H
#include
typedef unsigned int Counter;
typedef char Digit;
typedef unsigned int Index;
typedef bool Sign;
const bool kSignPositive = 0;
const bool kSignNegative = 1;
const bool kCarryTrue = 1;
const bool kCarryFalse = 0;
const bool kBorrowTrue = 1;
const bool kBorrowFalse = 0;
const Counter kDefaultCounterValue = 0;
const Digit kDefaultDigitValue = 0;
const Digit kMinDigitValue = 0;
const Digit kMaxDigitValue = 9;
const Digit kRadix = 10;
const Index kFirstIndex = 0;
const size_t kByteSize = 8;
const size_t kMinLength = 1;
namespace helper_templates
{
// Count Digits
template
Counter TCountDigits(T x);
// Get Sign
template
bool TGetSign(T x);
}
class LargeInt
{
private:
Sign sign_;
size_t length_;
Digit* data_;
public:
/*
* Constructors
*/
// Default
LargeInt();
// Copy
LargeInt(const LargeInt& x);
// Copy and Resize
LargeInt(const S
LargeInteger class to store Integers beyond the size of long long as a dynamic character array. I have overloaded most of the relevant operators to be used with the large integers.I would like to find a way to further streamline the addition/subtraction. I think it ought to be possible to only use a single loop and manipulate the signs and order to produce the proper result, but all attempts thus far have failed.
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)
This does not use proper C++ components such as
std::vector, std::copy and others. My goals in this project were to avoid the use of the standard (or other) library aside from I/O.Everything else is fair game, and I welcome any and all criticisms. This is one of the first C++ classes I have made, so I expect that my coding is sub-par.
LargeInt.h
```
#ifndef LARGEINT_H
#define LARGEINT_H
#include
typedef unsigned int Counter;
typedef char Digit;
typedef unsigned int Index;
typedef bool Sign;
const bool kSignPositive = 0;
const bool kSignNegative = 1;
const bool kCarryTrue = 1;
const bool kCarryFalse = 0;
const bool kBorrowTrue = 1;
const bool kBorrowFalse = 0;
const Counter kDefaultCounterValue = 0;
const Digit kDefaultDigitValue = 0;
const Digit kMinDigitValue = 0;
const Digit kMaxDigitValue = 9;
const Digit kRadix = 10;
const Index kFirstIndex = 0;
const size_t kByteSize = 8;
const size_t kMinLength = 1;
namespace helper_templates
{
// Count Digits
template
Counter TCountDigits(T x);
// Get Sign
template
bool TGetSign(T x);
}
class LargeInt
{
private:
Sign sign_;
size_t length_;
Digit* data_;
public:
/*
* Constructors
*/
// Default
LargeInt();
// Copy
LargeInt(const LargeInt& x);
// Copy and Resize
LargeInt(const S
Solution
The only things I am unwilling to change:
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
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).
Assignment operator is broken
Algorithms.
You spend way to much time re-writing the same loop.
Much easier to use algorithms to all this kind of work.
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
This can be replaced with:
The test itself is very ineffecient:
Here you are creating a
Minus 0
Also is
Enum
The following could all be enum values.
Radix
Using a radix of 10 is very eneffecient.
This not only causes extra storage but also extra operations when moving through each digit.
- 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. 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 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 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.Minus 0
Also is
-0 not the same as +0 (in terms of testability). I don't think your test actually gets that correct.LargeInt x; // +0
LargeInt y = -x; // -0
if (x == y)
{
// I would expect this to be true.
// If you expect this to be different then I would document
// that fact somewhere.
}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.
Code Snippets
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.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;
}for(Index i = kFirstIndex; i < this->length_; i++)
{
this->data_[i] = x.data_[i];
}std::copy(x.data + kFirstIndex, x.data + length, data);if (x)
return true;
else return false;Context
StackExchange Code Review Q#122188, answer score: 2
Revisions (0)
No revisions yet.