BigInteger class, supporting addition only

Question

Here is just addition, no multiplication or subtraction, but I will implement them later. Before going on, I'm here to ask for advice about the design.

#pragma once
#include <stdlib.h>
#include <crtdbg.h>
#include "stdafx.h"
#include <string>
#include <iostream>
#include <vector>
#include <cassert>
#include <algorithm>
#define to_int(x) (x - '0')
#define to_char(x) (char)(x + '0')
struct InvalidBigInt {};
class BigInteger
{
private:
    std::vector<char> vnum;
    std::string snum;
public:
    static bool flag;
    BigInteger() = default;
    BigInteger(const std::vector<char>& v) : vnum(v) {}
    BigInteger(std::string&& str) : snum(str)
    {
        vnum.clear();
        vnum = { str.crbegin(), str.crend() };
        try
        {
            if (std::find_if(str.begin(), str.end(),
                [](char c) { return !(isdigit(c)); }) != str.end())
            {
                throw InvalidBigInt();
            }
        }
        catch (InvalidBigInt)
        {
            std::cout << "*****************      Input invalid integer      *****************" << std::endl << \
                "*****************      Please input again         *****************" << std::endl;
        }
        flag = true;
    }
    friend std::ostream& operator<<(std::ostream& out, BigInteger bi)
    {
        std::for_each(bi.vnum.rbegin(), bi.vnum.rend(), [&](char c) {out << c; });
        return out;
    }
    BigInteger operator+(BigInteger b)
    {
        std::string result;
        auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum, less = b.vnum.size() < this->vnum.size() ? b.vnum : this->vnum;
        int temp = 0, digit = 0, diff = more.size() - less.size();
        for (size_t i = 0; i < diff; i++) { less.push_back('0'); }
        for (auto IteratorMore = more.begin(), IteratorLess = less.begin();
            IteratorMore < more.end() && IteratorLess < less.end();
            IteratorMore++, IteratorLess++)
        {
            digit = to_int(*IteratorMore) + to_int(*IteratorLess) + temp;
            if (digit > 9) { temp = 1; digit -= 10; }
            else { temp = 0; }
            result = to_char(digit) + result;
        }
        if (temp) { result = to_char(temp) + result; }
        return BigInteger(std::move(result));
    }
    bool operator==(BigInteger b)
    {
        return this->vnum == b.vnum;
    }
    ~BigInteger() noexcept {}
};
bool BigInteger::flag{ false };

int main()
{
    /*********************************PLUS*******************************/
    {
        assert(BigInteger("0") + BigInteger("0") == BigInteger("0"));
        assert(BigInteger("0") + BigInteger("1") == BigInteger("1"));
        assert(BigInteger("1023") + BigInteger("1") == BigInteger("1024"));
        assert(BigInteger("987") + BigInteger("34") == BigInteger("1021"));
        assert(BigInteger("98765") + BigInteger("23456") == BigInteger("122221"));
        assert(BigInteger("9230842083418927893758317489127418924781923748912") + BigInteger("81923743214617823686412783461237846178463")
            == BigInteger("9230842165342671108376141175540202386019769927375"));
    }
}

Answer 1

Abusing the use of macros

I think there is no good reason for implementing to_int() and to_char() as macros - such an implementation may result in hard to find bugs as you don't have any type checking. Lack of parameter types also makes it harder to understand what the function is expected to do in case of more complex functions.

Also, the names of these functions are misleading as they don't inform in a clear way that you are expecting an input to represent a single digit and any other input is invalid.

If you stick to the macro anyway, it's a common convention to use uppercase letters for macro names - most people would expect an ordinary function after seeing lowercase to_int()/to_char().

Single responsibility principle violation

Using catch this way:

catch (InvalidBigInt)
        {
            std::cout << "*****************      Input invalid integer      *****************" << std::endl << \
                "*****************      Please input again         *****************" << std::endl;
        }

you are restricting the use of your class to console applications. Things such as text display should be moved outside of your class as the responsibility of your class is to calculate numbers, not display them nor interact with a user via an interface.

Unclear variable names

Tha name flag is very vague and it's necessary to read the code to see what it's supposed to do (assuming it is used in the intended way). I'm not sure what is its purpose anyway since it's only set and not referred to anywhere in the code. I also suspect it shouldn't be a static variable.

Organizing code into headers and implementation files

Don't put all your code within your main.cpp file. It's more versatile and easier to manage if you separate it and put declarations in headers and definitions in .cpp files - you'll see that as soon as you decide to separate the text display functionality mentioned above from your main class.

