Improving a custom integer class 2.0

As sort of a follow up to my previous question from a year ago, what are some suggestions you can think of that will improve this code, whether is directly programming related or algorithm related? I know there are better algorithms for multiplication and division, but I don't understand many of them (yet).

I have improved my code a lot since last year, but its still very slow.

Sorry for the long code, but there is a lot of stuff, even after removing over 100000 characters. Also, all this modifying might have messed up some of the formatting.

Please visit http://calccrypto.wikidot.com/programming:integer for the most up to date code. The code below is only for reference and may not have the bug fixes suggested put in

```
// Note: All of the operators have been templated or explicitly overloaded for standard
// integer types in the full code
// go to http://calccrypto.wikidot.com/programming:integer
// for an older version of this class because most functions haven't
// been touched for a long time

// i removed them because they took up so much space, and apparantly there
// is a limit of 300000 characters to post

#include
#include
#include

#ifndef __INTEGER__
#define __INTEGER__

class integer{
private:
bool SIGN; // false = positive, true = negative
std::deque value; // holds the actual value in base256

void clean(){ // remove 0 bytes from top of deque to save memory
while (value.size() && !value[0])
value.pop_front();
}

public:
// Constructors
integer(){SIGN = false;}

// similar for uint8_t, uint16_t, and uint64_t
integer(uint32_t rhs){
SIGN = false;
while (rhs){
value.push_front(rhs & 255);
rhs >>= 8;
}
}

// similar for int8_t, int16_t, and int64_t
integer(int32_t rhs){
SIGN = (rhs >= 8;
}
}

integer(const integer & rhs){
value = rhs.value;
SIGN = rhs.SIGN;
clean();
}

integer(std::deque & val){
value = val;
SIGN = false

I'm not familiar with the algorithms used (or any in the same category for that matter), but I do have a few notes on style/practices.

std::cerr/exit inside of a class

If a value is unexpected or invalid for a parameter, exceptions should be used, not outputting an error and then killing the program. What if the programmer wants to be able to recover from such an error? For example, what if you had a program that was taking hex input from users and the user provided an invalid character that ended up getting passed into integer(std::string str, int base). You probably would not want the program to die in the situation.

I would do something like:

else {
    throw std::runtime_error("Error: Character not between 'a' and 'f' found");
}

Same concept applies in the dm method.

SIGN

I'm guessing this is named in all uppercase because of the conflict with the sign method. I would consider using _sign or something similar instead. Everywhere I look in the code, the all caps property jumps out at me unnecessarily.

Also, I might consider renaming sign to isPositive. true and false conveys a different meaning to me than I except for a sign. I guess the use of a high and low bit for sign is by far standard enough for people to understand this behavior though.

integer

Extremely minor thing, and 100% opinion, but I would consider renaming the class.

integer makes me think of a standard int more so than an arbitrary length integer.

typedefs

I would use typedefs for a few things both to increase readability and lesser so maintainability.

For example, I might alias std::deque as value_type or something similar, and would use sign_type instead of bool.

clean

Very minor, but I would consider renaming this compact or trim.

Also, I would consider using empty instead of size since empty is constant complexity for all containers and size is only constant for some (though it is of course constant for deque).

initializer lists

It's more idiomatic to use initializer lists when possible (there's other benefits too, but in the case of your code, they're fairly insignificant).

For example:

integer() : SIGN(false) { }

integer(const integer & rhs) : SIGN(rhs.sign), value(rhs.value) {
    clean();
}

integer(std::deque  & val) : SIGN(false), value(val) {
    clean();
}

const correctness

There's a few places where const correctness is in place, but there's also some places where things could be const but are not.

For example:

integer(bool s, const std::deque  & val) {
    value = val;
    SIGN = value.size() ? s : false;
    clean();
}

type casting

I wouldn't provide the type casts (bool, uint8_t, etc). These only make sense if there is a meaningful cast to them, and in my opinion there is not. An arbitrary length integer is not meant to be cut into a smaller piece. I might expose a way to extract this data, but I would not do it via casts. (For example, a method to get the bottom N bytes or something.)

The ability to do this casting implicity especially concerns me. Imagine a method with signature void f(uint16_t). An integer being passed to by mistake could have some very odd implications.

cstdint

Not familiar enough with the C++ standard to comment on this for sure, but I suspect that you should be including cstdint explicity in integer.h rather than counting on a different header to pull it in.

integer(std::string str, int base)

This method has a few things going on.

I would try to avoid copying the string. That will of course require changing the body of the constructor.

base should have an unsigned type. A negative base has no meaning.

str shadows the method called str. In this situation, it doesn't matter, but shadowing should typically be avoided. (For what it's worth, I always compile with -Wshadow.)

match types where possible

In situations like:

for (unsigned int x = 0; x < str.size(); x++)

You should consider using:

for (std::string::size_type x = 0; x < str.size(); x++)

Also, since str.size() isn't changing, you should consider using:

for (std::string::size_type x = 0, s = str.size(); x < s; ++x)

integer(std::string str, int base) suggestion

I would get rid of the temp and rewrite it to be iterator based. This would end up being a lot more flexible while maintaining the existing functionality (and performance should be either the same, or very, very similar).

```
template
integer(Iterator start, const Iterator& end, uint16_t base)
{
if (start == end) {
return;
}
if (*start == '-') {
SIGN = true;
++start;
}

switch (base) {
case 2:
while (start != end) {
this = (this value.push_back(*start);
++start;
}
break;
default:
throw std::runtime_error("Unknown base provided (must be 2,8, 10, 16 or 256)");
break;
}
}

StackExchange Code Review Q#13424, answer score: 7