Inconsistent and uncommon naming convention

Here:

for (auto IteratorMore = more.begin(), IteratorLess = less.begin();
    IteratorMore < more.end() && IteratorLess < less.end();
    IteratorMore++, IteratorLess++)

the names of the variables IteratorMore and IteratorLess start with a capital 'I', whereas it is a common practice to start the name of a class with the capital letters and start variable names with a lowercase. You are also following this convention correctly throughout your code, except in this part.

Const correctness

You don't always adhere to the concept of const correctness: your operators don't modify the BigInteger argument and thus it should be passed as a const reference to avoid unnecessary copies and make room for possible compiler optimizations.

Code lines' length

In some places in your code, you are writing very long lines, such as:

auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum, less = b.vnum.size() < this->vnum.size() ? b.vnum : this->vnum;

Even though most modern IDE's have built-in line wrapping, it's a good practice to write your code in such a way that a single line comprises approximately 80 characters. It's more comfortable and faster to read and comprehend the code if you are not forced to scroll it or move your eyeballs repeatedly from one side of a wide screen to another. Also that way you have control over where you wrap your code, whereas line wrapper functionality may visually organize the code in a way that makes it less readable. Also it's easier to use diff tools and open several editor windows side-by-side if you stick to this rule. See the discussion e.g. here:

https://stackoverflow.com/questions/276022/line-width-formatting-standard

Redundant variables

It seems you are not using snum anywhere in your code right now and even if you do use it in the future, it is probably to serve exactly the same puropse as vnum, so it's better to convert one type to another instead of keeping both and using only one of them depending on what's passed to the constructor - right now you would have to double your code by checking which variable is set inside each of your functions.

I would ditch vnum and use snum everywhere instead as std::string is a semantic equivalent of std::vector<char>, except you can benefit from small string optimization.

Answer 2

Code Review

The #pragma once is designed for header files. You should not use it in source files.

#pragma once

Also this pragma is limited to only a few compilers you should prefer standard header guards.

The stdafx.h is a pre-compiled header file for MS and and crtdbg.h is for debugging on MS.

#include "stdafx.h"
#include <crtdbg.h>

Its not very portable and only really part of the MS project.

This header files are part of the C language. Don't use it.

#include <stdlib.h>

The equivelent C++ header files is <cstdlib>.

You are playing a dangerous game with these macros.

#define to_int(x) (x - '0')
#define to_char(x) (char)(x + '0')

Remember macros don'y obey any scope rules and can quite easily obliterate code. Prefer to use functions rather than function like macros. With modern compilers and inlining you will not see any difference in generated code but have functions that behave well in terms of other language features:

inline int  to_int(char x) {return x - '0';}
inline char to_char(int x) {return x + '0';}

Of you three variables:

    std::vector<char> vnum;
    std::string snum;
    static bool flag;

Only vnum seems to have any purpose. snum is used locally in one of the constructors and flag is set to true on error for some reason but plays no other purpose.

You have a copy constructor using a vector.

    BigInteger(const std::vector<char>& v) : vnum(v) {}

Why not have a move const for vectors? The Constructor that takes a string foes a lot of validation. Why does this constructor do zero validation?

This constructor requires that you move a string (or provide an r-value reference). But you don't actually move the string you still make a copy!!

    BigInteger(std::string&& str) : snum(str)

Note: Named values can not themselves by r-value references (though this acts as a bind point for an r-value reference). To forward this value to snum you need to call std::move() to make sure you have an r-value reference and that is movable.

    BigInteger(std::string&& str) : snum(std::move(str))

You are in a constructor.The member vnum has just been default constructed (because you did not do anything in the initializer list). So this call to clear() is redundant.

        vnum.clear();

You are storing the number inverted:

vnum = { str.crbegin(), str.crend() };

Its a choice. I don't think its a good choice. I think this will make the rest of your code harder to write.

You check that all values in the string are digits:

            if (std::find_if(str.begin(), str.end(),
                [](char c) { return !(isdigit(c)); }) != str.end())
            {
                throw InvalidBigInt();
            }

But you don't test for the empty string. What value is the empty string? Is it zero. If you print out the empty string what will it display? It displays a blank. So that would confuse the user. So a blank/empty string is invalid input.

This try catch is redundant.

        try
        {
        }
        catch (InvalidBigInt)
        {
        }

You can use a simple test if you want to do things locally. throwing exceptions is what you do when you want to indicates an error that goes beyond the public interface of your class and thus you can not control how it will be received. Internally you have full control and thus exceptions are not expected.

But here you do want to throw an exception and let it escape your class. You do not want the user to be able to construct an invalid object. If the input is bad throw an exception. Force the user of your class to catch and deal with exceptions. If they do not deal with exceptions then the program should exit as you would be running code with an invalid object otherwise.

Printing is non mutating.

    friend std::ostream& operator<<(std::ostream& out, BigInteger bi)

As a result the bi parameter should be marked as a const. Also you are passing by value. Your object contains a vector so this is forcing a copy of the object just so it can be printed. So pass by const reference => BigInteger const& bi.

    friend std::ostream& operator<<(std::ostream& out, BigInteger const& bi)

You can simplify this for_each().

  std::for_each(bi.vnum.rbegin(), bi.vnum.rend(), [&](char c) {out << c; });

  // Alternative algorithm:
  std::copy(bi.vnum.rbegin(), bi.vnum.rend(), std::ostream_iterator<char>(out));

  // Use the range based for:
  for(auto const& val: boost::adaptors::reverse(bi.vnum)) {
      out << val;
  }

  // Personally I think this shows the issues with storing the
  // number the wrong way around. I would re-thing that decision.
  // Basically every interaction has to be reversed.

Rather than using the member functions begin() and end() you should prefer to use std::begin() and std::end() (or in your case std::rbegin() and std::rend()). This allows you to change the underlying data type without having to change the code. If you changed vnum to an array the code would continue to work as normal.

Passing parameters by value causes them to be copied.

    BigInteger operator+(BigInteger b)

You should pass them by reference (const reference) to avoid them being copied. Also the operator+ does not change the state of the current object so you should also mark it const

    BigInteger operator+(BigInteger const& b) const

Normally when you implement numeric types it is easier to specify the standard operators + - * / in terms of their assignment versions += -= *= /=.

    BigInteger operator+(BigInteger const& b) const
    {
        BigInteger result(*this);
        return result += b;
    }

Now you just need to implement operator+= and you get both.

This line is abusing the comma operator!!

        auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum, less = b.vnum.size() < this->vnum.size() ? b.vnum : this->vnum;

Split into two lines it is two operations after all:

       auto more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum;
       auto less = b.vnum.size() <  this->vnum.size() ? b.vnum : this->vnum;

Also you are performing a bunch of uneeded copies. Capture these values by reference rather than value;

       auto& more = b.vnum.size() >= this->vnum.size() ? b.vnum : this->vnum;
       auto& less = b.vnum.size() <  this->vnum.size() ? b.vnum : this->vnum;
       //  ^  Note the & sign. This make the variables references.

Avoid using this-> it hides errors.

The only reason to use this-> is to disambiguity a member variable from a shadowing local variable. If you forget to use this-> there is no error or warning you will one day mistake and accidentally forget and thus have an error in your program.

A better solution is to make your compiler warn (generate an error) you about shadowed members then make sure you change the name so it is not shadowed. Now you can never make the mistake of forgetting the this-> as a result less chance of a bug.

       auto& more = b.vnum.size() >= this->vnum.size() ? b.vnum : vnum;
       auto& less = b.vnum.size() <  this->vnum.size() ? b.vnum : vnum;

One variable declaration per line.

        int temp = 0, digit = 0, diff = more.size() - less.size();

There are no prizes for squeezing multiple declarations onto a line and it just hinders readability. There are a couple of corner cases where it can be an issue (but these are obtuse). But you should be writing the code so that it is easy to read. NOT so that it fits onto fewer lines.

Again:

        for (size_t i = 0; i < diff; i++) { less.push_back('0'); }

No prizes for compressed code. You are just making it harder to read:

        for (size_t i = 0; i < diff; i++) {
            less.push_back('0');
        }

If I see the operator==() defined:

    bool operator==(BigInteger b)

Then I expect the operator!=() to also be defined.

Destructor by default is noexcept there is no need to declare this.

    ~BigInteger() noexcept {}

Answer 3

Your code won't be very efficient.

Usually if you do such class, you probably want to either work at some power of 2 and use integers... or if you want to work in base 10, then you might want to pack multiple digits in x bytes (for ex. 2 digits per byte (BCD) or 9 digits in 4 bytes (2 unused bits).

There is also an efficiency problem for operator +. Using that function you always have to allocate new data. It is better to use operator += as the basic implementation as already mentioned in another review.

Most if not all other points in the other 2 reviews are valid.